Re: [f2fs-dev] [PATCH] fscrypto: move ioctl processing more fully into common code

[Theodore Ts'o]

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

[Eric Biggers]

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

[Eric Biggers]

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

[Eric Biggers]

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

[Richard Weinberger]

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