Re: [RFC 01/10] crypto: factor async completion for general use
Hi Gilad,
On Sat, May 06, 2017 at 03:59:50PM +0300, Gilad Ben-Yossef wrote:
> Invoking a possibly async. crypto op and waiting for completion
> while correctly handling backlog processing is a common task
> in the crypto API implementation and outside users of it.
>
> This patch re-factors one of the in crypto API implementation in
> preparation for using it across the board instead of hand
> rolled versions.
Thanks for doing this! It annoyed me too that there wasn't a helper function
for this. Just a few comments below:
> diff --git a/crypto/af_alg.c b/crypto/af_alg.c
> index 3556d8e..bf4acaf 100644
> --- a/crypto/af_alg.c
> +++ b/crypto/af_alg.c
> @@ -480,33 +480,6 @@ int af_alg_cmsg_send(struct msghdr *msg, struct
> af_alg_control *con)
> }
> EXPORT_SYMBOL_GPL(af_alg_cmsg_send);
>
> -int af_alg_wait_for_completion(int err, struct af_alg_completion *completion)
> -{
> - switch (err) {
> - case -EINPROGRESS:
> - case -EBUSY:
> - wait_for_completion(&completion->completion);
> - reinit_completion(&completion->completion);
> - err = completion->err;
> - break;
> - };
> -
> - return err;
> -}
> -EXPORT_SYMBOL_GPL(af_alg_wait_for_completion);
> -
> -void af_alg_complete(struct crypto_async_request *req, int err)
> -{
> - struct af_alg_completion *completion = req->data;
> -
> - if (err == -EINPROGRESS)
> - return;
> -
> - completion->err = err;
> - complete(&completion->completion);
> -}
> -EXPORT_SYMBOL_GPL(af_alg_complete);
> -
I think it would be cleaner to switch af_alg and algif_* over to the new
interface in its own patch, rather than in the same patch that introduces the
new interface.
> @@ -88,8 +88,8 @@ static int hash_sendmsg(struct socket *sock, struct msghdr
> *msg,
> if ((msg->msg_flags & MSG_MORE))
> hash_free_result(sk, ctx);
>
> - err = af_alg_wait_for_completion(crypto_ahash_init(&ctx->req),
> - &ctx->completion);
> + err = crypto_wait_req(crypto_ahash_init(&ctx->req),
> + &ctx->wait);
> if (err)
> goto unlock;
> }
In general can you try to keep the argument lists indented sanely? (This
applies throughout the patch series.) e.g. here I'd have expected:
err = crypto_wait_req(crypto_ahash_init(&ctx->req),
&ctx->wait);
>
> diff --git a/crypto/api.c b/crypto/api.c
> index 941cd4c..1c6e9cd 100644
> --- a/crypto/api.c
> +++ b/crypto/api.c
> @@ -24,6 +24,7 @@
> #include
> #include
> #include
> +#include
> #include "internal.h"
>
> LIST_HEAD(crypto_alg_list);
> @@ -595,5 +596,32 @@ int crypto_has_alg(const char *name, u32 type, u32 mask)
> }
> EXPORT_SYMBOL_GPL(crypto_has_alg);
>
> +int crypto_wait_req(int err, struct crypto_wait *wait)
> +{
> + switch (err) {
> + case -EINPROGRESS:
> + case -EBUSY:
> + wait_for_completion(&wait->completion);
> + reinit_completion(&wait->completion);
> + err = wait->err;
> + break;
> + };
> +
> + return err;
> +}
> +EXPORT_SYMBOL_GPL(crypto_wait_req);
crypto_wait_req() maybe should be inlined, since it doesn't do much (note that
reinit_completion is actually just a variable assignment), and the common case
is that 'err' will be 0, so there will be nothing to wait for.
Also drop the unnecessary semicolon at the end of the switch block.
With regards to the wait being uninterruptible, I agree that this should be the
default behavior, because I think users waiting for specific crypto requests are
generally not prepared to handle the wait actually being interrupted. After
interruption the crypto operation will still proceed in the background, and it
will use buffers which the caller has in many cases already freed. However, I'd
suggest taking a close look at anything that was actually doing an interruptible
wait before, to see whether it was a bug or intentional (or "doesn't matter").
And yes there could always be a crypto_wait_req_interruptible() introduced if
some users need it.
>
> #define CRYPTO_MINALIGN_ATTR __attribute__ ((__aligned__(CRYPTO_MINALIGN)))
>
> +/*
> + * Macro for declaring a crypto op async wait object on stack
> + */
> +#define DECLARE_CRYPTO_WAIT(_wait) \
> + struct crypto_wait _wait = { \
> + COMPLETION_INITIALIZER_ONSTACK((_wait).completion), 0 }
> +
Move this definition down below, so it's next to crypto_wait?
>
> /*
> * Algorithm registration interface.
> */
> @@ -1604,5 +1631,6 @@ static inline int crypto_comp_decompress(struct
> crypto_comp *tfm,
> src, slen, dst, dlen);
> }
>
> +
Don't add unrelated blank lines.
- Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message
Re: [RFC 07/10] fscrypt: move to generic async completion
Hi Gilad, On Sat, May 06, 2017 at 03:59:56PM +0300, Gilad Ben-Yossef wrote: > int fscrypt_do_page_crypto(const struct inode *inode, fscrypt_direction_t rw, > u64 lblk_num, struct page *src_page, > struct page *dest_page, unsigned int len, > @@ -150,7 +135,7 @@ int fscrypt_do_page_crypto(const struct inode *inode, > fscrypt_direction_t rw, > u8 padding[FS_XTS_TWEAK_SIZE - sizeof(__le64)]; > } xts_tweak; > struct skcipher_request *req = NULL; > - DECLARE_FS_COMPLETION_RESULT(ecr); > + DECLARE_CRYPTO_WAIT(ecr); > struct scatterlist dst, src; > struct fscrypt_info *ci = inode->i_crypt_info; > struct crypto_skcipher *tfm = ci->ci_ctfm; > @@ -168,7 +153,7 @@ int fscrypt_do_page_crypto(const struct inode *inode, > fscrypt_direction_t rw, [...] This patch looks good --- thanks for doing this! I suggest also renaming 'ecr' to 'wait' in the places being updated to use crypto_wait, since the name is obsolete (and was already; it originally stood for "ext4 completion result"). - Eric -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC 01/10] crypto: factor async completion for general use
On Thu, May 11, 2017 at 10:29:47AM +0300, Gilad Ben-Yossef wrote: > > With regards to the wait being uninterruptible, I agree that this should be > > the > > default behavior, because I think users waiting for specific crypto > > requests are > > generally not prepared to handle the wait actually being interrupted. After > > interruption the crypto operation will still proceed in the background, and > > it > > will use buffers which the caller has in many cases already freed. > > However, I'd > > suggest taking a close look at anything that was actually doing an > > interruptible > > wait before, to see whether it was a bug or intentional (or "doesn't > > matter"). > > > > And yes there could always be a crypto_wait_req_interruptible() introduced > > if > > some users need it. > > So this one was a bit of a shocker. I though the _interruptible use > sites seemed > wrong in the sense of being needless. However, after reading your feedback and > reviewing the code I'm pretty sure every single one of them (including > the one I've > added in dm-verity-target.c this merge window) are down right dangerous and > can cause random data corruption... so thanks for pointing this out! > > I though of this patch set as a "make the code pretty" for 4.13 kind > of patch set. > Looks like it's a bug fix now, maybe even stable material. > > Anyway, I'll roll a v2 and we'll see. > Any that are called only by kernel threads would theoretically be safe since kernel threads don't ordinarily receive signals. But I think that at least the drbg and gcm waits can be reached by user threads, since they can be called via algif_rng and algif_aead respectively. I recommend putting any important fixes first, so they can be backported without depending on crypto_wait_req(). Eric -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] fscrypt: add a documentation file for filesystem-level encryption
From: Eric Biggers Perhaps long overdue, add a documentation file for filesystem-level encryption, a.k.a. fscrypt or fs/crypto/, to the Documentation directory. The new file is based loosely on the latest version of the "EXT4 Encryption Design Document (public version)" Google Doc, but with many improvements made, including: - Reflect the reality that it is not specific to ext4 anymore. - More thoroughly document the design and user-visible API/behavior. - Replace outdated information, such as the outdated explanation of how encrypted filenames are hashed for indexed directories and how encrypted filenames are presented to userspace without the key. (This was changed just before release.) For now the focus is on the design and user-visible API/behavior, not on how to add encryption support to a filesystem --- since the internal API is still pretty messy and any standalone documentation for it would become outdated as things get refactored over time. Signed-off-by: Eric Biggers --- Documentation/filesystems/fscrypt.rst | 587 ++ Documentation/filesystems/index.rst | 11 + MAINTAINERS | 1 + 3 files changed, 599 insertions(+) create mode 100644 Documentation/filesystems/fscrypt.rst diff --git a/Documentation/filesystems/fscrypt.rst b/Documentation/filesystems/fscrypt.rst new file mode 100644 index ..633d859a0ab1 --- /dev/null +++ b/Documentation/filesystems/fscrypt.rst @@ -0,0 +1,587 @@ += +Filesystem-level encryption (fscrypt) += + +Introduction + + +fscrypt is a library which filesystems can hook into to support +transparent encryption of files and directories. + +Note: "fscrypt" in this document refers to the kernel-level portion, +implemented in ``fs/crypto/``, as opposed to the userspace tool +`fscrypt <https://github.com/google/fscrypt>`_. This document only +covers the kernel-level portion. For command-line examples of how to +use encryption, see the documentation for the userspace tool `fscrypt +<https://github.com/google/fscrypt>`_. + +Unlike dm-crypt, fscrypt operates at the filesystem level rather than +at the block device level. This allows it to encrypt different files +with different keys and to have unencrypted files on the same +filesystem. This is useful for multi-user systems where each user's +data-at-rest needs to be cryptographically isolated from the others. +However, except for filenames, fscrypt does not encrypt filesystem +metadata. + +Unlike eCryptfs, which is a stacked filesystem, fscrypt is integrated +directly into supported filesystems --- currently ext4, F2FS, and +UBIFS. This allows encrypted files to be read and written without +caching both the decrypted and encrypted pages in the pagecache, +thereby halving the memory used and bringing it in line with +unencrypted files. Similarly, half as many dentries and inodes are +needed. eCryptfs also limits filenames to 143 bytes, causing +application compatibility issues; fscrypt allows the full 255 bytes +(NAME_MAX). Finally, unlike eCryptfs, the fscrypt API can be used by +unprivileged users, with no need to mount anything. + +fscrypt does not support encrypting files in-place. Instead, it +supports marking an empty directory as encrypted. Then, after +userspace provides the key, all regular files, directories, and +symbolic links created in that directory tree are transparently +encrypted. + +Threat model + + +Offline attacks +--- + +Provided that userspace chooses a strong encryption key, fscrypt +protects the confidentiality of file contents and filenames in the +event of a single point-in-time permanent offline compromise of the +block device content. fscrypt does not protect the confidentiality of +non-filename metadata, e.g. file sizes, file permissions, file +timestamps, and extended attributes. Also, the existence and location +of holes (unallocated blocks which logically contain all zeroes) in +files is not protected. + +fscrypt is not guaranteed to protect confidentiality or authenticity +if an attacker is able to manipulate the filesystem offline prior to +an authorized user later accessing the filesystem. + +Online attacks +-- + +fscrypt (and storage encryption in general) can only provide limited +protection, if any at all, against online attacks. In detail: + +fscrypt is only resistant to side-channel attacks, such as timing or +electromagnetic attacks, to the extent that the underlying Linux +Cryptographic API algorithms are. If a vulnerable algorithm is used, +such as a table-based implementation of AES, it may be possible for an +attacker to mount a side channel attack against the online system. + +After an encryption key has been provided, fscrypt is not designed to +hide the plaintext file contents or filenames from other users on the +same system
Re: [PATCH] fscrypt: add a documentation file for filesystem-level encryption
On Sat, Aug 19, 2017 at 10:32:27PM -0400, Theodore Ts'o wrote: > On Fri, Aug 18, 2017 at 03:06:52PM -0600, Andreas Dilger wrote: > > On Aug 18, 2017, at 1:47 PM, Eric Biggers wrote: > > > +Key hierarchy > > > += > > > + > > > +Master Keys > > > +--- > > > + > > > +Userspace should generate master keys either using a cryptographically > > > +secure random number generator, e.g. by reading from ``/dev/urandom`` > > > +or calling getrandom(), or by using a KDF (Key Derivation Function). > > > +Note that whenever a KDF is used to "stretch" a lower-entropy secret > > > +such as a passphrase, it is critical that a KDF designed for this > > > +purpose be used, such as scrypt, PBKDF2, or Argon2. > > > > One minor suggestion - when generating a master key for a filesystem, > > I'd think it is preferable to use /dev/random instead of /dev/urandom > > to ensure there is enough entropy. > > I would just say "use getrandom" and be done with it. More > importantly, we probably just want to direct users to use either > https://github.com/google/fscrypt or Android key management system at > the beginning of the file. > > If the readers of this documentation file need to be told how to get > good random number generators, they're very likely to make any number > of other basic security mistakes. If people don't think the fscrypt > user program isn't user-friendly or flexible enough to encompass their > use case, it's probably better to encourage them to submit > enhancements to an existing open source key management system such as > google/fscrypt. If we minimize the number of userspace > implementations, the easier it will be to make sure they are > appropriately audited for security issues... > Yes, the intent wasn't really to start a debate about /dev/random vs. /dev/urandom vs. getrandom(). I'll probably just remove the text "e.g. by reading from /dev/urandom or calling getrandom()". I agree that we should recommend the 'fscrypt' userspace tool more strongly as well. I did mention it at the beginning of the document, but I didn't explicitly say something like "use this instead of writing your own tool". Either way, the kernel API should still be fully documented. And part of that is documenting that the master key needs to be a good pseudorandom key. Otherwise I could imagine someone mistakenly thinking that the filesystem does key stretching for them, for example. (Yes, that person probably wouldn't read the documentation and would screw up anyway, but we might as well try.) Eric -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fscrypt: add a documentation file for filesystem-level encryption
On Mon, Aug 21, 2017 at 09:44:11PM +0800, Anand Jain wrote: > > > >+fscrypt is not guaranteed to protect confidentiality or authenticity > >+if an attacker is able to manipulate the filesystem offline prior to > >+an authorized user later accessing the filesystem. > > How does fscrypt / Android protect against Evil Maid attack. ? > > Thanks, Anand As Ted mentioned, it really depends on the type of attack. If we assume that the attacker can *only* change the contents of disk, then there is a protection against a specific type of attack. Android has Verified Boot, which verifies the integrity and authenticity of the kernel and the 'system' partition: https://source.android.com/security/verifiedboot/ >From there, the vold binary (which has been authenticated using dm-verity) is able to unwrap the encryption keys (which are authenticated using AES-GCM), then check that the encrypted directories have the correct encryption policies. The filesystem then enforces the one-policy-per-tree constraint, as described in my proposed documentation: Except for those special files, it is forbidden to have unencrypted files, or files encrypted with a different encryption policy, in an encrypted directory tree. Attempts to link or rename such a file into an encrypted directory will fail with EPERM. This is also enforced during ->lookup() to provide limited protection against offline attacks that try to disable or downgrade encryption in known locations where applications may later write sensitive data. So on Android, an "Evil Maid" attacker cannot simply replace an encrypted directory with an unencrypted one, causing a program to write unencrypted files to that directory. _However_, an "Evil Maid" attacker can probably still do other, perhaps much more effective attacks --- e.g. installing a hardware "key logger", or perhaps installing a binary into /data in such a way that it gets auto-executed and compromises the system after the user logs in. Or they could attack the actual file contents encryption which is not authenticated. Or they could mess around with filesystem metadata on the userdata partition, which is neither encrypted nor authenticated. I suppose that dm-integrity could be used to protect against some of those attacks, but of course it would not protect against hardware key loggers, etc. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fscrypt: add a documentation file for filesystem-level encryption
On Tue, Aug 22, 2017 at 10:22:30AM +0800, Anand Jain wrote: > > Hi Eric, > > How about a section on the threat model specific to the file-name ? > > (Sorry if I am missing something). > > Thanks, Anand It's already mentioned that filenames are encrypted: "fscrypt protects the confidentiality of file contents and filenames in the event of a single point-in-time permanent offline compromise of the block device content." There's not much more to it than that; all the other points in the "Threat model" section (offline manipulations, timing attacks, access control, key eviction, etc.) are essentially the same between contents and filenames encryption. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fscrypt: add a documentation file for filesystem-level encryption
On Tue, Aug 22, 2017 at 10:22:13AM +0800, Anand Jain wrote: > > > > > > > +fscrypt is not guaranteed to protect confidentiality or authenticity > > > > +if an attacker is able to manipulate the filesystem offline prior to > > > > +an authorized user later accessing the filesystem. > > > > > > How does fscrypt / Android protect against Evil Maid attack. ? > > > _However_, an "Evil Maid" attacker can probably still do other, perhaps much > > more effective attacks --- e.g. > :: > > . Or they could attack the actual > > file contents encryption which is not authenticated. Or they could mess > > around > > with filesystem metadata on the userdata partition, which is neither > > encrypted > > nor authenticated. > > In specific, the scenario I had in mind was the above threat. > > > I suppose that dm-integrity could be used to protect against some of those > > attacks, but of course it would not protect against hardware key loggers, > > etc. > > OK. > > > I think AE is the only good solution for this, File-name encryption at > this stage won't solve any kind of Evil Maid attack, (as it was quoted > somewhere else in ML). > > > Further, below, is define but not used. > - > #define FS_AES_256_GCM_KEY_SIZE 32 > - > Yes, authenticated encryption with AES-256-GCM was in an older version of the ext4 encryption design document. But unfortunately it was never really thought through. The primary problem, even ignoring rollback protection, is that there is nowhere to store the per-block metadata (GCM authentication tag and IV) *and* have it updated atomicly with the block contents. Recently, dm-integrity solves this at the block device layer, but it uses data journaling which is very inefficient. This maybe could be implemented more efficiently on a COW filesystem like BTRFS. But even after that, another problem is that authenticated encryption of file contents only would not stop an attacker from swapping around blocks, files, directories, or creating links, etc. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fscrypt: add a documentation file for filesystem-level encryption
On Tue, Aug 22, 2017 at 11:33:51PM +0800, Anand Jain wrote: > > > On 08/22/2017 10:55 AM, Eric Biggers wrote: > >On Tue, Aug 22, 2017 at 10:22:30AM +0800, Anand Jain wrote: > >> > >>Hi Eric, > >> > >> How about a section on the threat model specific to the file-name ? > >> > >> (Sorry if I am missing something). > >> > >>Thanks, Anand > > > >It's already mentioned that filenames are encrypted: "fscrypt protects the > >confidentiality of file contents and filenames in the event of a single > >point-in-time permanent offline compromise of the block device content." > >There's not much more to it than that; all the other points in the "Threat > >model" section (offline manipulations, timing attacks, access control, key > >eviction, etc.) are essentially the same between contents and filenames > >encryption. > > Do you think if application does not keep the sensitive information > in the file-name, would that remove the file-name from the list of > items that should be protected ? > If *no* applications care whether the filenames are encrypted or not, sure. But are you absolutely sure that no applications care? How do you know? And what is the advantage of not encrypting the filenames anyway? It is better to encrypt by default. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fscrypt: add a documentation file for filesystem-level encryption
On Tue, Aug 22, 2017 at 11:35:20PM +0800, Anand Jain wrote: > >> > >> I think AE is the only good solution for this, File-name encryption at > >>this stage won't solve any kind of Evil Maid attack, (as it was quoted > >>somewhere else in ML). > >> > >> > >> Further, below, is define but not used. > >>- > >> #define FS_AES_256_GCM_KEY_SIZE 32 > >>- > >> > > > >Yes, authenticated encryption with AES-256-GCM was in an older version of the > >ext4 encryption design document. But unfortunately it was never really > >thought > >through. The primary problem, even ignoring rollback protection, is that > >there > >is nowhere to store the per-block metadata (GCM authentication tag and IV) > >*and* > >have it updated atomicly with the block contents. Recently, dm-integrity > >solves > >this at the block device layer, but it uses data journaling which is very > >inefficient. This maybe could be implemented more efficiently on a COW > >filesystem like BTRFS. But even after that, another problem is that > >authenticated encryption of file contents only would not stop an attacker > >from > >swapping around blocks, files, directories, or creating links, etc. > > > Some of the problems to be solved in this area are quite > interesting and challenging and IMO BTRFS fits nicely. Per extent AE > for BTRFS is drafted, it needs scrutiny and constructive feedback. > > Thanks, Anand > > > >Eric > > Where is the code? Is there a design document, and it is it readable by people not as familiar with btrfs? Is the API compatible with ext4, f2fs, and ubifs? Eric -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fscrypt: add a documentation file for filesystem-level encryption
Hi Anand, On Mon, Aug 28, 2017 at 08:18:46PM +0800, Anand Jain wrote: > > > On 08/23/2017 01:07 AM, Eric Biggers wrote: > >On Tue, Aug 22, 2017 at 11:33:51PM +0800, Anand Jain wrote: > >> > >> > >>On 08/22/2017 10:55 AM, Eric Biggers wrote: > >>>On Tue, Aug 22, 2017 at 10:22:30AM +0800, Anand Jain wrote: > >>>> > >>>>Hi Eric, > >>>> > >>>> How about a section on the threat model specific to the file-name ? > >>>> > >>>> (Sorry if I am missing something). > >>>> > >>>>Thanks, Anand > >>> > >>>It's already mentioned that filenames are encrypted: "fscrypt protects the > >>>confidentiality of file contents and filenames in the event of a single > >>>point-in-time permanent offline compromise of the block device content." > >>>There's not much more to it than that; all the other points in the "Threat > >>>model" section (offline manipulations, timing attacks, access control, key > >>>eviction, etc.) are essentially the same between contents and filenames > >>>encryption. > >> > >> Do you think if application does not keep the sensitive information > >>in the file-name, would that remove the file-name from the list of > >>items that should be protected ? > >> > > > >If *no* applications care whether the filenames are encrypted or not, sure. > >But are you absolutely sure that no applications care? How do you know? > >And what > >is the advantage of not encrypting the filenames anyway? It is better to > >encrypt by default. > > > >Eric > > (sorry for the delay in reply due to my vacation). > > It all depends on the use case, Android is one such use case. Some > data center use a known set of application. Again it all depends on > the use case. > > File-name is a kind of File-system semantic and altering based on > the on the user key context does not guarantee the system will be > compatible with all their legacy applications. > You really need to give more detail about why filename encryption specifically is a problem. What "legacy applications" is a problem for, and why? What do you mean by a "data center" use case? Isn't contents encryption a "problem" for some "legacy applications" as well? Contents encryption changes filesystem semantics as well. > Also a section on backup and restore in this doc will be a good > idea. As I think that will be affected IMO. And needing to have the > user master key to restore encrypted file isn't practical in some > data center multi tenanted solutions. Albeit it may work in some > cases but hard to generalize. > The proposed documentation already mentions that there is no way to backup the raw ciphertext currently. I'll try to make this a bit clearer, but in any case I am documenting the current state of things; the new documentation file is *not* a plan for things that don't exist yet. If/when someone proposes a patchset which adds a backup+restore API they can propose a documentation update along with it. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fscrypt: add a documentation file for filesystem-level encryption
Hi Anand, On Tue, Aug 29, 2017 at 11:54:47AM +0800, Anand Jain wrote: > > BTRFS has an experimental fscrypt implementation[1] which does not > include the file-name encryption part it should be included but as > an optional since not all uses cases saves sensitive information in > the file-name. OR even if the attacker is able to identify a file > called secrete.txt and break it then its still points at the > weakness of the file-data encryption. Can we say that ? apparently > from the discussion here it seems the answer is yes. > Filenames by themselves can be sensitive information, since a filename can serve as a strong indicator of what a file is. For example, a filename could be used as evidence that a person had access to a certain document. > >Hence, making the encryption of the filenames optional doesn't just to > >make life easier for backup/restore isn't a compelling argument, since > >the backup/restore program is going to have to have special case > >handling for fscrypt protected file systems *anyway*. > > fscrypt backup and restore does not work even without file-name > encryption because the Extended Attribute needs special ioctl in the > fscrypt (I did rise this objection before). > > But its entirely possible to create a string based encryption > metadata which can be updated/retrieved using the legacy backup > tools such as > > rsync --xattrs > > That will be a design for fscryptv2 probably.. > > OR I mean to say possible optional file-name encryption is not the > ground reason for the encrypted backup and restore challenge. > This was already discussed. You cannot simply add and remove the encryption xattr from random files and directories; what happens to all the open file descriptors, memory maps, pagecache pages, dcache entries, etc.? Are the existing contents and directory entries supposed to be left unencrypted, or are they supposed to be already encrypted, or is the filesystem supposed to automatically encrypt them when the xattr is set? What about when the xattr is removed? Again, you're of course free to propose a design which takes into account all of this and makes the argument for adding an xattr-based API, but I haven't seen it yet. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] fscrypt: add a documentation file for filesystem-level encryption
On Mon, Aug 28, 2017 at 08:18:55PM +0800, Anand Jain wrote: > > > On 08/23/2017 01:36 AM, Eric Biggers wrote: > >On Tue, Aug 22, 2017 at 11:35:20PM +0800, Anand Jain wrote: > >>>> > >>>> I think AE is the only good solution for this, File-name encryption at > >>>>this stage won't solve any kind of Evil Maid attack, (as it was quoted > >>>>somewhere else in ML). > >>>> > >>>> > >>>> Further, below, is define but not used. > >>>>- > >>>> #define FS_AES_256_GCM_KEY_SIZE 32 > >>>>- > >>>> > >>> > >>>Yes, authenticated encryption with AES-256-GCM was in an older version of > >>>the > >>>ext4 encryption design document. But unfortunately it was never really > >>>thought > >>>through. The primary problem, even ignoring rollback protection, is that > >>>there > >>>is nowhere to store the per-block metadata (GCM authentication tag and IV) > >>>*and* > >>>have it updated atomicly with the block contents. Recently, dm-integrity > >>>solves > >>>this at the block device layer, but it uses data journaling which is very > >>>inefficient. This maybe could be implemented more efficiently on a COW > >>>filesystem like BTRFS. But even after that, another problem is that > >>>authenticated encryption of file contents only would not stop an attacker > >>>from > >>>swapping around blocks, files, directories, or creating links, etc. > >> > >> > >> Some of the problems to be solved in this area are quite > >>interesting and challenging and IMO BTRFS fits nicely. Per extent AE > >>for BTRFS is drafted, it needs scrutiny and constructive feedback. > >> > >>Thanks, Anand > >> > >> > >>>Eric > >>> > > > >Where is the code? Is there a design document, and it is it readable by > >people > >not as familiar with btrfs? Is the API compatible with ext4, f2fs, and > >ubifs? > > > >Eric > > (sorry for the delay in replay due to my vacation). > > Eric, No code yet, proposed encryption method is seeking review. > Link sent to you. > > Thanks, Anand Thanks, I'll review it when I have time. Can you please consider sending out a public link to linux-fscrypt, linux-fsdevel, linux-btrfs, etc. so that other people can review it too? Eric -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] fscrypt: add a documentation file for filesystem-level encryption
From: Eric Biggers Perhaps long overdue, add a documentation file for filesystem-level encryption, a.k.a. fscrypt or fs/crypto/, to the Documentation directory. The new file is based loosely on the latest version of the "EXT4 Encryption Design Document (public version)" Google Doc, but with many improvements made, including: - Reflect the reality that it is not specific to ext4 anymore. - More thoroughly document the design and user-visible API/behavior. - Replace outdated information, such as the outdated explanation of how encrypted filenames are hashed for indexed directories and how encrypted filenames are presented to userspace without the key. (This was changed just before release.) For now the focus is on the design and user-visible API/behavior, not on how to add encryption support to a filesystem --- since the internal API is still pretty messy and any standalone documentation for it would become outdated as things get refactored over time. Signed-off-by: Eric Biggers --- Changes since v1: - Mention that using existing userspace tools is preferred - Don't start an argument about the best way to get random numbers - Make it clear that backup/restore of encrypted files without key is not supported yet - Mention a reason why the encryption xattr should not be exposed via the xattr system calls Documentation/filesystems/fscrypt.rst | 597 ++ Documentation/filesystems/index.rst | 11 + MAINTAINERS | 1 + 3 files changed, 609 insertions(+) create mode 100644 Documentation/filesystems/fscrypt.rst diff --git a/Documentation/filesystems/fscrypt.rst b/Documentation/filesystems/fscrypt.rst new file mode 100644 index ..ec4cad049dde --- /dev/null +++ b/Documentation/filesystems/fscrypt.rst @@ -0,0 +1,597 @@ += +Filesystem-level encryption (fscrypt) += + +Introduction + + +fscrypt is a library which filesystems can hook into to support +transparent encryption of files and directories. + +Note: "fscrypt" in this document refers to the kernel-level portion, +implemented in ``fs/crypto/``, as opposed to the userspace tool +`fscrypt <https://github.com/google/fscrypt>`_. This document only +covers the kernel-level portion. For command-line examples of how to +use encryption, see the documentation for the userspace tool `fscrypt +<https://github.com/google/fscrypt>`_. Also, it is strongly +recommended to use the fscrypt userspace tool, or other existing +userspace tools such as Android's key management system, over using +the kernel's API directly. Using existing tools reduces the chance of +introducing your own security bugs. (Nevertheless, for completeness +this documentation covers the kernel's API anyway.) + +Unlike dm-crypt, fscrypt operates at the filesystem level rather than +at the block device level. This allows it to encrypt different files +with different keys and to have unencrypted files on the same +filesystem. This is useful for multi-user systems where each user's +data-at-rest needs to be cryptographically isolated from the others. +However, except for filenames, fscrypt does not encrypt filesystem +metadata. + +Unlike eCryptfs, which is a stacked filesystem, fscrypt is integrated +directly into supported filesystems --- currently ext4, F2FS, and +UBIFS. This allows encrypted files to be read and written without +caching both the decrypted and encrypted pages in the pagecache, +thereby halving the memory used and bringing it in line with +unencrypted files. Similarly, half as many dentries and inodes are +needed. eCryptfs also limits filenames to 143 bytes, causing +application compatibility issues; fscrypt allows the full 255 bytes +(NAME_MAX). Finally, unlike eCryptfs, the fscrypt API can be used by +unprivileged users, with no need to mount anything. + +fscrypt does not support encrypting files in-place. Instead, it +supports marking an empty directory as encrypted. Then, after +userspace provides the key, all regular files, directories, and +symbolic links created in that directory tree are transparently +encrypted. + +Threat model + + +Offline attacks +--- + +Provided that userspace chooses a strong encryption key, fscrypt +protects the confidentiality of file contents and filenames in the +event of a single point-in-time permanent offline compromise of the +block device content. fscrypt does not protect the confidentiality of +non-filename metadata, e.g. file sizes, file permissions, file +timestamps, and extended attributes. Also, the existence and location +of holes (unallocated blocks which logically contain all zeroes) in +files is not protected. + +fscrypt is not guaranteed to protect confidentiality or authenticity +if an attacker is able to manipulate the filesystem offline prior to +an authoriz
Re: [PATCH v2] fscrypt: add a documentation file for filesystem-level encryption
Hi Michael, On Fri, Sep 01, 2017 at 05:12:28PM -0700, Michael Halcrow wrote: > > +fscrypt is only resistant to side-channel attacks, such as timing or > > +electromagnetic attacks, to the extent that the underlying Linux > > +Cryptographic API algorithms are. If a vulnerable algorithm is used, > > +such as a table-based implementation of AES, it may be possible for an > > +attacker to mount a side channel attack against the online system. > > Conservatively, I'd go a step further and say that even if the kernel > crypto API were to mitigate all side channel attacks, we don't make > any claims as to whether encryption key material (or data for that > matter) would be vulnerable from code in fs/crypto or the individual > filesystems. If possible, I'd prefer to make a stronger claim, at least for the key material. We are very careful with how we manage the keys in the kernel, and I'd consider any timing attack against the keys to be a bug --- and I expect that other people would too. Data is a different story though, since ultimately it is used by applications to actually do something. > > +After an encryption key has been provided, fscrypt is not designed to > > +hide the plaintext file contents or filenames from other users on the > > +same system, regardless of the visibility of the keyring key. > > +Instead, existing access control mechanisms such as file mode bits, > > +POSIX ACLs, or SELinux should be used for this purpose. Also note > > Suggest replacing "SELinux" with "LSMs." Also do you think we should > mention namespaces? We could mention that a filesystem containing encrypted files can be "hidden" by unmounting it, though that should be obvious. It's also possible to hide individual files or directories by bind-mounting stuff over them, though that is getting somewhat eccentric; usually mode bits, ACLs, etc. would be used instead. > > +that as long as the encryption keys are *anywhere* in memory, an > > +online attacker can necessarily compromise them by mounting a physical > > +attack or by exploiting any kernel security vulnerability which > > +provides an arbitrary memory read primitive. > > Or hooking into a FireWire port and doing DMA? That falls under the category of "physical attack". > > +Currently, fscrypt does not prevent a user from maliciously providing > > +an incorrect key for another user's existing encrypted files. A > > +protection against this is planned. > > How's the review for that patch coming along? So far you're the only person who has reviewed the patchset in detail. I've tried to get more people interested, but it has been difficult. Perhaps the new documentation will help people understand the problems that the patchset solves. Being able to provide the wrong key for another user's encrypted files is a security vulnerability, so we *have* to fix it eventually. > > +- Per-file keys strengthen the encryption of filenames, where IVs are > > + reused out of necessity. With a unique key per directory, IV reuse > > + is limited to within a single directory. > > Side note: Alex's HEH patchset addresses this. Is there interest in > the community for us to push for a merge at this point? > > https://www.spinics.net/lists/linux-crypto/msg22411.html Well, I think there are higher priorities right now, but if we want to start that conversation we'd first need to send the updated version of the patchset which implements the latest version of the algorithm (https://tools.ietf.org/html/draft-cope-heh-01) and has been rebased onto the latest kernel. > > +Note: this KDF meets the primary security requirement, which is to > > +produce unique derived keys that preserve the entropy of the master > > +key, assuming that the master key is already a good pseudorandom key. > > +However, it is nonstandard and has some theoretical problems such as > > +being reversible, so it is generally considered to be a mistake! It > > Being reversible isn't theoretical -- it's trivially reversible *if* > you can exploit kernel memory. The attack I'm concerned about is that > an adversary is somehow able to get to an inode key but isn't able to > get to the master key. Exploiting kernel memory wasn't in my > adversarial model at the time, and when I suggested going to something > like HKDF, Ted convinced me that it would be too painful since he had > already merged the code. > > Regardless, I'm happy to call it a mistake. > > > +may be replaced with HKDF or another more standard KDF in the future. > > Again, I should point out that your patchset that includes migrating > to HKDF is still pending merge. I'd like to see that go in soon. It is too late for v4.14 since that merge window is already open. It could potentially go into v4.15, though as a prerequisite for that I expect that Ted would like to see more people review/discuss it first. Note that we do not want to rush it, since we will be locked into the new on-disk format once merged. - Eric -- To unsubscribe from
[PATCH v3] fscrypt: add a documentation file for filesystem-level encryption
From: Eric Biggers Perhaps long overdue, add a documentation file for filesystem-level encryption, a.k.a. fscrypt or fs/crypto/, to the Documentation directory. The new file is based loosely on the latest version of the "EXT4 Encryption Design Document (public version)" Google Doc, but with many improvements made, including: - Reflect the reality that it is not specific to ext4 anymore. - More thoroughly document the design and user-visible API/behavior. - Replace outdated information, such as the outdated explanation of how encrypted filenames are hashed for indexed directories and how encrypted filenames are presented to userspace without the key. (This was changed just before release.) For now the focus is on the design and user-visible API/behavior, not on how to add encryption support to a filesystem --- since the internal API is still pretty messy and any standalone documentation for it would become outdated as things get refactored over time. Reviewed-by: Michael Halcrow Signed-off-by: Eric Biggers --- Changes since v2: - A few tweaks to address comments from Michael Halcrow and Joe Richey. Changes since v1: - Mention that using existing userspace tools is preferred - Don't start an argument about the best way to get random numbers - Make it clear that backup/restore of encrypted files without key is not supported yet - Mention a reason why the encryption xattr should not be exposed via the xattr system calls Documentation/filesystems/fscrypt.rst | 610 ++ Documentation/filesystems/index.rst | 11 + MAINTAINERS | 1 + 3 files changed, 622 insertions(+) create mode 100644 Documentation/filesystems/fscrypt.rst diff --git a/Documentation/filesystems/fscrypt.rst b/Documentation/filesystems/fscrypt.rst new file mode 100644 index ..776ddc655f79 --- /dev/null +++ b/Documentation/filesystems/fscrypt.rst @@ -0,0 +1,610 @@ += +Filesystem-level encryption (fscrypt) += + +Introduction + + +fscrypt is a library which filesystems can hook into to support +transparent encryption of files and directories. + +Note: "fscrypt" in this document refers to the kernel-level portion, +implemented in ``fs/crypto/``, as opposed to the userspace tool +`fscrypt <https://github.com/google/fscrypt>`_. This document only +covers the kernel-level portion. For command-line examples of how to +use encryption, see the documentation for the userspace tool `fscrypt +<https://github.com/google/fscrypt>`_. Also, it is recommended to use +the fscrypt userspace tool, or other existing userspace tools such as +`fscryptctl <https://github.com/google/fscryptctl>`_ or `Android's key +management system +<https://source.android.com/security/encryption/file-based>`_, over +using the kernel's API directly. Using existing tools reduces the +chance of introducing your own security bugs. (Nevertheless, for +completeness this documentation covers the kernel's API anyway.) + +Unlike dm-crypt, fscrypt operates at the filesystem level rather than +at the block device level. This allows it to encrypt different files +with different keys and to have unencrypted files on the same +filesystem. This is useful for multi-user systems where each user's +data-at-rest needs to be cryptographically isolated from the others. +However, except for filenames, fscrypt does not encrypt filesystem +metadata. + +Unlike eCryptfs, which is a stacked filesystem, fscrypt is integrated +directly into supported filesystems --- currently ext4, F2FS, and +UBIFS. This allows encrypted files to be read and written without +caching both the decrypted and encrypted pages in the pagecache, +thereby nearly halving the memory used and bringing it in line with +unencrypted files. Similarly, half as many dentries and inodes are +needed. eCryptfs also limits encrypted filenames to 143 bytes, +causing application compatibility issues; fscrypt allows the full 255 +bytes (NAME_MAX). Finally, unlike eCryptfs, the fscrypt API can be +used by unprivileged users, with no need to mount anything. + +fscrypt does not support encrypting files in-place. Instead, it +supports marking an empty directory as encrypted. Then, after +userspace provides the key, all regular files, directories, and +symbolic links created in that directory tree are transparently +encrypted. + +Threat model + + +Offline attacks +--- + +Provided that userspace chooses a strong encryption key, fscrypt +protects the confidentiality of file contents and filenames in the +event of a single point-in-time permanent offline compromise of the +block device content. fscrypt does not protect the confidentiality of +non-filename metadata, e.g. file sizes, file permissions, file +timestamps, and extended attributes. Also, the existence and location
Re: [PATCH v3] fscrypt: add a documentation file for filesystem-level encryption
On Fri, Sep 08, 2017 at 05:15:12PM -0700, Eric Biggers wrote: > From: Eric Biggers > > Perhaps long overdue, add a documentation file for filesystem-level > encryption, a.k.a. fscrypt or fs/crypto/, to the Documentation > directory. The new file is based loosely on the latest version of the > "EXT4 Encryption Design Document (public version)" Google Doc, but with > many improvements made, including: > > - Reflect the reality that it is not specific to ext4 anymore. > - More thoroughly document the design and user-visible API/behavior. > - Replace outdated information, such as the outdated explanation of how > encrypted filenames are hashed for indexed directories and how > encrypted filenames are presented to userspace without the key. > (This was changed just before release.) > > For now the focus is on the design and user-visible API/behavior, not on > how to add encryption support to a filesystem --- since the internal API > is still pretty messy and any standalone documentation for it would > become outdated as things get refactored over time. > > Reviewed-by: Michael Halcrow Ted, are you interested in taking this through the fscrypt tree for v4.15? Eric -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] KEYS: fix in-kernel documentation for keyctl_read()
From: Eric Biggers When keyctl_read() is passed a buffer that is too small, the behavior is inconsistent. Some key types will fill as much of the buffer as possible, while others won't copy anything. Moreover, the in-kernel documentation contradicted the man page on this point. Update the in-kernel documentation to say that this point is unspecified. Signed-off-by: Eric Biggers --- Documentation/security/keys/core.rst | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/Documentation/security/keys/core.rst b/Documentation/security/keys/core.rst index 1266eeae45f6..16f196069721 100644 --- a/Documentation/security/keys/core.rst +++ b/Documentation/security/keys/core.rst @@ -628,12 +628,12 @@ The keyctl syscall functions are: defined key type will return its data as is. If a key type does not implement this function, error EOPNOTSUPP will result. - As much of the data as can be fitted into the buffer will be copied to - userspace if the buffer pointer is not NULL. - - On a successful return, the function will always return the amount of data - available rather than the amount copied. + On success, the function will return the amount of data placed into the + buffer. + If the specified buffer is too small, then the size of the buffer required + will be returned, and it is unspecified whether any data will be copied + into the buffer. * Instantiate a partially constructed key:: -- 2.15.0.rc2.357.g7e34df9404-goog -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] KEYS: fix in-kernel documentation for keyctl_read()
On Wed, Nov 01, 2017 at 01:57:18PM +, David Howells wrote: > Eric Biggers wrote: > > > - As much of the data as can be fitted into the buffer will be copied to > > - userspace if the buffer pointer is not NULL. > > - > > - On a successful return, the function will always return the amount of > > data > > - available rather than the amount copied. > > + On success, the function will return the amount of data placed into > > the > > + buffer. > > > > + If the specified buffer is too small, then the size of the buffer > > required > > + will be returned, and it is unspecified whether any data will be > > copied > > + into the buffer. > > How about: > > If the specified buffer is too small, then the size of the buffer > required will be returned. Note that, in this case, the contents of the > buffer may be have been overwritten in some undefined way. > > Otherwise, on success, the function will return the amount of data copied > into the buffer. > > David I guess that's fine --- either way users can't rely on the contents of the buffer. The man page should use the same wording, though. Should I send a v2 of both patches? Eric -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] KEYS: fix in-kernel documentation for keyctl_read()
From: Eric Biggers When keyctl_read() is passed a buffer that is too small, the behavior is inconsistent. Some key types will fill as much of the buffer as possible, while others won't copy anything. Moreover, the in-kernel documentation contradicted the man page on this point. Update the in-kernel documentation to say that this point is unspecified. Signed-off-by: Eric Biggers --- Documentation/security/keys/core.rst | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/Documentation/security/keys/core.rst b/Documentation/security/keys/core.rst index 1266eeae45f6..9ce7256c6edb 100644 --- a/Documentation/security/keys/core.rst +++ b/Documentation/security/keys/core.rst @@ -628,12 +628,12 @@ The keyctl syscall functions are: defined key type will return its data as is. If a key type does not implement this function, error EOPNOTSUPP will result. - As much of the data as can be fitted into the buffer will be copied to - userspace if the buffer pointer is not NULL. - - On a successful return, the function will always return the amount of data - available rather than the amount copied. + If the specified buffer is too small, then the size of the buffer required + will be returned. Note that in this case, the contents of the buffer may + have been overwritten in some undefined way. + Otherwise, on success, the function will return the amount of data copied + into the buffer. * Instantiate a partially constructed key:: -- 2.15.0.403.gc27cc4dac6-goog -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[keyutils PATCH v2] man: keyctl_read(3): fix documentation for short buffer case
From: Eric Biggers When keyctl_read() is passed a buffer that is too small, the behavior is inconsistent. Some key types will fill as much of the buffer as possible, while others won't copy anything. Moreover, the in-kernel documentation contradicted the man page on this point. Update the man page to say that this point is unspecified. Signed-off-by: Eric Biggers --- man/keyctl_read.3 | 8 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/man/keyctl_read.3 b/man/keyctl_read.3 index 25821ad..852bc05 100644 --- a/man/keyctl_read.3 +++ b/man/keyctl_read.3 @@ -33,8 +33,8 @@ permission on a key to be able to read it. and .I buflen specify the buffer into which the payload data will be placed. If the buffer -is too small, the full size of the payload will be returned and no copy will -take place. +is too small, then the full size of the payload will be returned, and the +contents of the buffer may be overwritten in some undefined way. .P .BR keyctl_read_alloc () is similar to @@ -62,8 +62,8 @@ though the byte-ordering is as appropriate for the kernel. On success .BR keyctl_read () returns the amount of data placed into the buffer. If the buffer was too -small, then the size of buffer required will be returned, but no data will be -transferred. +small, then the size of buffer required will be returned, and the contents of +the buffer may have been overwritten in some undefined way. .P On success .BR keyctl_read_alloc () -- 2.15.0.403.gc27cc4dac6-goog -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] encrypted-keys: document new fscrypt key format
Hi André, On Wed, Jan 10, 2018 at 12:44:18PM +, André Draszik wrote: > diff --git a/Documentation/security/keys/fscrypt.rst > b/Documentation/security/keys/fscrypt.rst > new file mode 100644 > index ..e4a29592513e > --- /dev/null > +++ b/Documentation/security/keys/fscrypt.rst > @@ -0,0 +1,67 @@ > + > +Encrypted keys for the fscrypt subsystem > + There is now documentation for fscrypt in Documentation/filesystems/fscrypt.rst; see in particular the "Adding keys" section. The documentation for any new ways to add keys should go in there. > + > +fscrypt allows file systems to implement transparent encryption and > decryption > +of files, similar to eCryptfs, using keys derived from a master key > descriptor. Note that the master key *descriptor* refers to the hex string used in the keyring key description. It is not the same as the master key itself, which is stored in the payload. The cryptography is done with the master key, not with the master key *descriptor*. > +In order to avoid known-plaintext attacks, the datablob obtained through > +commands 'keyctl print' or 'keyctl pipe' does not contain the overall > +fscrypt_key, the contents of which is well known, but only the master key > +descriptor itself in encrypted form. > + > +The fscrypt subsystem may really benefit from using encrypted keys in that > the > +required key can be securely generated by an Administrator and provided at > boot > +time after the unsealing of a 'trusted' key in order to perform the mount in > a > +controlled environment. Another advantage is that the key is not exposed to > +threats of malicious software, because it is available in clear form only at > +kernel level. Please be very clear about exactly what security properties are achieved by using encrypted-keys. Note that such keys are present in the clear in kernel memory, so they will be exposed by any exploit that compromises the kernel, or even just finds a way to read its memory. (And if you've been paying attention in the last week, you may be aware that certain CPU vendors have "helpfully" made reading kernel memory quite easy.) So, it's definitely *not* categorically true that "the key is not exposed to threats of malicious software". Also note that fscrypt is already using the "logon" key type which cannot be read by userspace (without exploits). This is different from eCryptfs which uses the "user" key type. > +Usage:: > + > + keyctl add encrypted fscrypt:policy "new fscrypt key-type:master-key-name > keylen" ring > + keyctl add encrypted fscrypt:policy "load hex_blob" ring > + keyctl update keyid "update key-type:master-key-name" > + > +Where:: > + > + policy:= '<16 hexadecimal characters>' > + key-type:= 'trusted' | 'user' > + keylen:= 16 | 32 | 64 > + > + > +Example of encrypted key usage with the fscrypt subsystem: > + > +Create an encrypted key "1234567890123456" of length 64 bytes with format > +'fscrypt' and save it using a previously loaded user key "test":: > + > +$ keyctl add encrypted fscrypt:1234567890123456 "new fscrypt user:test > 64" @u > +1023935199 > + > +$ keyctl print 1023935199 > +fscrypt user:test 64 e5606689fdc25d78a787249f4069fb3b007e992f4b21d0eda60 > +c97986fc2e3326b5542e2b32216fc5007d9fd19cd3cb6668fa9850e954d2ba25e1b8a331 > +1b0c1f20666c > + > +$ keyctl pipe 1023935199 > fscrypt.blob What is the point of having the kernel wrap a key with the "user" key type? It seems pointless; userspace could just do it instead. Note that if you use keyctl_read() to read from an encrypted-key, you can actually request that the key be encrypted using an arbitrary key of the type with which the key is supposed to be wrapped. This can be done by adding a key to your thread/process/session keyring whose type and description matches the intended wrapping key. Thus, any "encrypted" key wrapped with a "user" key can effectively be retrieved in the clear by calling keyctl_read(), then decrypting the ciphertext in userspace. Perhaps that's actually a bug; I don't know. But using "user" wrapping keys seems pretty pointless anyway. I think it's really only "trusted" wrapping keys where this new feature would have any useful security properties. So the documentation needs to explain that, and use that in the examples. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] crypto: document correct return value for request allocation
Signed-off-by: Eric Biggers
---
Documentation/DocBook/crypto-API.tmpl | 6 +++---
include/crypto/aead.h | 3 +--
include/crypto/hash.h | 3 +--
include/crypto/skcipher.h | 3 +--
include/linux/crypto.h| 3 +--
5 files changed, 7 insertions(+), 11 deletions(-)
diff --git a/Documentation/DocBook/crypto-API.tmpl
b/Documentation/DocBook/crypto-API.tmpl
index 348619f..d55dc5a 100644
--- a/Documentation/DocBook/crypto-API.tmpl
+++ b/Documentation/DocBook/crypto-API.tmpl
@@ -1936,9 +1936,9 @@ static int test_skcipher(void)
}
req = skcipher_request_alloc(skcipher, GFP_KERNEL);
- if (IS_ERR(req)) {
- pr_info("could not allocate request queue\n");
- ret = PTR_ERR(req);
+ if (!req) {
+ pr_info("could not allocate skcipher request\n");
+ ret = -ENOMEM;
goto out;
}
diff --git a/include/crypto/aead.h b/include/crypto/aead.h
index 957bb87..75174f8 100644
--- a/include/crypto/aead.h
+++ b/include/crypto/aead.h
@@ -405,8 +405,7 @@ static inline void aead_request_set_tfm(struct aead_request
*req,
* encrypt and decrypt API calls. During the allocation, the provided aead
* handle is registered in the request data structure.
*
- * Return: allocated request handle in case of success; IS_ERR() is true in
case
- *of an error, PTR_ERR() returns the error code.
+ * Return: allocated request handle in case of success, or NULL if out of
memory
*/
static inline struct aead_request *aead_request_alloc(struct crypto_aead *tfm,
gfp_t gfp)
diff --git a/include/crypto/hash.h b/include/crypto/hash.h
index 1969f14..2660588 100644
--- a/include/crypto/hash.h
+++ b/include/crypto/hash.h
@@ -547,8 +547,7 @@ static inline void ahash_request_set_tfm(struct
ahash_request *req,
* the allocation, the provided ahash handle
* is registered in the request data structure.
*
- * Return: allocated request handle in case of success; IS_ERR() is true in
case
- *of an error, PTR_ERR() returns the error code.
+ * Return: allocated request handle in case of success, or NULL if out of
memory
*/
static inline struct ahash_request *ahash_request_alloc(
struct crypto_ahash *tfm, gfp_t gfp)
diff --git a/include/crypto/skcipher.h b/include/crypto/skcipher.h
index 905490c..0f987f5 100644
--- a/include/crypto/skcipher.h
+++ b/include/crypto/skcipher.h
@@ -425,8 +425,7 @@ static inline struct skcipher_request
*skcipher_request_cast(
* encrypt and decrypt API calls. During the allocation, the provided skcipher
* handle is registered in the request data structure.
*
- * Return: allocated request handle in case of success; IS_ERR() is true in
case
- *of an error, PTR_ERR() returns the error code.
+ * Return: allocated request handle in case of success, or NULL if out of
memory
*/
static inline struct skcipher_request *skcipher_request_alloc(
struct crypto_skcipher *tfm, gfp_t gfp)
diff --git a/include/linux/crypto.h b/include/linux/crypto.h
index 99c9489..6e28c89 100644
--- a/include/linux/crypto.h
+++ b/include/linux/crypto.h
@@ -948,8 +948,7 @@ static inline struct ablkcipher_request
*ablkcipher_request_cast(
* encrypt and decrypt API calls. During the allocation, the provided
ablkcipher
* handle is registered in the request data structure.
*
- * Return: allocated request handle in case of success; IS_ERR() is true in
case
- *of an error, PTR_ERR() returns the error code.
+ * Return: allocated request handle in case of success, or NULL if out of
memory
*/
static inline struct ablkcipher_request *ablkcipher_request_alloc(
struct crypto_ablkcipher *tfm, gfp_t gfp)
--
2.7.4
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/4] crypto: drbg wait for crypto op not signal safe
Hi Gilad,
On Thu, May 11, 2017 at 02:53:43PM +0300, Gilad Ben-Yossef wrote:
> drbg_kcapi_sym_ctr() was using wait_for_completion_interruptible() to
> wait for completion of async crypto op but if a signal occurs it
> may return before DMA ops of HW crypto provider finish, thus
> corrupting the output buffer.
>
> Resolve this by using wait_for_completion() instead.
>
> Reported-by: Eric Biggers
> Signed-off-by: Gilad Ben-Yossef
> CC: sta...@vger.kernel.org
> ---
> crypto/drbg.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/crypto/drbg.c b/crypto/drbg.c
> index fa749f4..fa9054d 100644
> --- a/crypto/drbg.c
> +++ b/crypto/drbg.c
> @@ -1767,8 +1767,7 @@ static int drbg_kcapi_sym_ctr(struct drbg_state *drbg,
> break;
> case -EINPROGRESS:
> case -EBUSY:
> - ret = wait_for_completion_interruptible(
> - &drbg->ctr_completion);
> + ret = wait_for_completion(&drbg->ctr_completion);
> if (!ret && !drbg->ctr_async_err) {
> reinit_completion(&drbg->ctr_completion);
> break;
> --
wait_for_completion() doesn't return a value. This was fixed in the next patch,
but it should be done in this patch.
Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] crypto: Documentation: fix none signal safe sample
On Thu, May 11, 2017 at 02:53:45PM +0300, Gilad Ben-Yossef wrote:
> The sample code was showing use of wait_for_completion_interruptible()
> for waiting for an async. crypto op to finish. However, if a signal
> arrived it would free the buffers used even while crypto HW might
> still DMA from/into the buffers.
>
> Resolve this by using wait_for_completion() instead.
>
> Reported-by: Eric Biggers
> Signed-off-by: Gilad Ben-Yossef
> ---
> Documentation/crypto/api-samples.rst | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/crypto/api-samples.rst
> b/Documentation/crypto/api-samples.rst
> index d021fd9..944f08b 100644
> --- a/Documentation/crypto/api-samples.rst
> +++ b/Documentation/crypto/api-samples.rst
> @@ -48,7 +48,7 @@ Code Example For Symmetric Key Cipher Operation
> break;
> case -EINPROGRESS:
> case -EBUSY:
> -rc = wait_for_completion_interruptible(
> +rc = wait_for_completion(
> &sk->result.completion);
> if (!rc && !sk->result.err) {
> reinit_completion(&sk->result.completion);
> --
> 2.1.4
>
Same issue here: wait_for_completion() doesn't return a value.
Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [keyutils PATCH v2] man: keyctl_read(3): fix documentation for short buffer case
On Thu, Nov 02, 2017 at 11:06:05AM -0700, Eric Biggers wrote: > From: Eric Biggers > > When keyctl_read() is passed a buffer that is too small, the behavior is > inconsistent. Some key types will fill as much of the buffer as > possible, while others won't copy anything. Moreover, the in-kernel > documentation contradicted the man page on this point. > > Update the man page to say that this point is unspecified. > > Signed-off-by: Eric Biggers > --- > man/keyctl_read.3 | 8 > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/man/keyctl_read.3 b/man/keyctl_read.3 > index 25821ad..852bc05 100644 > --- a/man/keyctl_read.3 > +++ b/man/keyctl_read.3 > @@ -33,8 +33,8 @@ permission on a key to be able to read it. > and > .I buflen > specify the buffer into which the payload data will be placed. If the buffer > -is too small, the full size of the payload will be returned and no copy will > -take place. > +is too small, then the full size of the payload will be returned, and the > +contents of the buffer may be overwritten in some undefined way. > .P > .BR keyctl_read_alloc () > is similar to > @@ -62,8 +62,8 @@ though the byte-ordering is as appropriate for the kernel. > On success > .BR keyctl_read () > returns the amount of data placed into the buffer. If the buffer was too > -small, then the size of buffer required will be returned, but no data will be > -transferred. > +small, then the size of buffer required will be returned, and the contents of > +the buffer may have been overwritten in some undefined way. > .P > On success > .BR keyctl_read_alloc () > -- > 2.15.0.403.gc27cc4dac6-goog > Ping. -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [keyutils PATCH v2] man: keyctl_read(3): fix documentation for short buffer case
On Tue, Feb 20, 2018 at 11:42:34AM -0800, Eric Biggers wrote: > On Thu, Nov 02, 2017 at 11:06:05AM -0700, Eric Biggers wrote: > > From: Eric Biggers > > > > When keyctl_read() is passed a buffer that is too small, the behavior is > > inconsistent. Some key types will fill as much of the buffer as > > possible, while others won't copy anything. Moreover, the in-kernel > > documentation contradicted the man page on this point. > > > > Update the man page to say that this point is unspecified. > > > > Signed-off-by: Eric Biggers > > --- > > man/keyctl_read.3 | 8 > > 1 file changed, 4 insertions(+), 4 deletions(-) > > > > diff --git a/man/keyctl_read.3 b/man/keyctl_read.3 > > index 25821ad..852bc05 100644 > > --- a/man/keyctl_read.3 > > +++ b/man/keyctl_read.3 > > @@ -33,8 +33,8 @@ permission on a key to be able to read it. > > and > > .I buflen > > specify the buffer into which the payload data will be placed. If the > > buffer > > -is too small, the full size of the payload will be returned and no copy > > will > > -take place. > > +is too small, then the full size of the payload will be returned, and the > > +contents of the buffer may be overwritten in some undefined way. > > .P > > .BR keyctl_read_alloc () > > is similar to > > @@ -62,8 +62,8 @@ though the byte-ordering is as appropriate for the kernel. > > On success > > .BR keyctl_read () > > returns the amount of data placed into the buffer. If the buffer was too > > -small, then the size of buffer required will be returned, but no data will > > be > > -transferred. > > +small, then the size of buffer required will be returned, and the contents > > of > > +the buffer may have been overwritten in some undefined way. > > .P > > On success > > .BR keyctl_read_alloc () > > -- > > 2.15.0.403.gc27cc4dac6-goog > > > > Ping. Ping. -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] crypto: doc - fix documentation for bulk registration functions
Update the documentation for crypto_register_algs() and crypto_unregister_algs() to match the actual behavior. Signed-off-by: Eric Biggers --- Documentation/DocBook/crypto-API.tmpl | 38 --- 1 file changed, 26 insertions(+), 12 deletions(-) diff --git a/Documentation/DocBook/crypto-API.tmpl b/Documentation/DocBook/crypto-API.tmpl index fb2a152..088b79c 100644 --- a/Documentation/DocBook/crypto-API.tmpl +++ b/Documentation/DocBook/crypto-API.tmpl @@ -797,7 +797,8 @@ kernel crypto API| Caller include/linux/crypto.h and their definition can be seen below. The former function registers a single transformation, while the latter works on an array of transformation descriptions. - The latter is useful when registering transformations in bulk. + The latter is useful when registering transformations in bulk, + for example when a driver implements multiple transformations. @@ -822,18 +823,31 @@ kernel crypto API| Caller - The bulk registration / unregistration functions require - that struct crypto_alg is an array of count size. These - functions simply loop over that array and register / - unregister each individual algorithm. If an error occurs, - the loop is terminated at the offending algorithm definition. - That means, the algorithms prior to the offending algorithm - are successfully registered. Note, the caller has no way of - knowing which cipher implementations have successfully - registered. If this is important to know, the caller should - loop through the different implementations using the single - instance *_alg functions for each individual implementation. + The bulk registration/unregistration functions + register/unregister each transformation in the given array of + length count. They handle errors as follows: + + + + crypto_register_algs() succeeds if and only if it + successfully registers all the given transformations. If an + error occurs partway through, then it rolls back successful + registrations before returning the error code. Note that if + a driver needs to handle registration errors for individual + transformations, then it will need to use the non-bulk + function crypto_register_alg() instead. + + + + + crypto_unregister_algs() tries to unregister all the given + transformations, continuing on error. It logs errors and + always returns zero. + + + + Single-Block Symmetric Ciphers [CIPHER] -- 2.8.0.rc3.226.g39d4020 -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH RESEND] crypto: doc - fix documentation for bulk registration functions
Update the documentation for crypto_register_algs() and crypto_unregister_algs() to match the actual behavior. Signed-off-by: Eric Biggers --- Documentation/DocBook/crypto-API.tmpl | 38 --- 1 file changed, 26 insertions(+), 12 deletions(-) diff --git a/Documentation/DocBook/crypto-API.tmpl b/Documentation/DocBook/crypto-API.tmpl index fb2a152..088b79c 100644 --- a/Documentation/DocBook/crypto-API.tmpl +++ b/Documentation/DocBook/crypto-API.tmpl @@ -797,7 +797,8 @@ kernel crypto API| Caller include/linux/crypto.h and their definition can be seen below. The former function registers a single transformation, while the latter works on an array of transformation descriptions. - The latter is useful when registering transformations in bulk. + The latter is useful when registering transformations in bulk, + for example when a driver implements multiple transformations. @@ -822,18 +823,31 @@ kernel crypto API| Caller - The bulk registration / unregistration functions require - that struct crypto_alg is an array of count size. These - functions simply loop over that array and register / - unregister each individual algorithm. If an error occurs, - the loop is terminated at the offending algorithm definition. - That means, the algorithms prior to the offending algorithm - are successfully registered. Note, the caller has no way of - knowing which cipher implementations have successfully - registered. If this is important to know, the caller should - loop through the different implementations using the single - instance *_alg functions for each individual implementation. + The bulk registration/unregistration functions + register/unregister each transformation in the given array of + length count. They handle errors as follows: + + + + crypto_register_algs() succeeds if and only if it + successfully registers all the given transformations. If an + error occurs partway through, then it rolls back successful + registrations before returning the error code. Note that if + a driver needs to handle registration errors for individual + transformations, then it will need to use the non-bulk + function crypto_register_alg() instead. + + + + + crypto_unregister_algs() tries to unregister all the given + transformations, continuing on error. It logs errors and + always returns zero. + + + + Single-Block Symmetric Ciphers [CIPHER] -- 2.8.0.rc3.226.g39d4020 -- To unsubscribe from this list: send the line "unsubscribe linux-doc" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH V2 1/7] ext4: use IS_ENCRYPTED() to check encryption status
On Tue, Dec 04, 2018 at 03:26:44PM +0530, Chandan Rajendra wrote:
> This commit removes the ext4 specific ext4_encrypted_inode() and makes
> use of the generic IS_ENCRYPTED() macro to check for the encryption
> status of an inode.
>
> Signed-off-by: Chandan Rajendra
Reviewed-by: Eric Biggers
Though if you send this out again, there are a few places where you can remove a
line break while remaining within 80 characters, due to IS_ENCRYPTED() being
shorter than ext4_encrypted_inode().
- Eric
> ---
> fs/ext4/dir.c | 8
> fs/ext4/ext4.h| 5 -
> fs/ext4/ext4_jbd2.h | 2 +-
> fs/ext4/extents.c | 4 ++--
> fs/ext4/ialloc.c | 2 +-
> fs/ext4/inode.c | 14 +++---
> fs/ext4/move_extent.c | 3 +--
> fs/ext4/namei.c | 8
> fs/ext4/page-io.c | 2 +-
> fs/ext4/readpage.c| 2 +-
> 10 files changed, 22 insertions(+), 28 deletions(-)
>
> diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c
> index f93f9881ec18..fb7a64ea5679 100644
> --- a/fs/ext4/dir.c
> +++ b/fs/ext4/dir.c
> @@ -111,7 +111,7 @@ static int ext4_readdir(struct file *file, struct
> dir_context *ctx)
> int dir_has_error = 0;
> struct fscrypt_str fstr = FSTR_INIT(NULL, 0);
>
> - if (ext4_encrypted_inode(inode)) {
> + if (IS_ENCRYPTED(inode)) {
> err = fscrypt_get_encryption_info(inode);
> if (err && err != -ENOKEY)
> return err;
> @@ -138,7 +138,7 @@ static int ext4_readdir(struct file *file, struct
> dir_context *ctx)
> return err;
> }
>
> - if (ext4_encrypted_inode(inode)) {
> + if (IS_ENCRYPTED(inode)) {
> err = fscrypt_fname_alloc_buffer(inode, EXT4_NAME_LEN, &fstr);
> if (err < 0)
> return err;
> @@ -245,7 +245,7 @@ static int ext4_readdir(struct file *file, struct
> dir_context *ctx)
> offset += ext4_rec_len_from_disk(de->rec_len,
> sb->s_blocksize);
> if (le32_to_cpu(de->inode)) {
> - if (!ext4_encrypted_inode(inode)) {
> + if (!IS_ENCRYPTED(inode)) {
> if (!dir_emit(ctx, de->name,
> de->name_len,
> le32_to_cpu(de->inode),
> @@ -613,7 +613,7 @@ static int ext4_dx_readdir(struct file *file, struct
> dir_context *ctx)
>
> static int ext4_dir_open(struct inode * inode, struct file * filp)
> {
> - if (ext4_encrypted_inode(inode))
> + if (IS_ENCRYPTED(inode))
> return fscrypt_get_encryption_info(inode) ? -EACCES : 0;
> return 0;
> }
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 80957f9d3cbe..2ae6ab88f218 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -2297,11 +2297,6 @@ extern unsigned ext4_free_clusters_after_init(struct
> super_block *sb,
> struct ext4_group_desc *gdp);
> ext4_fsblk_t ext4_inode_to_goal_block(struct inode *);
>
> -static inline bool ext4_encrypted_inode(struct inode *inode)
> -{
> - return ext4_test_inode_flag(inode, EXT4_INODE_ENCRYPT);
> -}
> -
> static inline bool ext4_verity_inode(struct inode *inode)
> {
> #ifdef CONFIG_EXT4_FS_VERITY
> diff --git a/fs/ext4/ext4_jbd2.h b/fs/ext4/ext4_jbd2.h
> index 15b6dd733780..a1ac7e9245ec 100644
> --- a/fs/ext4/ext4_jbd2.h
> +++ b/fs/ext4/ext4_jbd2.h
> @@ -411,7 +411,7 @@ static inline int ext4_inode_journal_mode(struct inode
> *inode)
> (ext4_test_inode_flag(inode, EXT4_INODE_JOURNAL_DATA) &&
> !test_opt(inode->i_sb, DELALLOC))) {
> /* We do not support data journalling for encrypted data */
> - if (S_ISREG(inode->i_mode) && ext4_encrypted_inode(inode))
> + if (S_ISREG(inode->i_mode) && IS_ENCRYPTED(inode))
> return EXT4_INODE_ORDERED_DATA_MODE; /* ordered */
> return EXT4_INODE_JOURNAL_DATA_MODE;/* journal data */
> }
> diff --git a/fs/ext4/extents.c b/fs/ext4/extents.c
> index 240b6dea5441..79d986dbf5af 100644
> --- a/fs/ext4/extents.c
> +++ b/fs/ext4/extents.c
> @@ -3631,7 +3631,7 @@ static int ext4_ext_convert_to_initialized(handle_t
> *handle,
> max_zeroout = sbi->s_extent_max_zeroout_kb >>
> (inode->i_sb->s_blocksize_bits - 10);
>
> - if (ext4_encrypted_inode(inode))
> + if (IS_ENCRYPTED(inode))
> ma
Re: [PATCH V2 2/7] f2fs: use IS_ENCRYPTED() to check encryption status
On Tue, Dec 04, 2018 at 03:26:45PM +0530, Chandan Rajendra wrote:
> This commit removes the f2fs specific f2fs_encrypted_inode() and makes
> use of the generic IS_ENCRYPTED() macro to check for the encryption
> status of an inode.
>
> Acked-by: Chao Yu
> Signed-off-by: Chandan Rajendra
This commit message is incorrect because f2fs_encrypted_inode() isn't actually
removed by this patch. Did you mean to remove it? I think you can if you
change f2fs_encrypted_file() to:
static inline bool f2fs_encrypted_file(struct inode *inode)
{
return IS_ENCRYPTED(inode) && S_ISREG(inode->i_mode);
}
... and if you replace the other calls to f2fs_encrypted_inode() with
file_is_encrypt().
- Eric
> ---
> fs/f2fs/data.c | 4 ++--
> fs/f2fs/dir.c | 10 +-
> fs/f2fs/file.c | 10 +-
> fs/f2fs/namei.c | 6 +++---
> 4 files changed, 15 insertions(+), 15 deletions(-)
>
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 09d9fc1676a7..844ec573263e 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -1491,7 +1491,7 @@ int f2fs_fiemap(struct inode *inode, struct
> fiemap_extent_info *fieinfo,
> }
>
> if (size) {
> - if (f2fs_encrypted_inode(inode))
> + if (IS_ENCRYPTED(inode))
> flags |= FIEMAP_EXTENT_DATA_ENCRYPTED;
>
> ret = fiemap_fill_next_extent(fieinfo, logical,
> @@ -1764,7 +1764,7 @@ static inline bool check_inplace_update_policy(struct
> inode *inode,
> if (policy & (0x1 << F2FS_IPU_ASYNC) &&
> fio && fio->op == REQ_OP_WRITE &&
> !(fio->op_flags & REQ_SYNC) &&
> - !f2fs_encrypted_inode(inode))
> + !IS_ENCRYPTED(inode))
> return true;
>
> /* this is only set during fdatasync */
> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> index bacc667950b6..cf9e2564388d 100644
> --- a/fs/f2fs/dir.c
> +++ b/fs/f2fs/dir.c
> @@ -385,7 +385,7 @@ struct page *f2fs_init_inode_metadata(struct inode
> *inode, struct inode *dir,
> if (err)
> goto put_error;
>
> - if ((f2fs_encrypted_inode(dir) || dummy_encrypt) &&
> + if ((IS_ENCRYPTED(dir) || dummy_encrypt) &&
> f2fs_may_encrypt(inode)) {
> err = fscrypt_inherit_context(dir, inode, page, false);
> if (err)
> @@ -399,7 +399,7 @@ struct page *f2fs_init_inode_metadata(struct inode
> *inode, struct inode *dir,
>
> if (new_name) {
> init_dent_inode(new_name, page);
> - if (f2fs_encrypted_inode(dir))
> + if (IS_ENCRYPTED(dir))
> file_set_enc_name(inode);
> }
>
> @@ -808,7 +808,7 @@ int f2fs_fill_dentries(struct dir_context *ctx, struct
> f2fs_dentry_ptr *d,
> de_name.name = d->filename[bit_pos];
> de_name.len = le16_to_cpu(de->name_len);
>
> - if (f2fs_encrypted_inode(d->inode)) {
> + if (IS_ENCRYPTED(d->inode)) {
> int save_len = fstr->len;
>
> err = fscrypt_fname_disk_to_usr(d->inode,
> @@ -852,7 +852,7 @@ static int f2fs_readdir(struct file *file, struct
> dir_context *ctx)
> struct fscrypt_str fstr = FSTR_INIT(NULL, 0);
> int err = 0;
>
> - if (f2fs_encrypted_inode(inode)) {
> + if (IS_ENCRYPTED(inode)) {
> err = fscrypt_get_encryption_info(inode);
> if (err && err != -ENOKEY)
> goto out;
> @@ -914,7 +914,7 @@ static int f2fs_readdir(struct file *file, struct
> dir_context *ctx)
>
> static int f2fs_dir_open(struct inode *inode, struct file *filp)
> {
> - if (f2fs_encrypted_inode(inode))
> + if (IS_ENCRYPTED(inode))
> return fscrypt_get_encryption_info(inode) ? -EACCES : 0;
> return 0;
> }
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 87794b2a45ff..6c7ad15000b9 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -585,7 +585,7 @@ static int truncate_partial_data_page(struct inode
> *inode, u64 from,
> zero_user(page, offset, PAGE_SIZE - offset);
>
> /* An encrypted inode should have a key and truncate the last page. */
> - f2fs_bug_on(F2FS_I_SB(inode), cache_only &&
> f2fs_encrypted_inode(inode));
> + f2fs_bug_on(F2FS_I_SB(inode), cache_only && IS_ENCRYPTED(inode));
> if (!cache_only)
> set_page_dirty(page);
> f2fs_put_page(page, 1);
> @@ -730,7 +730,7 @@ int f2fs_getattr(const struct path *path, struct kstat
> *stat,
> stat->attributes |= STATX_ATTR_APPEND;
> if (flags & F2FS_COMPR_FL)
> stat->attributes |= STATX_ATTR_COMPRESSED;
> - if (f2fs_encrypted_inode(inode))
> + if (IS_ENCRYPTED(inode))
> stat->attributes |= STATX_ATTR_ENCRYPTED;
> if (flags & F2FS_IMMUTABLE_FL)
> stat->attributes |= STATX_ATTR_IM
Re: [PATCH V2 3/7] fscrypt: remove filesystem specific build config option
Hi Chandan, On Tue, Dec 04, 2018 at 03:26:46PM +0530, Chandan Rajendra wrote: > In order to have a common code base for fscrypt "post read" processing > for all filesystems which support encryption, this commit removes > filesystem specific build config option (e.g. CONFIG_EXT4_FS_ENCRYPTION) > and replaces it with a build option (i.e. CONFIG_FS_ENCRYPTION) whose > value affects all the filesystems making use of fscrypt. > > Signed-off-by: Chandan Rajendra [...] > -config F2FS_FS_ENCRYPTION > - bool "F2FS Encryption" > - depends on F2FS_FS > - depends on F2FS_FS_XATTR > - select FS_ENCRYPTION > - help > - Enable encryption of f2fs files and directories. This > - feature is similar to ecryptfs, but it is more memory > - efficient since it avoids caching the encrypted and > - decrypted pages in the page cache. > - [...] > -config UBIFS_FS_ENCRYPTION > - bool "UBIFS Encryption" > - depends on UBIFS_FS && UBIFS_FS_XATTR && BLOCK > - select FS_ENCRYPTION > - default n > - help > - Enable encryption of UBIFS files and directories. This > - feature is similar to ecryptfs, but it is more memory > - efficient since it avoids caching the encrypted and > - decrypted pages in the page cache. Will it cause problems that now f2fs encryption can be "enabled" without F2FS_FS_XATTR, and ubifs encryption without UBIFS_FS_XATTR && BLOCK? Otherwise I think this patch looks fine. I'm a bit concerned about the bloat from making FS_ENCRYPTION non-modular, but given that it will make sharing I/O code much easier, it's probably worthwhile. It would help to strip down the dependencies of FS_ENCRYPTION to just the stuff needed for just AES-256-XTS and AES-256-CTS. I already sent out a patch a couple months ago (https://patchwork.kernel.org/patch/10589319/) to remove CONFIG_CTR which isn't used at all; I'll remind Ted to apply that. But we could also drop CONFIG_SHA256, which is only needed for AES-128-CBC contents encryption. If we do that, it should be a separate patch, though. Thanks, - Eric
Re: [PATCH V2 4/7] Add S_VERITY and IS_VERITY()
On Tue, Dec 04, 2018 at 03:26:47PM +0530, Chandan Rajendra wrote:
> Similar to S_ENCRYPTED/IS_ENCRYPTED(), this commit adds
> S_VERITY/IS_VERITY() to be able to check if a VFS inode has verity
> information associated with it.
>
> Signed-off-by: Chandan Rajendra
> ---
> include/linux/fs.h | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index 389a35e028bf..de602d9f8d0e 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1938,6 +1938,7 @@ struct super_operations {
> #define S_DAX0 /* Make all the DAX code disappear */
> #endif
> #define S_ENCRYPTED 16384 /* Encrypted file (using fs/crypto/) */
> +#define S_VERITY 32768 /* Verity file (using fs/verity/) */
>
> /*
> * Note that nosuid etc flags are inode-specific: setting some file-system
> @@ -1978,6 +1979,7 @@ static inline bool sb_rdonly(const struct super_block
> *sb) { return sb->s_flags
> #define IS_NOSEC(inode) ((inode)->i_flags & S_NOSEC)
> #define IS_DAX(inode)((inode)->i_flags & S_DAX)
> #define IS_ENCRYPTED(inode) ((inode)->i_flags & S_ENCRYPTED)
> +#define IS_VERITY(inode) ((inode)->i_flags & S_VERITY)
>
> #define IS_WHITEOUT(inode) (S_ISCHR(inode->i_mode) && \
>(inode)->i_rdev == WHITEOUT_DEV)
> --
> 2.19.1
>
Reviewed-by: Eric Biggers
Re: [PATCH V2 5/7] ext4: use IS_VERITY() to check inode's fsverity status
On Tue, Dec 04, 2018 at 03:26:48PM +0530, Chandan Rajendra wrote:
> This commit now uses IS_VERITY() macro to check if fsverity is
> enabled on an inode.
>
> Signed-off-by: Chandan Rajendra
> ---
> fs/ext4/ext4.h | 9 -
> fs/ext4/file.c | 2 +-
> fs/ext4/inode.c| 10 ++
> fs/ext4/readpage.c | 2 +-
> fs/ext4/super.c| 1 +
> 5 files changed, 9 insertions(+), 15 deletions(-)
>
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index db21df885186..64bf9fb7ef18 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -2296,15 +2296,6 @@ extern unsigned ext4_free_clusters_after_init(struct
> super_block *sb,
> struct ext4_group_desc *gdp);
> ext4_fsblk_t ext4_inode_to_goal_block(struct inode *);
>
> -static inline bool ext4_verity_inode(struct inode *inode)
> -{
> -#ifdef CONFIG_EXT4_FS_VERITY
> - return ext4_test_inode_flag(inode, EXT4_INODE_VERITY);
> -#else
> - return false;
> -#endif
> -}
> -
> #ifdef CONFIG_FS_ENCRYPTION
> static inline int ext4_fname_setup_filename(struct inode *dir,
> const struct qstr *iname,
> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> index cb4b69ef01a2..30fbd663354f 100644
> --- a/fs/ext4/file.c
> +++ b/fs/ext4/file.c
> @@ -444,7 +444,7 @@ static int ext4_file_open(struct inode * inode, struct
> file * filp)
> if (ret)
> return ret;
>
> - if (ext4_verity_inode(inode)) {
> + if (IS_VERITY(inode)) {
> ret = fsverity_file_open(inode, filp);
> if (ret)
> return ret;
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 09d8857b0e3c..79d14d8bbbf4 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -3884,7 +3884,7 @@ static ssize_t ext4_direct_IO(struct kiocb *iocb,
> struct iov_iter *iter)
> return 0;
> #endif
>
> - if (ext4_verity_inode(inode))
> + if (IS_VERITY(inode))
> return 0;
>
> /*
> @@ -4726,7 +4726,7 @@ static bool ext4_should_use_dax(struct inode *inode)
> return false;
> if (ext4_test_inode_flag(inode, EXT4_INODE_ENCRYPT))
> return false;
> - if (ext4_verity_inode(inode))
> + if (ext4_test_inode_flag(inode, EXT4_INODE_VERITY))
> return false;
> return true;
> }
> @@ -4750,9 +4750,11 @@ void ext4_set_inode_flags(struct inode *inode)
> new_fl |= S_DAX;
> if (flags & EXT4_ENCRYPT_FL)
> new_fl |= S_ENCRYPTED;
> + if (flags & EXT4_VERITY_FL)
> + new_fl |= S_VERITY;
> inode_set_flags(inode, new_fl,
> S_SYNC|S_APPEND|S_IMMUTABLE|S_NOATIME|S_DIRSYNC|S_DAX|
> - S_ENCRYPTED);
> + S_ENCRYPTED|S_VERITY);
> }
>
> static blkcnt_t ext4_inode_blocks(struct ext4_inode *raw_inode,
> @@ -5510,7 +5512,7 @@ int ext4_setattr(struct dentry *dentry, struct iattr
> *attr)
> if (error)
> return error;
>
> - if (ext4_verity_inode(inode)) {
> + if (IS_VERITY(inode)) {
> error = fsverity_prepare_setattr(dentry, attr);
> if (error)
> return error;
> diff --git a/fs/ext4/readpage.c b/fs/ext4/readpage.c
> index 7252f0a60cdb..2c037df629dd 100644
> --- a/fs/ext4/readpage.c
> +++ b/fs/ext4/readpage.c
> @@ -206,7 +206,7 @@ static void mpage_end_io(struct bio *bio)
> static inline loff_t ext4_readpage_limit(struct inode *inode)
> {
> #ifdef CONFIG_EXT4_FS_VERITY
> - if (ext4_verity_inode(inode)) {
> + if (IS_VERITY(inode)) {
> if (inode->i_verity_info)
> /* limit to end of metadata region */
> return fsverity_full_i_size(inode);
> diff --git a/fs/ext4/super.c b/fs/ext4/super.c
> index 16fb483a6f4a..35ed3c48f8d2 100644
> --- a/fs/ext4/super.c
> +++ b/fs/ext4/super.c
> @@ -1344,6 +1344,7 @@ static int ext4_set_verity(struct inode *inode, loff_t
> data_i_size)
> err = ext4_reserve_inode_write(handle, inode, &iloc);
> if (err == 0) {
> ext4_set_inode_flag(inode, EXT4_INODE_VERITY);
> + ext4_set_inode_flags(inode);
> EXT4_I(inode)->i_disksize = data_i_size;
> err = ext4_mark_iloc_dirty(handle, inode, &iloc);
> }
> --
> 2.19.1
>
Reviewed-by: Eric Biggers
Note: we should also move the IS_VERITY() check into fsverity_file_open() and
fsverity_prepare_setattr(), to be like fscrypt_file_open() and
fscrypt_prepare_setattr(). But starting with just this is fine.
- Eric
Re: [PATCH V2 6/7] f2fs: use IS_VERITY() to check inode's fsverity status
Hi Chandan,
On Tue, Dec 04, 2018 at 03:26:49PM +0530, Chandan Rajendra wrote:
> This commit now uses IS_VERITY() macro to check if fsverity is
> enabled on an inode.
>
> Acked-by: Chao Yu
> Signed-off-by: Chandan Rajendra
> ---
> fs/f2fs/file.c | 6 +++---
> fs/f2fs/inode.c | 4 +++-
> 2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 6c7ad15000b9..2eb4821d95d1 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -491,7 +491,7 @@ static int f2fs_file_open(struct inode *inode, struct
> file *filp)
> if (err)
> return err;
>
> - if (f2fs_verity_file(inode)) {
> + if (IS_VERITY(inode)) {
> err = fsverity_file_open(inode, filp);
> if (err)
> return err;
> @@ -701,7 +701,7 @@ int f2fs_getattr(const struct path *path, struct kstat
> *stat,
> struct f2fs_inode *ri;
> unsigned int flags;
>
> - if (f2fs_verity_file(inode)) {
> + if (IS_VERITY(inode)) {
> /*
>* For fs-verity we need to override i_size with the original
>* data i_size. This requires I/O to the file which with
> @@ -800,7 +800,7 @@ int f2fs_setattr(struct dentry *dentry, struct iattr
> *attr)
> if (err)
> return err;
>
> - if (f2fs_verity_file(inode)) {
> + if (IS_VERITY(inode)) {
> err = fsverity_prepare_setattr(dentry, attr);
> if (err)
> return err;
> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> index ddef483ad689..02806feed64c 100644
> --- a/fs/f2fs/inode.c
> +++ b/fs/f2fs/inode.c
> @@ -45,9 +45,11 @@ void f2fs_set_inode_flags(struct inode *inode)
> new_fl |= S_DIRSYNC;
> if (f2fs_encrypted_inode(inode))
> new_fl |= S_ENCRYPTED;
> + if (f2fs_verity_file(inode))
> + new_fl |= S_VERITY;
> inode_set_flags(inode, new_fl,
> S_SYNC|S_APPEND|S_IMMUTABLE|S_NOATIME|S_DIRSYNC|
> - S_ENCRYPTED);
> + S_ENCRYPTED|S_VERITY);
> }
>
> static void __get_inode_rdev(struct inode *inode, struct f2fs_inode *ri)
> --
> 2.19.1
>
Similar to what I said for fscrypt: why not remove f2fs_verity_file() entirely
and always use IS_VERITY() or file_is_verity() as appropriate?
- Eric
Re: [PATCH V2 3/7] fscrypt: remove filesystem specific build config option
On Tue, Dec 04, 2018 at 03:26:46PM +0530, Chandan Rajendra wrote: > diff --git a/include/linux/fscrypt.h b/include/linux/fscrypt.h > index 952ab97af325..6ba193c23f37 100644 > --- a/include/linux/fscrypt.h > +++ b/include/linux/fscrypt.h > @@ -2,9 +2,8 @@ > /* > * fscrypt.h: declarations for per-file encryption > * > - * Filesystems that implement per-file encryption include this header > - * file with the __FS_HAS_ENCRYPTION set according to whether that filesystem > - * is being built with encryption support or not. > + * Filesystems that implement per-file encryption must include this header > + * file. > * > * Copyright (C) 2015, Google, Inc. > * There's still a definition of __FS_HAS_ENCRYPTION in fs/crypto/fscrypt_private.h. This patch removes everything that checks __FS_HAS_ENCRYPTION, so that should be removed too. Thanks, - Eric
Re: [PATCH V2 7/7] fsverity: Remove filesystem specific build config option
Hi Chandan,
On Tue, Dec 04, 2018 at 03:26:50PM +0530, Chandan Rajendra wrote:
> In order to have a common code base for fsverity "post read" processing
> for all filesystems which support fsverity, this commit removes
> filesystem specific build config option (e.g. CONFIG_EXT4_FS_VERITY)
> and replaces it with a build option (i.e. CONFIG_FS_VERITY) whose
> value affects all the filesystems making use of fsverity.
>
> Signed-off-by: Chandan Rajendra
> ---
> Documentation/filesystems/fsverity.rst | 4 ++--
> fs/ext4/Kconfig| 20
> fs/ext4/ext4.h | 2 --
> fs/ext4/readpage.c | 4 ++--
> fs/ext4/super.c| 6 +++---
> fs/ext4/sysfs.c| 4 ++--
> fs/f2fs/Kconfig| 20
> fs/f2fs/data.c | 2 +-
> fs/f2fs/f2fs.h | 2 --
> fs/f2fs/super.c| 6 +++---
> fs/f2fs/sysfs.c| 4 ++--
> fs/verity/Kconfig | 3 ++-
> include/linux/fs.h | 4 ++--
> include/linux/fsverity.h | 3 +--
> 14 files changed, 20 insertions(+), 64 deletions(-)
>
[...]
> diff --git a/include/linux/fsverity.h b/include/linux/fsverity.h
> index c30c4f6ed411..f37132c640ee 100644
> --- a/include/linux/fsverity.h
> +++ b/include/linux/fsverity.h
> @@ -19,8 +19,7 @@ struct fsverity_operations {
> int (*get_metadata_end)(struct inode *inode, loff_t *metadata_end_ret);
> };
>
> -#if __FS_HAS_VERITY
> -
> +#ifdef CONFIG_FS_VERITY
> /* ioctl.c */
> extern int fsverity_ioctl_enable(struct file *filp, const void __user *arg);
> extern int fsverity_ioctl_measure(struct file *filp, void __user *arg);
> --
> 2.19.1
>
Can you grep for __FS_HAS_VERITY? It's still mentioned twice in comments for
this #ifdef (these should be changed to CONFIG_FS_VERITY), and it's still
defined in fs/verity/fsverity_private.h (that one should be removed).
Thanks,
- Eric
Re: [PATCH V3 8/9] fsverity: Move verity status check to fsverity_file_open
Hi Chandan,
On Sat, Dec 08, 2018 at 12:21:43PM +0530, Chandan Rajendra wrote:
> Instead of conditionally checking for verity status of an inode before
> invoking fsverity_file_open(), this commit moves the check inside the
> definition of fsverity_file_open().
>
> Signed-off-by: Chandan Rajendra
> ---
> fs/ext4/file.c| 8 +++-
> fs/f2fs/file.c| 8 +++-
> fs/verity/setup.c | 3 +++
> 3 files changed, 9 insertions(+), 10 deletions(-)
>
> diff --git a/fs/ext4/file.c b/fs/ext4/file.c
> index 30fbd663354f..b404a857cd48 100644
> --- a/fs/ext4/file.c
> +++ b/fs/ext4/file.c
> @@ -444,11 +444,9 @@ static int ext4_file_open(struct inode * inode, struct
> file * filp)
> if (ret)
> return ret;
>
> - if (IS_VERITY(inode)) {
> - ret = fsverity_file_open(inode, filp);
> - if (ret)
> - return ret;
> - }
> + ret = fsverity_file_open(inode, filp);
> + if (ret)
> + return ret;
>
> /*
>* Set up the jbd2_inode if we are opening the inode for
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 2eb4821d95d1..925c0d9608da 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -491,11 +491,9 @@ static int f2fs_file_open(struct inode *inode, struct
> file *filp)
> if (err)
> return err;
>
> - if (IS_VERITY(inode)) {
> - err = fsverity_file_open(inode, filp);
> - if (err)
> - return err;
> - }
> + err = fsverity_file_open(inode, filp);
> + if (err)
> + return err;
>
> filp->f_mode |= FMODE_NOWAIT;
>
> diff --git a/fs/verity/setup.c b/fs/verity/setup.c
> index 08b609127531..bc463dc601b1 100644
> --- a/fs/verity/setup.c
> +++ b/fs/verity/setup.c
> @@ -771,6 +771,9 @@ static int setup_fsverity_info(struct inode *inode)
> */
> int fsverity_file_open(struct inode *inode, struct file *filp)
> {
> + if (!IS_VERITY(inode))
> + return 0;
> +
> if (filp->f_mode & FMODE_WRITE) {
> pr_debug("Denying opening verity file (ino %lu) for write\n",
>inode->i_ino);
> --
> 2.19.1
>
This will break ext4 and f2fs when !CONFIG_FS_VERITY because then:
static inline int fsverity_file_open(struct inode *inode, struct file *filp)
{
return -EOPNOTSUPP;
}
Can you please make it like fscrypt_file_open()?
static inline int fsverity_file_open(struct inode *inode, struct file *filp)
{
if (IS_VERITY(inode))
return -EOPNOTSUPP;
return 0;
}
Same with fsverity_prepare_setattr().
- Eric
Re: [PATCH V2 3/7] fscrypt: remove filesystem specific build config option
On Sat, Dec 08, 2018 at 12:37:20PM +0530, Chandan Rajendra wrote:
> On Wednesday, December 5, 2018 5:13:21 AM IST Eric Biggers wrote:
> > Hi Chandan,
> >
> > On Tue, Dec 04, 2018 at 03:26:46PM +0530, Chandan Rajendra wrote:
> > > In order to have a common code base for fscrypt "post read" processing
> > > for all filesystems which support encryption, this commit removes
> > > filesystem specific build config option (e.g. CONFIG_EXT4_FS_ENCRYPTION)
> > > and replaces it with a build option (i.e. CONFIG_FS_ENCRYPTION) whose
> > > value affects all the filesystems making use of fscrypt.
> > >
> > > Signed-off-by: Chandan Rajendra
> > [...]
> > > -config F2FS_FS_ENCRYPTION
> > > - bool "F2FS Encryption"
> > > - depends on F2FS_FS
> > > - depends on F2FS_FS_XATTR
> > > - select FS_ENCRYPTION
> > > - help
> > > - Enable encryption of f2fs files and directories. This
> > > - feature is similar to ecryptfs, but it is more memory
> > > - efficient since it avoids caching the encrypted and
> > > - decrypted pages in the page cache.
> > > -
> > [...]
> > > -config UBIFS_FS_ENCRYPTION
> > > - bool "UBIFS Encryption"
> > > - depends on UBIFS_FS && UBIFS_FS_XATTR && BLOCK
> > > - select FS_ENCRYPTION
> > > - default n
> > > - help
> > > - Enable encryption of UBIFS files and directories. This
> > > - feature is similar to ecryptfs, but it is more memory
> > > - efficient since it avoids caching the encrypted and
> > > - decrypted pages in the page cache.
> >
> > Will it cause problems that now f2fs encryption can be "enabled" without
> > F2FS_FS_XATTR, and ubifs encryption without UBIFS_FS_XATTR && BLOCK?
> >
> > Otherwise I think this patch looks fine. I'm a bit concerned about the
> > bloat
> > from making FS_ENCRYPTION non-modular, but given that it will make sharing
> > I/O
> > code much easier, it's probably worthwhile.
> >
> > It would help to strip down the dependencies of FS_ENCRYPTION to just the
> > stuff
> > needed for just AES-256-XTS and AES-256-CTS. I already sent out a patch a
> > couple months ago (https://patchwork.kernel.org/patch/10589319/) to remove
> > CONFIG_CTR which isn't used at all; I'll remind Ted to apply that. But we
> > could
> > also drop CONFIG_SHA256, which is only needed for AES-128-CBC contents
> > encryption. If we do that, it should be a separate patch, though.
>
> Hi Eric,
>
> fscrypt_valid_enc_modes() allows FS_ENCRYPTION_MODE_AES_128_CBC to be used for
> encryption of file's contents. This is consistent with what you had mentioned
> above.
>
> static inline bool fscrypt_valid_enc_modes(u32 contents_mode,
>u32 filenames_mode)
> {
> if (contents_mode == FS_ENCRYPTION_MODE_AES_128_CBC &&
> filenames_mode == FS_ENCRYPTION_MODE_AES_128_CTS)
> return true;
>
> if (contents_mode == FS_ENCRYPTION_MODE_AES_256_XTS &&
> filenames_mode == FS_ENCRYPTION_MODE_AES_256_CTS)
> return true;
>
> return false;
> }
>
> Hence FS_ENCRYPTION does need to have AES-128-CBC and by extension SHA256 code
> compiled in right?
>
No those algorithms don't have to be compiled in if userspace doesn't use the
(FS_ENCRYPTION_MODE_AES_128_CBC, FS_ENCRYPTION_MODE_AES_128_CTS) pair, because
the algorithms are allocated dynamically on-demand through the crypto API.
- Eric
Re: [PATCH V3 2/9] f2fs: use IS_ENCRYPTED() to check encryption status
On Sat, Dec 08, 2018 at 12:21:37PM +0530, Chandan Rajendra wrote:
> This commit removes the f2fs specific f2fs_encrypted_inode() and makes
> use of the generic IS_ENCRYPTED() macro to check for the encryption
> status of an inode.
>
> Acked-by: Chao Yu
> Signed-off-by: Chandan Rajendra
Reviewed-by: Eric Biggers
> ---
> fs/f2fs/data.c | 4 ++--
> fs/f2fs/dir.c | 10 +-
> fs/f2fs/f2fs.h | 7 +--
> fs/f2fs/file.c | 10 +-
> fs/f2fs/inode.c | 4 ++--
> fs/f2fs/namei.c | 6 +++---
> 6 files changed, 18 insertions(+), 23 deletions(-)
>
> diff --git a/fs/f2fs/data.c b/fs/f2fs/data.c
> index 09d9fc1676a7..844ec573263e 100644
> --- a/fs/f2fs/data.c
> +++ b/fs/f2fs/data.c
> @@ -1491,7 +1491,7 @@ int f2fs_fiemap(struct inode *inode, struct
> fiemap_extent_info *fieinfo,
> }
>
> if (size) {
> - if (f2fs_encrypted_inode(inode))
> + if (IS_ENCRYPTED(inode))
> flags |= FIEMAP_EXTENT_DATA_ENCRYPTED;
>
> ret = fiemap_fill_next_extent(fieinfo, logical,
> @@ -1764,7 +1764,7 @@ static inline bool check_inplace_update_policy(struct
> inode *inode,
> if (policy & (0x1 << F2FS_IPU_ASYNC) &&
> fio && fio->op == REQ_OP_WRITE &&
> !(fio->op_flags & REQ_SYNC) &&
> - !f2fs_encrypted_inode(inode))
> + !IS_ENCRYPTED(inode))
> return true;
>
> /* this is only set during fdatasync */
> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> index bacc667950b6..cf9e2564388d 100644
> --- a/fs/f2fs/dir.c
> +++ b/fs/f2fs/dir.c
> @@ -385,7 +385,7 @@ struct page *f2fs_init_inode_metadata(struct inode
> *inode, struct inode *dir,
> if (err)
> goto put_error;
>
> - if ((f2fs_encrypted_inode(dir) || dummy_encrypt) &&
> + if ((IS_ENCRYPTED(dir) || dummy_encrypt) &&
> f2fs_may_encrypt(inode)) {
> err = fscrypt_inherit_context(dir, inode, page, false);
> if (err)
> @@ -399,7 +399,7 @@ struct page *f2fs_init_inode_metadata(struct inode
> *inode, struct inode *dir,
>
> if (new_name) {
> init_dent_inode(new_name, page);
> - if (f2fs_encrypted_inode(dir))
> + if (IS_ENCRYPTED(dir))
> file_set_enc_name(inode);
> }
>
> @@ -808,7 +808,7 @@ int f2fs_fill_dentries(struct dir_context *ctx, struct
> f2fs_dentry_ptr *d,
> de_name.name = d->filename[bit_pos];
> de_name.len = le16_to_cpu(de->name_len);
>
> - if (f2fs_encrypted_inode(d->inode)) {
> + if (IS_ENCRYPTED(d->inode)) {
> int save_len = fstr->len;
>
> err = fscrypt_fname_disk_to_usr(d->inode,
> @@ -852,7 +852,7 @@ static int f2fs_readdir(struct file *file, struct
> dir_context *ctx)
> struct fscrypt_str fstr = FSTR_INIT(NULL, 0);
> int err = 0;
>
> - if (f2fs_encrypted_inode(inode)) {
> + if (IS_ENCRYPTED(inode)) {
> err = fscrypt_get_encryption_info(inode);
> if (err && err != -ENOKEY)
> goto out;
> @@ -914,7 +914,7 @@ static int f2fs_readdir(struct file *file, struct
> dir_context *ctx)
>
> static int f2fs_dir_open(struct inode *inode, struct file *filp)
> {
> - if (f2fs_encrypted_inode(inode))
> + if (IS_ENCRYPTED(inode))
> return fscrypt_get_encryption_info(inode) ? -EACCES : 0;
> return 0;
> }
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index dadb5f468f20..368c7e78fe6b 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -3436,14 +3436,9 @@ void f2fs_unregister_sysfs(struct f2fs_sb_info *sbi);
> /*
> * crypto support
> */
> -static inline bool f2fs_encrypted_inode(struct inode *inode)
> -{
> - return file_is_encrypt(inode);
> -}
> -
> static inline bool f2fs_encrypted_file(struct inode *inode)
> {
> - return f2fs_encrypted_inode(inode) && S_ISREG(inode->i_mode);
> + return IS_ENCRYPTED(inode) && S_ISREG(inode->i_mode);
> }
>
> static inline void f2fs_set_encrypted_inode(struct inode *inode)
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 87794b2a45ff..6c7ad15000b9 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -585,7 +585,7 @@ static int truncate_partial_data_page(struct inode
> *inode, u64 from,
> zero_user(page, offset, P
Re: [PATCH V3 3/9] fscrypt: remove filesystem specific build config option
On Sat, Dec 08, 2018 at 12:21:38PM +0530, Chandan Rajendra wrote: > In order to have a common code base for fscrypt "post read" processing > for all filesystems which support encryption, this commit removes > filesystem specific build config option (e.g. CONFIG_EXT4_FS_ENCRYPTION) > and replaces it with a build option (i.e. CONFIG_FS_ENCRYPTION) whose > value affects all the filesystems making use of fscrypt. > > Signed-off-by: Chandan Rajendra Reviewed-by: Eric Biggers > --- > Documentation/filesystems/fscrypt.rst | 4 +- > arch/mips/configs/generic_defconfig | 2 +- > arch/nds32/configs/defconfig| 2 +- > arch/s390/configs/debug_defconfig | 2 +- > arch/s390/configs/performance_defconfig | 2 +- > fs/crypto/Kconfig | 5 +- > fs/crypto/fscrypt_private.h | 1 - > fs/ext4/Kconfig | 15 - > fs/ext4/dir.c | 2 - > fs/ext4/ext4.h | 7 +- > fs/ext4/inode.c | 8 +- > fs/ext4/ioctl.c | 4 +- > fs/ext4/namei.c | 10 +- > fs/ext4/page-io.c | 6 +- > fs/ext4/readpage.c | 2 +- > fs/ext4/super.c | 6 +- > fs/ext4/sysfs.c | 4 +- > fs/f2fs/Kconfig | 12 +- > fs/f2fs/f2fs.h | 7 +- > fs/f2fs/super.c | 8 +- > fs/f2fs/sysfs.c | 4 +- > fs/ubifs/Kconfig| 13 +- > fs/ubifs/Makefile | 2 +- > fs/ubifs/ioctl.c| 4 +- > fs/ubifs/sb.c | 2 +- > fs/ubifs/super.c| 2 +- > fs/ubifs/ubifs.h| 5 +- > include/linux/fs.h | 4 +- > include/linux/fscrypt.h | 416 +++- > include/linux/fscrypt_notsupp.h | 231 - > include/linux/fscrypt_supp.h| 204 > 31 files changed, 461 insertions(+), 535 deletions(-) > delete mode 100644 include/linux/fscrypt_notsupp.h > delete mode 100644 include/linux/fscrypt_supp.h > > diff --git a/Documentation/filesystems/fscrypt.rst > b/Documentation/filesystems/fscrypt.rst > index cfbc18f0d9c9..92084762ad20 100644 > --- a/Documentation/filesystems/fscrypt.rst > +++ b/Documentation/filesystems/fscrypt.rst > @@ -318,9 +318,9 @@ FS_IOC_SET_ENCRYPTION_POLICY can fail with the following > errors: > - ``ENOTEMPTY``: the file is unencrypted and is a nonempty directory > - ``ENOTTY``: this type of filesystem does not implement encryption > - ``EOPNOTSUPP``: the kernel was not configured with encryption > - support for this filesystem, or the filesystem superblock has not > + support for filesystems, or the filesystem superblock has not >had encryption enabled on it. (For example, to use encryption on an > - ext4 filesystem, CONFIG_EXT4_ENCRYPTION must be enabled in the > + ext4 filesystem, CONFIG_FS_ENCRYPTION must be enabled in the >kernel config, and the superblock must have had the "encrypt" >feature flag enabled using ``tune2fs -O encrypt`` or ``mkfs.ext4 -O >encrypt``.) > diff --git a/arch/mips/configs/generic_defconfig > b/arch/mips/configs/generic_defconfig > index 684c9dcba126..b8a21b9b934a 100644 > --- a/arch/mips/configs/generic_defconfig > +++ b/arch/mips/configs/generic_defconfig > @@ -63,7 +63,7 @@ CONFIG_HID_MONTEREY=y > CONFIG_EXT4_FS=y > CONFIG_EXT4_FS_POSIX_ACL=y > CONFIG_EXT4_FS_SECURITY=y > -CONFIG_EXT4_ENCRYPTION=y > +CONFIG_FS_ENCRYPTION=y > CONFIG_FANOTIFY=y > CONFIG_FUSE_FS=y > CONFIG_CUSE=y > diff --git a/arch/nds32/configs/defconfig b/arch/nds32/configs/defconfig > index 2546d8770785..65ce9259081b 100644 > --- a/arch/nds32/configs/defconfig > +++ b/arch/nds32/configs/defconfig > @@ -74,7 +74,7 @@ CONFIG_GENERIC_PHY=y > CONFIG_EXT4_FS=y > CONFIG_EXT4_FS_POSIX_ACL=y > CONFIG_EXT4_FS_SECURITY=y > -CONFIG_EXT4_ENCRYPTION=y > +CONFIG_FS_ENCRYPTION=y > CONFIG_FUSE_FS=y > CONFIG_MSDOS_FS=y > CONFIG_VFAT_FS=y > diff --git a/arch/s390/configs/debug_defconfig > b/arch/s390/configs/debug_defconfig > index 259d1698ac50..e8868e0fba76 100644 > --- a/arch/s390/configs/debug_defconfig > +++ b/arch/s390/configs/debug_defconfig > @@ -492,7 +492,6 @@ CONFIG_VIRTIO_INPUT=y > CONFIG_EXT4_FS=y > CONFIG_EXT4_FS_POSIX_ACL=y > CONFIG_EXT4_FS_SECURITY=y > -CONFIG_EXT4_ENCRYPTION=y > CONFIG_JBD2_DEBUG=y > CONFIG_JFS_FS=m > CONFIG_JFS_POSIX_ACL=y > @@ -512,6 +511,7
Re: [PATCH V3 6/9] f2fs: use IS_VERITY() to check inode's fsverity status
On Sat, Dec 08, 2018 at 12:21:41PM +0530, Chandan Rajendra wrote:
> This commit removes the f2fs specific f2fs_verity_file() and makes use
> of the generic IS_VERITY() macro or file_is_verity() to check for the
> verity status of an inode.
>
> Signed-off-by: Chandan Rajendra
Reviewed-by: Eric Biggers
> ---
> fs/f2fs/f2fs.h | 7 +--
> fs/f2fs/file.c | 6 +++---
> fs/f2fs/inode.c | 4 +++-
> fs/f2fs/super.c | 1 +
> 4 files changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index f9c8b0afc1de..54bd93c7b630 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -3448,18 +3448,13 @@ static inline void f2fs_set_encrypted_inode(struct
> inode *inode)
> #endif
> }
>
> -static inline bool f2fs_verity_file(struct inode *inode)
> -{
> - return file_is_verity(inode);
> -}
> -
> /*
> * Returns true if the reads of the inode's data need to undergo some
> * postprocessing step, like decryption or authenticity verification.
> */
> static inline bool f2fs_post_read_required(struct inode *inode)
> {
> - return f2fs_encrypted_file(inode) || f2fs_verity_file(inode);
> + return f2fs_encrypted_file(inode) || IS_VERITY(inode);
> }
>
> #define F2FS_FEATURE_FUNCS(name, flagname) \
> diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
> index 6c7ad15000b9..2eb4821d95d1 100644
> --- a/fs/f2fs/file.c
> +++ b/fs/f2fs/file.c
> @@ -491,7 +491,7 @@ static int f2fs_file_open(struct inode *inode, struct
> file *filp)
> if (err)
> return err;
>
> - if (f2fs_verity_file(inode)) {
> + if (IS_VERITY(inode)) {
> err = fsverity_file_open(inode, filp);
> if (err)
> return err;
> @@ -701,7 +701,7 @@ int f2fs_getattr(const struct path *path, struct kstat
> *stat,
> struct f2fs_inode *ri;
> unsigned int flags;
>
> - if (f2fs_verity_file(inode)) {
> + if (IS_VERITY(inode)) {
> /*
>* For fs-verity we need to override i_size with the original
>* data i_size. This requires I/O to the file which with
> @@ -800,7 +800,7 @@ int f2fs_setattr(struct dentry *dentry, struct iattr
> *attr)
> if (err)
> return err;
>
> - if (f2fs_verity_file(inode)) {
> + if (IS_VERITY(inode)) {
> err = fsverity_prepare_setattr(dentry, attr);
> if (err)
> return err;
> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> index d731d23aedee..ff9126616d65 100644
> --- a/fs/f2fs/inode.c
> +++ b/fs/f2fs/inode.c
> @@ -45,9 +45,11 @@ void f2fs_set_inode_flags(struct inode *inode)
> new_fl |= S_DIRSYNC;
> if (file_is_encrypt(inode))
> new_fl |= S_ENCRYPTED;
> + if (file_is_verity(inode))
> + new_fl |= S_VERITY;
> inode_set_flags(inode, new_fl,
> S_SYNC|S_APPEND|S_IMMUTABLE|S_NOATIME|S_DIRSYNC|
> - S_ENCRYPTED);
> + S_ENCRYPTED|S_VERITY);
> }
>
> static void __get_inode_rdev(struct inode *inode, struct f2fs_inode *ri)
> diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
> index 4287cf348d3c..73320202bd01 100644
> --- a/fs/f2fs/super.c
> +++ b/fs/f2fs/super.c
> @@ -2207,6 +2207,7 @@ static int f2fs_set_verity(struct inode *inode, loff_t
> data_i_size)
> return err;
>
> file_set_verity(inode);
> + f2fs_set_inode_flags(inode);
> f2fs_mark_inode_dirty_sync(inode, true);
> return 0;
> }
> --
> 2.19.1
>
Re: [PATCH V3 7/9] fsverity: Remove filesystem specific build config option
On Sat, Dec 08, 2018 at 12:21:42PM +0530, Chandan Rajendra wrote: > In order to have a common code base for fsverity "post read" processing > for all filesystems which support fsverity, this commit removes > filesystem specific build config option (e.g. CONFIG_EXT4_FS_VERITY) > and replaces it with a build option (i.e. CONFIG_FS_VERITY) whose > value affects all the filesystems making use of fsverity. > > Signed-off-by: Chandan Rajendra Reviewed-by: Eric Biggers > --- > Documentation/filesystems/fsverity.rst | 4 ++-- > fs/ext4/Kconfig| 20 > fs/ext4/ext4.h | 2 -- > fs/ext4/readpage.c | 4 ++-- > fs/ext4/super.c| 6 +++--- > fs/ext4/sysfs.c| 4 ++-- > fs/f2fs/Kconfig| 20 > fs/f2fs/data.c | 2 +- > fs/f2fs/f2fs.h | 2 -- > fs/f2fs/super.c| 6 +++--- > fs/f2fs/sysfs.c| 4 ++-- > fs/verity/Kconfig | 3 ++- > fs/verity/fsverity_private.h | 1 - > include/linux/fs.h | 4 ++-- > include/linux/fsverity.h | 7 +++ > 15 files changed, 22 insertions(+), 67 deletions(-) > > diff --git a/Documentation/filesystems/fsverity.rst > b/Documentation/filesystems/fsverity.rst > index d633fc0567bd..bb208dad10d9 100644 > --- a/Documentation/filesystems/fsverity.rst > +++ b/Documentation/filesystems/fsverity.rst > @@ -279,7 +279,7 @@ ext4 > > ext4 supports fs-verity since kernel version TODO. > > -CONFIG_EXT4_FS_VERITY must be enabled in the kernel config. Also, the > +CONFIG_FS_VERITY must be enabled in the kernel config. Also, the > filesystem must have been formatted with ``-O verity``, or had > ``tune2fs -O verity`` run on it. These require e2fsprogs v1.44.4-2 or > later. This e2fsprogs version is also required for e2fsck to > @@ -306,7 +306,7 @@ f2fs > > f2fs supports fs-verity since kernel version TODO. > > -CONFIG_F2FS_FS_VERITY must be enabled in the kernel config. Also, the > +CONFIG_FS_VERITY must be enabled in the kernel config. Also, the > filesystem must have been formatted with ``-O verity``. This requires > f2fs-tools v1.11.0 or later. > > diff --git a/fs/ext4/Kconfig b/fs/ext4/Kconfig > index e1002bbf35bf..031e5a82d556 100644 > --- a/fs/ext4/Kconfig > +++ b/fs/ext4/Kconfig > @@ -96,26 +96,6 @@ config EXT4_FS_SECURITY > If you are not using a security module that requires using > extended attributes for file security labels, say N. > > -config EXT4_FS_VERITY > - bool "Ext4 Verity" > - depends on EXT4_FS > - select FS_VERITY > - help > - This option enables fs-verity for ext4. fs-verity is the > - dm-verity mechanism implemented at the file level. Userspace > - can append a Merkle tree (hash tree) to a file, then enable > - fs-verity on the file. ext4 will then transparently verify > - any data read from the file against the Merkle tree. The file > - is also made read-only. > - > - This serves as an integrity check, but the availability of the > - Merkle tree root hash also allows efficiently supporting > - various use cases where normally the whole file would need to > - be hashed at once, such as auditing and authenticity > - verification (appraisal). > - > - If unsure, say N. > - > config EXT4_DEBUG > bool "EXT4 debugging support" > depends on EXT4_FS > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h > index 64bf9fb7ef18..bff8d639dd0c 100644 > --- a/fs/ext4/ext4.h > +++ b/fs/ext4/ext4.h > @@ -41,8 +41,6 @@ > #endif > > #include > - > -#define __FS_HAS_VERITY IS_ENABLED(CONFIG_EXT4_FS_VERITY) > #include > > #include > diff --git a/fs/ext4/readpage.c b/fs/ext4/readpage.c > index 2c037df629dd..8717ac0a5bb2 100644 > --- a/fs/ext4/readpage.c > +++ b/fs/ext4/readpage.c > @@ -158,7 +158,7 @@ static struct bio_post_read_ctx > *get_bio_post_read_ctx(struct inode *inode, > > if (IS_ENCRYPTED(inode) && S_ISREG(inode->i_mode)) > post_read_steps |= 1 << STEP_DECRYPT; > -#ifdef CONFIG_EXT4_FS_VERITY > +#ifdef CONFIG_FS_VERITY > if (inode->i_verity_info != NULL && > (index < ((i_size_read(inode) + PAGE_SIZE - 1) >> PAGE_SHIFT))) > post_read_steps |= 1 << STEP_VERITY; > @@ -205,7 +205,7 @@ static void mpage_end_io(struct bio *bio) > > static inline loff_t
Re: [PATCH V2 3/7] fscrypt: remove filesystem specific build config option
On Tue, Dec 11, 2018 at 05:52:11PM -0800, Guenter Roeck wrote: > Hi, > > On Tue, Dec 04, 2018 at 03:26:46PM +0530, Chandan Rajendra wrote: > > In order to have a common code base for fscrypt "post read" processing > > for all filesystems which support encryption, this commit removes > > filesystem specific build config option (e.g. CONFIG_EXT4_FS_ENCRYPTION) > > and replaces it with a build option (i.e. CONFIG_FS_ENCRYPTION) whose > > value affects all the filesystems making use of fscrypt. > > > > Signed-off-by: Chandan Rajendra > > this patch causes a recursive dependency when trying to build ia64 images. > > make ARCH=ia64 allnoconfig: > > scripts/kconfig/conf --allnoconfig Kconfig > arch/ia64/Kconfig:126:error: recursive dependency detected! > arch/ia64/Kconfig:126:choice contains symbol IA64_HP_SIM > arch/ia64/Kconfig:200:symbol IA64_HP_SIM is part of choice PM > kernel/power/Kconfig:144: symbol PM is selected by PM_SLEEP > kernel/power/Kconfig:104: symbol PM_SLEEP depends on HIBERNATE_CALLBACKS > kernel/power/Kconfig:31: symbol HIBERNATE_CALLBACKS is selected by > HIBERNATION > kernel/power/Kconfig:34: symbol HIBERNATION depends on SWAP > init/Kconfig:250: symbol SWAP depends on BLOCK > block/Kconfig:5: symbol BLOCK is selected by UBIFS_FS > fs/ubifs/Kconfig:1: symbol UBIFS_FS depends on MISC_FILESYSTEMS > fs/Kconfig:220: symbol MISC_FILESYSTEMS is selected by ACPI_APEI > drivers/acpi/apei/Kconfig:8: symbol ACPI_APEI depends on ACPI > drivers/acpi/Kconfig:9: symbol ACPI depends on ARCH_SUPPORTS_ACPI > drivers/acpi/Kconfig:6: symbol ARCH_SUPPORTS_ACPI is selected by > IA64_HP_SIM > arch/ia64/Kconfig:200:symbol IA64_HP_SIM is part of choice > For a resolution refer to Documentation/kbuild/kconfig-language.txt > subsection "Kconfig recursive dependency limitations" > > scripts/kconfig/Makefile:69: recipe for target 'allnoconfig' failed > > Reverting the patch fixes the problem. > Thanks for the report. Chandan, it appears the problem is UBIFS_FS selecting BLOCK. It's actually not necessary because now the parts of fs/crypto/ that depend on BLOCK are separated out into a separate file fs/crypto/bio.c that is only compiled with CONFIG_BLOCK. So how about just removing the selection of BLOCK from fs/ubifs/Kconfig: select BLOCK if FS_ENCRYPTION - Eric
Re: [PATCH] Documentation: coding-style: don't encourage WARN*()
On Sun, Apr 14, 2024 at 12:08:50PM -0500, Alex Elder wrote: > diff --git a/Documentation/process/coding-style.rst > b/Documentation/process/coding-style.rst > index 9c7cf73473943..bce43b01721cb 100644 > --- a/Documentation/process/coding-style.rst > +++ b/Documentation/process/coding-style.rst > @@ -1235,17 +1235,18 @@ example. Again: WARN*() must not be used for a > condition that is expected > to trigger easily, for example, by user space actions. pr_warn_once() is a > possible alternative, if you need to notify the user of a problem. > > -Do not worry about panic_on_warn users > -** > +The panic_on_warn kernel option > + > > -A few more words about panic_on_warn: Remember that ``panic_on_warn`` is an > -available kernel option, and that many users set this option. This is why > -there is a "Do not WARN lightly" writeup, above. However, the existence of > -panic_on_warn users is not a valid reason to avoid the judicious use > -WARN*(). That is because, whoever enables panic_on_warn has explicitly > -asked the kernel to crash if a WARN*() fires, and such users must be > -prepared to deal with the consequences of a system that is somewhat more > -likely to crash. > +Note that ``panic_on_warn`` is an available kernel option. If it is enabled, > +a WARN*() call whose condition holds leads to a kernel panic. Many users > +(including Android and many cloud providers) set this option, and this is > +why there is a "Do not WARN lightly" writeup, above. > + > +The existence of this option is not a valid reason to avoid the judicious > +use of warnings. There are other options: ``dev_warn*()`` and ``pr_warn*()`` > +issue warnings but do **not** cause the kernel to crash. Use these if you > +want to prevent such panics. > Nacked-by: Eric Biggers WARN*() are for recoverable assertions, i.e. situations where the condition being true can only happen due to a kernel bug but where they can be recovered from (unlike BUG*() which are for unrecoverable situations). The people who use panic_on_warn *want* the kernel to crash when such a situation happens so that the underlying issue can be discovered and fixed. That's the whole point. Also, it's not true that "Android" sets this option. It might be the case that some individual Android OEMs have decided to use it for some reason (they do have the ability to customize their kernel command line, after all). It's certainly not used by default or even recommended. - Eric
