Re: [f2fs-dev] [PATCH] fscrypto: move ioctl processing more fully into common code
On Sat, Nov 26, 2016 at 08:20:48PM -0800, Eric Biggers wrote: > > I guess I'm okay with that, since struct fscrypt_policy won't have any padding > bytes because its members are all bytes. Plus it's marked __packed, though I > think that was a mistake given that the struct isn't stored on disk directly. > It wouldn't have mattered if wasn't marked __packed, since the first four fields are __u8, and the master_key_descriptor is a 4 byte aligned __u8 array of size 8. The use of __packed in the fscrypt code came from Michael, and I suspect it's more of a Microsoft thing, since his previous experience was as the architect for Bitlocker. It's actually pretty rare that we use __packed in Linux kernel sources in general, and in ext4 specifically. Personally, I tend to depend on __uNN declaration and various assumptions that we make about "sane" packing rules which are assumed by the kernel. See how the on-disk ext4 superblock is defined; that's not the only place where we make assumptions about sane structure packing, and anyone who tried porting Linux to a 18-bit or 36-bit architecture would have lots of other problems, even if a modern Linux kernel could be made small enough to fit in the memory available to a PDP-10 or a PDP-15. :-) We probably could remove a few of them, but I haven't bothered, since in general they aren't doing any harm. Cheers, - Ted -- ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH] fscrypto: move ioctl processing more fully into common code
On Sat, Nov 26, 2016 at 07:09:01PM -0500, Theodore Ts'o wrote: > On Mon, Oct 17, 2016 at 09:54:06AM -0700, Eric Biggers wrote: > > In addition, make the common functions do the copies to and from > > userspace rather than duplicating this code within each filesystem, and > > memset the policy to 0 to make it clear there is no stack leak. > > I don't see any point of doing this, given that we initialize all > parts of the fscrypt_policy structure; and since this structure is > part of UAPI, we can't change it without breaking userspace. > > I'll apply this with the memset (and the above comment in the commit > description) removed. > > - Ted I guess I'm okay with that, since struct fscrypt_policy won't have any padding bytes because its members are all bytes. Plus it's marked __packed, though I think that was a mistake given that the struct isn't stored on disk directly. Eric -- ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH] fscrypto: move ioctl processing more fully into common code
On Mon, Oct 17, 2016 at 09:54:06AM -0700, Eric Biggers wrote: > In addition, make the common functions do the copies to and from > userspace rather than duplicating this code within each filesystem, and > memset the policy to 0 to make it clear there is no stack leak. I don't see any point of doing this, given that we initialize all parts of the fscrypt_policy structure; and since this structure is part of UAPI, we can't change it without breaking userspace. I'll apply this with the memset (and the above comment in the commit description) removed. - Ted -- ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH] fscrypto: move ioctl processing more fully into common code
On Mon, Oct 17, 2016 at 09:54:06AM -0700, Eric Biggers wrote: > Multiple bugs were recently fixed in the "set encryption policy" ioctl. > To make it clear that fscrypt_process_policy() and fscrypt_get_policy() > implement ioctls and therefore their implementations must take standard > security and correctness precautions, rename them to > fscrypt_ioctl_set_policy() and fscrypt_ioctl_get_policy(). Make the > latter take in a struct file * to make it consistent with the former. > > In addition, make the common functions do the copies to and from > userspace rather than duplicating this code within each filesystem, and > memset the policy to 0 to make it clear there is no stack leak. > Ted, do you have any interest in taking this patch for 4.10? Thanks, Eric -- ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH] fscrypto: move ioctl processing more fully into common code
On Tue, Oct 18, 2016 at 02:22:07PM +0200, Richard Weinberger wrote: > > Hmm, are you sure the change is worth it? > The patch basically moves a copy_from/to_user() from ext4/f2fs into fscrypto. > Hi Richard, In my opinion consolidating the copy_from/to_user() is worthwhile by itself. The filesystem encryption code should be shared when possible, and right now there's no reason for each filesystem to do its own copy_from/to_user(). The renaming to fscrypt_ioctl_* is also important because it makes it clear that the functions implement ioctls. I've already fixed four bugs in the "set policy" ioctl, and I think these bugs would have been more obvious with a clearer code organization. Eric -- Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH] fscrypto: move ioctl processing more fully into common code
Eric, On Mon, Oct 17, 2016 at 6:54 PM, Eric Biggers wrote: > Multiple bugs were recently fixed in the "set encryption policy" ioctl. > To make it clear that fscrypt_process_policy() and fscrypt_get_policy() > implement ioctls and therefore their implementations must take standard > security and correctness precautions, rename them to > fscrypt_ioctl_set_policy() and fscrypt_ioctl_get_policy(). Make the > latter take in a struct file * to make it consistent with the former. > > In addition, make the common functions do the copies to and from > userspace rather than duplicating this code within each filesystem, and > memset the policy to 0 to make it clear there is no stack leak. > > Signed-off-by: Eric Biggers > --- > fs/crypto/policy.c | 36 +++- > fs/ext4/ext4.h | 4 ++-- > fs/ext4/ioctl.c | 34 +- > fs/f2fs/f2fs.h | 4 ++-- > fs/f2fs/file.c | 19 ++- > include/linux/fscrypto.h | 12 ++-- > 6 files changed, 40 insertions(+), 69 deletions(-) Hmm, are you sure the change is worth it? The patch basically moves a copy_from/to_user() from ext4/f2fs into fscrypto. -- Thanks, //richard -- Check out the vibrant tech community on one of the world's most engaging tech sites, SlashDot.org! http://sdm.link/slashdot ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [PATCH] fscrypto: move ioctl processing more fully into common code
Multiple bugs were recently fixed in the "set encryption policy" ioctl.
To make it clear that fscrypt_process_policy() and fscrypt_get_policy()
implement ioctls and therefore their implementations must take standard
security and correctness precautions, rename them to
fscrypt_ioctl_set_policy() and fscrypt_ioctl_get_policy(). Make the
latter take in a struct file * to make it consistent with the former.
In addition, make the common functions do the copies to and from
userspace rather than duplicating this code within each filesystem, and
memset the policy to 0 to make it clear there is no stack leak.
Signed-off-by: Eric Biggers
---
fs/crypto/policy.c | 36 +++-
fs/ext4/ext4.h | 4 ++--
fs/ext4/ioctl.c | 34 +-
fs/f2fs/f2fs.h | 4 ++--
fs/f2fs/file.c | 19 ++-
include/linux/fscrypto.h | 12 ++--
6 files changed, 40 insertions(+), 69 deletions(-)
diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c
index 6865663..fa30618 100644
--- a/fs/crypto/policy.c
+++ b/fs/crypto/policy.c
@@ -93,16 +93,19 @@ static int create_encryption_context_from_policy(struct
inode *inode,
return inode->i_sb->s_cop->set_context(inode, &ctx, sizeof(ctx), NULL);
}
-int fscrypt_process_policy(struct file *filp,
- const struct fscrypt_policy *policy)
+int fscrypt_ioctl_set_policy(struct file *filp, const void __user *arg)
{
+ struct fscrypt_policy policy;
struct inode *inode = file_inode(filp);
int ret;
+ if (copy_from_user(&policy, arg, sizeof(policy)))
+ return -EFAULT;
+
if (!inode_owner_or_capable(inode))
return -EACCES;
- if (policy->version != 0)
+ if (policy.version != 0)
return -EINVAL;
ret = mnt_want_write_file(filp);
@@ -120,9 +123,9 @@ int fscrypt_process_policy(struct file *filp,
ret = -ENOTEMPTY;
else
ret = create_encryption_context_from_policy(inode,
- policy);
+ &policy);
} else if (!is_encryption_context_consistent_with_policy(inode,
-policy)) {
+&policy)) {
printk(KERN_WARNING
"%s: Policy inconsistent with encryption context\n",
__func__);
@@ -134,11 +137,13 @@ int fscrypt_process_policy(struct file *filp,
mnt_drop_write_file(filp);
return ret;
}
-EXPORT_SYMBOL(fscrypt_process_policy);
+EXPORT_SYMBOL(fscrypt_ioctl_set_policy);
-int fscrypt_get_policy(struct inode *inode, struct fscrypt_policy *policy)
+int fscrypt_ioctl_get_policy(struct file *filp, void __user *arg)
{
+ struct inode *inode = file_inode(filp);
struct fscrypt_context ctx;
+ struct fscrypt_policy policy;
int res;
if (!inode->i_sb->s_cop->get_context ||
@@ -151,15 +156,20 @@ int fscrypt_get_policy(struct inode *inode, struct
fscrypt_policy *policy)
if (ctx.format != FS_ENCRYPTION_CONTEXT_FORMAT_V1)
return -EINVAL;
- policy->version = 0;
- policy->contents_encryption_mode = ctx.contents_encryption_mode;
- policy->filenames_encryption_mode = ctx.filenames_encryption_mode;
- policy->flags = ctx.flags;
- memcpy(&policy->master_key_descriptor, ctx.master_key_descriptor,
+ memset(&policy, 0, sizeof(policy));
+
+ policy.version = 0;
+ policy.contents_encryption_mode = ctx.contents_encryption_mode;
+ policy.filenames_encryption_mode = ctx.filenames_encryption_mode;
+ policy.flags = ctx.flags;
+ memcpy(policy.master_key_descriptor, ctx.master_key_descriptor,
FS_KEY_DESCRIPTOR_SIZE);
+
+ if (copy_to_user(arg, &policy, sizeof(policy)))
+ return -EFAULT;
return 0;
}
-EXPORT_SYMBOL(fscrypt_get_policy);
+EXPORT_SYMBOL(fscrypt_ioctl_get_policy);
int fscrypt_has_permitted_context(struct inode *parent, struct inode *child)
{
diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 282a51b..bd8bc3b 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -2338,8 +2338,8 @@ static inline void ext4_fname_free_filename(struct
ext4_filename *fname) { }
#define fscrypt_pullback_bio_page fscrypt_notsupp_pullback_bio_page
#define fscrypt_restore_control_page fscrypt_notsupp_restore_control_page
#define fscrypt_zeroout_range fscrypt_notsupp_zeroout_range
-#define fscrypt_process_policy fscrypt_notsupp_process_policy
-#define fscrypt_get_policy fscrypt_notsupp_get_policy
+#define fscrypt_ioctl_set_policy fscrypt_notsupp_ioctl_set_policy
+#define fscrypt_ioctl_get_
