Re: [PATCH] ubifs: Return -ENOKEY from rename if encryption keys are missing

Re: [PATCH] ubifs: Return -ENOKEY from rename if encryption keys are missing

[Eric Biggers]

Hi David and Richard,

On Mon, Apr 24, 2017 at 11:46:21PM +0200, Richard Weinberger wrote:
> From: David Oberhollenzer <david.oberhollenzer@sigma-star.at>
> 
> If either source or destination directory is encrypted and the
> encryption key is unknown, make sure we return -ENOKEY instead
> of -EPERM, similar to how this case is handled in ext4.
> 
> Signed-off-by: David Oberhollenzer <david.oberhollenzer@sigma-star.at>
> Signed-off-by: Richard Weinberger <richard@nod.at>
> 
> diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
> index ff77a0aa2f2b..c342f23581d2 100644
> --- a/fs/ubifs/dir.c
> +++ b/fs/ubifs/dir.c
> @@ -1340,6 +1340,12 @@ static int do_rename(struct inode *old_dir, struct dentry *old_dentry,
>  	if (unlink)
>  		ubifs_assert(inode_is_locked(new_inode));
>  
> +	if ((ubifs_crypt_is_encrypted(old_dir) &&
> +	     !fscrypt_has_encryption_key(old_dir)) ||
> +	    (ubifs_crypt_is_encrypted(new_dir) &&
> +	     !fscrypt_has_encryption_key(new_dir)))
> +		return -ENOKEY;
> +
>  	if (old_dir != new_dir) {
>  		if (ubifs_crypt_is_encrypted(new_dir) &&
>  		    !fscrypt_has_permitted_context(new_dir, old_inode))
> @@ -1564,6 +1570,12 @@ static int ubifs_xrename(struct inode *old_dir, struct dentry *old_dentry,
>  
>  	ubifs_assert(fst_inode && snd_inode);
>  
> +	if ((ubifs_crypt_is_encrypted(old_dir) &&
> +	     !fscrypt_has_encryption_key(old_dir)) ||
> +	    (ubifs_crypt_is_encrypted(new_dir) &&
> +	     !fscrypt_has_encryption_key(new_dir)))
> +		return -ENOKEY;
> +
>  	if ((ubifs_crypt_is_encrypted(old_dir) ||
>  	    ubifs_crypt_is_encrypted(new_dir)) &&
>  	    (old_dir != new_dir) &&
> -- 

Did you test that this change actually does anything?  Unlike ext4 and f2fs,
ubifs calls fscrypt_setup_filename() from its rename methods rather than through
a helper function ${FS}_find_entry().  Therefore it's able to pass in lookup=0,
which means that the key is required.  So it should already be failing with
ENOKEY.  You can verify this by running xfstests generic/419.

- Eric

Re: [PATCH] ubifs: Return -ENOKEY from rename if encryption keys are missing

[David Oberhollenzer]

On 04/25/2017 07:54 PM, Eric Biggers wrote:
> Did you test that this change actually does anything?  Unlike ext4 and f2fs,
> ubifs calls fscrypt_setup_filename() from its rename methods rather than through
> a helper function ${FS}_find_entry().  Therefore it's able to pass in lookup=0,
> which means that the key is required.  So it should already be failing with
> ENOKEY.  You can verify this by running xfstests generic/419.

Actually, running xfstests was how this cropped up in the first place.

The UBIFS rename and xrename functions allready call
fscrypt_setup_filename with lookup=0, however there are other tests
before that call and moving them around causes generic/419 to fail
at a different place where EPERM was expected.

Therefore I concluded that the safest way to fix this might be to
simply copy the way the checks are handled in ext4.

With recent xfstests + UBIFS support patch, after applying this patch,
generic/419 passes.


David

Re: [PATCH] ubifs: Return -ENOKEY from rename if encryption keys are missing

[Eric Biggers]

Hi David,

On Wed, Apr 26, 2017 at 01:48:10PM +0200, David Oberhollenzer wrote:
> On 04/25/2017 07:54 PM, Eric Biggers wrote:
> > Did you test that this change actually does anything?  Unlike ext4 and f2fs,
> > ubifs calls fscrypt_setup_filename() from its rename methods rather than through
> > a helper function ${FS}_find_entry().  Therefore it's able to pass in lookup=0,
> > which means that the key is required.  So it should already be failing with
> > ENOKEY.  You can verify this by running xfstests generic/419.
> 
> Actually, running xfstests was how this cropped up in the first place.
> 
> The UBIFS rename and xrename functions allready call
> fscrypt_setup_filename with lookup=0, however there are other tests
> before that call and moving them around causes generic/419 to fail
> at a different place where EPERM was expected.
> 
> Therefore I concluded that the safest way to fix this might be to
> simply copy the way the checks are handled in ext4.
> 
> With recent xfstests + UBIFS support patch, after applying this patch,
> generic/419 passes.

Are you sure?  I just tried rebasing my ubifs support patches for xfstests and
xfstests-bld onto the latest xfstests and xfstests-bld respectively, then
building a new kvm-xfstests appliance and the latest kernel from Linus's tree.
These are the failures I see with ubifs in the "encrypt" group:

$ kvm-xfstests -c ubifs -g encrypt
 [15:39:00] - output mismatch (see /results/ubifs/results-default/generic/397.out.bad)
    --- tests/generic/397.out	2017-04-26 14:37:27.000000000 -0700
    +++ /results/ubifs/results-default/generic/397.out.bad	2017-04-26 15:39:00.807418574 -0700
    @@ -10,4 +10,12 @@
     mkdir: cannot create directory 'SCRATCH_MNT/edir/0123456789abcdef': Required key not available
     ln: failed to create symbolic link 'SCRATCH_MNT/edir/newlink': Required key not available
     ln: failed to create symbolic link 'SCRATCH_MNT/edir/0123456789abcdef': Required key not available
    -stat: cannot stat 'SCRATCH_MNT/edir': No such file or directory
    +rm: cannot remove '/vdc/edir': Directory not empty
    +  File: 'SCRATCH_MNT/edir'
    +  Size: 632       	Blocks: 0          IO Block: 4096   directory
    ...
    (Run 'diff -u tests/generic/397.out /results/ubifs/results-default/generic/397.out.bad'  to see the entire diff)
...
 [15:39:03] - output mismatch (see /results/ubifs/results-default/generic/398.out.bad)
    --- tests/generic/398.out	2017-04-26 14:37:24.000000000 -0700
    +++ /results/ubifs/results-default/generic/398.out.bad	2017-04-26 15:39:03.114085240 -0700
    @@ -42,4 +42,4 @@
     Required key not available
     
     *** Exchange encrypted <=> unencrypted without key ***
    -Required key not available
    +Operation not permitted
    ...
    (Run 'diff -u tests/generic/398.out /results/ubifs/results-default/generic/398.out.bad'  to see the entire diff)


generic/397 is failing (I don't know why), and generic/398 is failing.  But
generic/419 is *not* failing.

What's happening with generic/398 is that it's trying cross-rename to exchange
an unencrypted file with an encrypted one.  The tests expects ENOKEY, but there
are actually two separate reasons why this operation is expected to fail:

(1) It's trying to link a file into an encrypted directory with the directory's
    key being available (ENOKEY)
(2) It's trying to place an unencrypted file into an encrypted directory, which
    violates the policy that all files in an encrypted directory have the same
    encryption policy (EPERM)

Personally I think that maybe the generic/398 test should just be updated to
accept either error code, given that there are two valid reasons for the
operation to fail.

- Eric

Re: [PATCH] ubifs: Return -ENOKEY from rename if encryption keys are missing

[David Oberhollenzer]

Hi Eric,

On 04/27/2017 12:52 AM, Eric Biggers wrote:
> Hi David,
> 
> On Wed, Apr 26, 2017 at 01:48:10PM +0200, David Oberhollenzer wrote:
>> On 04/25/2017 07:54 PM, Eric Biggers wrote:
>>> Did you test that this change actually does anything?  Unlike ext4 and f2fs,
>>> ubifs calls fscrypt_setup_filename() from its rename methods rather than through
>>> a helper function ${FS}_find_entry().  Therefore it's able to pass in lookup=0,
>>> which means that the key is required.  So it should already be failing with
>>> ENOKEY.  You can verify this by running xfstests generic/419.
>>
>> Actually, running xfstests was how this cropped up in the first place.
>>
>> The UBIFS rename and xrename functions allready call
>> fscrypt_setup_filename with lookup=0, however there are other tests
>> before that call and moving them around causes generic/419 to fail
>> at a different place where EPERM was expected.

Sorry, I perhaps replied a little to hastily and mixed up the
test numbers.

I just double checked and read up on the IRC backlog,
it actually _was_ 398 (see below).


> Are you sure?  I just tried rebasing my ubifs support patches for xfstests and
> xfstests-bld onto the latest xfstests and xfstests-bld respectively, then
> building a new kvm-xfstests appliance and the latest kernel from Linus's tree.

I used this kernel tree: git://git.infradead.org/linux-ubifs.git

Plus the following patches: https://lkml.org/lkml/2017/2/9/675

Using xfstests-dev: git://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git

Inside a Debian VM with scratch and test UBI volumes on nandsim.


> $ kvm-xfstests -c ubifs -g encrypt
>  [15:39:00] - output mismatch (see /results/ubifs/results-default/generic/397.out.bad)
>     --- tests/generic/397.out	2017-04-26 14:37:27.000000000 -0700
>     +++ /results/ubifs/results-default/generic/397.out.bad	2017-04-26 15:39:00.807418574 -0700
>     @@ -10,4 +10,12 @@
>      mkdir: cannot create directory 'SCRATCH_MNT/edir/0123456789abcdef': Required key not available
>      ln: failed to create symbolic link 'SCRATCH_MNT/edir/newlink': Required key not available
>      ln: failed to create symbolic link 'SCRATCH_MNT/edir/0123456789abcdef': Required key not available
>     -stat: cannot stat 'SCRATCH_MNT/edir': No such file or directory
>     +rm: cannot remove '/vdc/edir': Directory not empty
>     +  File: 'SCRATCH_MNT/edir'
>     +  Size: 632       	Blocks: 0          IO Block: 4096   directory
>     ...
>     (Run 'diff -u tests/generic/397.out /results/ubifs/results-default/generic/397.out.bad'  to see the entire diff)
> ...

This is fixed by the first patch in https://lkml.org/lkml/2017/2/9/675


> What's happening with generic/398 is that it's trying cross-rename to exchange
> an unencrypted file with an encrypted one.  The tests expects ENOKEY, but there
> are actually two separate reasons why this operation is expected to fail:
> 
> (1) It's trying to link a file into an encrypted directory with the directory's
>     key being available (ENOKEY)
> (2) It's trying to place an unencrypted file into an encrypted directory, which
>     violates the policy that all files in an encrypted directory have the same
>     encryption policy (EPERM)

Sorry again for the mix up. This is specifically what this patch is
trying to address.


> Personally I think that maybe the generic/398 test should just be updated to
> accept either error code, given that there are two valid reasons for the
> operation to fail.

But if there are different error codes with clearly outlined reasons
for returning each, wouldn't it be preferable to test that instead of
allowing an implementation to return arbitrary error codes?

To my understanding, that is what the test is trying to do there and at
least the ext4 rename and cross rename functions seem to care about
properly distinguishing between those cases.


David

Re: [PATCH] ubifs: Return -ENOKEY from rename if encryption keys are missing

[Eric Biggers]

Hi David,

On Thu, Apr 27, 2017 at 10:59:15AM +0200, David Oberhollenzer wrote:
> > Are you sure?  I just tried rebasing my ubifs support patches for xfstests and
> > xfstests-bld onto the latest xfstests and xfstests-bld respectively, then
> > building a new kvm-xfstests appliance and the latest kernel from Linus's tree.
> 
> I used this kernel tree: git://git.infradead.org/linux-ubifs.git
> 
> Plus the following patches: https://lkml.org/lkml/2017/2/9/675
> 
> Using xfstests-dev: git://git.kernel.org/pub/scm/fs/xfs/xfstests-dev.git
> 
> Inside a Debian VM with scratch and test UBI volumes on nandsim.

Just FYI, I used block2mtd in the patch I wrote for xfstests-bld (i.e.
kvm-xfstests/gce-xfstests).  I didn't consider nandsim because I wasn't aware of
it.  If you take over the patch you probably should consider both options and
choose the better one.

> > $ kvm-xfstests -c ubifs -g encrypt
> >  [15:39:00] - output mismatch (see /results/ubifs/results-default/generic/397.out.bad)
> >     --- tests/generic/397.out	2017-04-26 14:37:27.000000000 -0700
> >     +++ /results/ubifs/results-default/generic/397.out.bad	2017-04-26 15:39:00.807418574 -0700
> >     @@ -10,4 +10,12 @@
> >      mkdir: cannot create directory 'SCRATCH_MNT/edir/0123456789abcdef': Required key not available
> >      ln: failed to create symbolic link 'SCRATCH_MNT/edir/newlink': Required key not available
> >      ln: failed to create symbolic link 'SCRATCH_MNT/edir/0123456789abcdef': Required key not available
> >     -stat: cannot stat 'SCRATCH_MNT/edir': No such file or directory
> >     +rm: cannot remove '/vdc/edir': Directory not empty
> >     +  File: 'SCRATCH_MNT/edir'
> >     +  Size: 632       	Blocks: 0          IO Block: 4096   directory
> >     ...
> >     (Run 'diff -u tests/generic/397.out /results/ubifs/results-default/generic/397.out.bad'  to see the entire diff)
> > ...
> 
> This is fixed by the first patch in https://lkml.org/lkml/2017/2/9/675

Okay, great!

> 
> 
> > What's happening with generic/398 is that it's trying cross-rename to exchange
> > an unencrypted file with an encrypted one.  The tests expects ENOKEY, but there
> > are actually two separate reasons why this operation is expected to fail:
> > 
> > (1) It's trying to link a file into an encrypted directory with the directory's
> >     key being available (ENOKEY)
> > (2) It's trying to place an unencrypted file into an encrypted directory, which
> >     violates the policy that all files in an encrypted directory have the same
> >     encryption policy (EPERM)
> 
> Sorry again for the mix up. This is specifically what this patch is
> trying to address.
> 
> 
> > Personally I think that maybe the generic/398 test should just be updated to
> > accept either error code, given that there are two valid reasons for the
> > operation to fail.
> 
> But if there are different error codes with clearly outlined reasons
> for returning each, wouldn't it be preferable to test that instead of
> allowing an implementation to return arbitrary error codes?
> 
> To my understanding, that is what the test is trying to do there and at
> least the ext4 rename and cross rename functions seem to care about
> properly distinguishing between those cases.

Well, the issue is that the operation is expected to fail for two separate
reasons, each of which has its own error code.  So I'm not sure it makes sense
to enforce that filesystems prioritize one reason over the other.  The test
could accept both ENOKEY and EPERM (but not any other error) by running 'sed -e
s/Required key not available/Operation not permitted/' on the command output.
Note that the reason generic/398 tests this operation is that it was
specifically a way that ext4 and f2fs actually did, in fact, allow violating the
"all files in a directory use the same encryption policy" constraint.

Alternatively I think you could simply update UBIFS to move the
fscrypt_has_permitted_context() checks in ubifs_rename() down below the calls to
fscrypt_setup_filename().  i.e. there's no need to add a new check; the existing
ones are sufficient, they just aren't in the order the test expects.

Eric