| Message ID | 1557204108-29048-1-git-send-email-hongjiefang@asrmicro.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fscrypt: don't set policy for a dead directory | expand |
Hi, On Tue, May 07, 2019 at 12:41:48PM +0800, hongjiefang wrote: > if the directory had been removed, should not set policy for it. > > Signed-off-by: hongjiefang <hongjiefang@asrmicro.com> Can you explain the motivation for this change? It makes some sense, but I don't see why it's really needed. If you look at all the other IS_DEADDIR() checks in the kernel, they're not for operations on the directory inode itself, but rather for creating/finding/listing entries in the directory. I think FS_IOC_SET_ENCRYPTION_POLICY is more like the former (though it does have to check whether the directory is empty). > --- > fs/crypto/policy.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c > index bd7eaf9..82900a4 100644 > --- a/fs/crypto/policy.c > +++ b/fs/crypto/policy.c > @@ -77,6 +77,12 @@ int fscrypt_ioctl_set_policy(struct file *filp, const void __user *arg) > > inode_lock(inode); > > + /* don't set policy for a dead directory */ > + if (IS_DEADDIR(inode)) { > + ret = -ENOENT; > + goto deaddir_out; > + } > + > ret = inode->i_sb->s_cop->get_context(inode, &ctx, sizeof(ctx)); > if (ret == -ENODATA) { > if (!S_ISDIR(inode->i_mode)) > @@ -96,6 +102,7 @@ int fscrypt_ioctl_set_policy(struct file *filp, const void __user *arg) > ret = -EEXIST; > } > > +deaddir_out: > inode_unlock(inode); Call this label 'out_unlock' instead? > > mnt_drop_write_file(filp); > -- > 1.9.1 > Thanks, - Eric
> -----Original Message----- > From: Eric Biggers [mailto:ebiggers@kernel.org] > Sent: Tuesday, May 07, 2019 11:56 PM > To: Fang Hongjie(方洪杰) > Cc: tytso@mit.edu; jaegeuk@kernel.org; linux-fscrypt@vger.kernel.org > Subject: Re: [PATCH] fscrypt: don't set policy for a dead directory > > Hi, > > On Tue, May 07, 2019 at 12:41:48PM +0800, hongjiefang wrote: > > if the directory had been removed, should not set policy for it. > > > > Signed-off-by: hongjiefang <hongjiefang@asrmicro.com> > > Can you explain the motivation for this change? It makes some sense, but I > don't see why it's really needed. If you look at all the other IS_DEADDIR() > checks in the kernel, they're not for operations on the directory inode itself, > but rather for creating/finding/listing entries in the directory. I think > FS_IOC_SET_ENCRYPTION_POLICY is more like the former (though it does have to > check whether the directory is empty). I met a panic issue when run the syzkaller on kernel 4.14.81(EXT4 FBE enabled). the flow of case as follow: r0 = openat$dir(0xffffffffffffff9c, &(0x7f0000000000)='.\x00', 0x0, 0x0) mkdirat(r0, &(0x7f0000000040)='./file0\x00', 0x0) r1 = openat$dir(0xffffffffffffff9c, &(0x7f0000000140)='./file0\x00', 0x0, 0x0) unlinkat(r0, &(0x7f0000000240)='./file0\x00', 0x200) ioctl$FS_IOC_SET_ENCRYPTION_POLICY(r1, 0x800c6613, &(0x7f00000000c0) ={0x0, @aes128, 0x0, "8acc73da97d6accc"}) The file0 directory maybe removed before doing FS_IOC_SET_ENCRYPTION_POLICY. In this case, fscrypt_ioctl_set_policy()-> ext4_empty_dir() will return the " invalid size " and trigger a panic when check the i_size of inode. the panic stack as follow: PID: 2682 TASK: ffffffc087d18080 CPU: 3 COMMAND: "syz-executor" #0 [ffffffc087d26fc0] panic at ffffff90080dc04c #1 [ffffffc087d27260] ext4_handle_error at ffffff9008689b08 #2 [ffffffc087d27290] __ext4_error_inode at ffffff9008689e90 #3 [ffffffc087d273f0] ext4_empty_dir at ffffff900865b064 #4 [ffffffc087d274d0] fscrypt_ioctl_set_policy at ffffff9008565d70 #5 [ffffffc087d27630] ext4_ioctl at ffffff900863105c #6 [ffffffc087d27b00] do_vfs_ioctl at ffffff90084cc440 #7 [ffffffc087d27e80] sys_ioctl at ffffff90084cdaf0 #8 [ffffffc087d27ff0] el0_svc_naked at ffffff9008084ffc So, it need to check the directory status in the fscrypt_ioctl_set_policy(). > > > --- > > fs/crypto/policy.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c > > index bd7eaf9..82900a4 100644 > > --- a/fs/crypto/policy.c > > +++ b/fs/crypto/policy.c > > @@ -77,6 +77,12 @@ int fscrypt_ioctl_set_policy(struct file *filp, const void __user > *arg) > > > > inode_lock(inode); > > > > + /* don't set policy for a dead directory */ > > + if (IS_DEADDIR(inode)) { > > + ret = -ENOENT; > > + goto deaddir_out; > > + } > > + > > ret = inode->i_sb->s_cop->get_context(inode, &ctx, sizeof(ctx)); > > if (ret == -ENODATA) { > > if (!S_ISDIR(inode->i_mode)) > > @@ -96,6 +102,7 @@ int fscrypt_ioctl_set_policy(struct file *filp, const void __user > *arg) > > ret = -EEXIST; > > } > > > > +deaddir_out: > > inode_unlock(inode); > > Call this label 'out_unlock' instead? > > > > > mnt_drop_write_file(filp); > > -- > > 1.9.1 > > > > Thanks, > > - Eric B&R Hongjie
[+Cc linux-ext4] On Wed, May 08, 2019 at 02:11:10AM +0000, Fang Hongjie(方洪杰) wrote: > > > -----Original Message----- > > From: Eric Biggers [mailto:ebiggers@kernel.org] > > Sent: Tuesday, May 07, 2019 11:56 PM > > To: Fang Hongjie(方洪杰) > > Cc: tytso@mit.edu; jaegeuk@kernel.org; linux-fscrypt@vger.kernel.org > > Subject: Re: [PATCH] fscrypt: don't set policy for a dead directory > > > > Hi, > > > > On Tue, May 07, 2019 at 12:41:48PM +0800, hongjiefang wrote: > > > if the directory had been removed, should not set policy for it. > > > > > > Signed-off-by: hongjiefang <hongjiefang@asrmicro.com> > > > > Can you explain the motivation for this change? It makes some sense, but I > > don't see why it's really needed. If you look at all the other IS_DEADDIR() > > checks in the kernel, they're not for operations on the directory inode itself, > > but rather for creating/finding/listing entries in the directory. I think > > FS_IOC_SET_ENCRYPTION_POLICY is more like the former (though it does have to > > check whether the directory is empty). > > I met a panic issue when run the syzkaller on kernel 4.14.81(EXT4 FBE enabled). > the flow of case as follow: > r0 = openat$dir(0xffffffffffffff9c, &(0x7f0000000000)='.\x00', 0x0, 0x0) > mkdirat(r0, &(0x7f0000000040)='./file0\x00', 0x0) > r1 = openat$dir(0xffffffffffffff9c, &(0x7f0000000140)='./file0\x00', 0x0, 0x0) > unlinkat(r0, &(0x7f0000000240)='./file0\x00', 0x200) > ioctl$FS_IOC_SET_ENCRYPTION_POLICY(r1, 0x800c6613, &(0x7f00000000c0) > ={0x0, @aes128, 0x0, "8acc73da97d6accc"}) > > The file0 directory maybe removed before doing FS_IOC_SET_ENCRYPTION_POLICY. > In this case, fscrypt_ioctl_set_policy()-> ext4_empty_dir() will return the > " invalid size " and trigger a panic when check the i_size of inode. > the panic stack as follow: > PID: 2682 TASK: ffffffc087d18080 CPU: 3 COMMAND: "syz-executor" > #0 [ffffffc087d26fc0] panic at ffffff90080dc04c > #1 [ffffffc087d27260] ext4_handle_error at ffffff9008689b08 > #2 [ffffffc087d27290] __ext4_error_inode at ffffff9008689e90 > #3 [ffffffc087d273f0] ext4_empty_dir at ffffff900865b064 > #4 [ffffffc087d274d0] fscrypt_ioctl_set_policy at ffffff9008565d70 > #5 [ffffffc087d27630] ext4_ioctl at ffffff900863105c > #6 [ffffffc087d27b00] do_vfs_ioctl at ffffff90084cc440 > #7 [ffffffc087d27e80] sys_ioctl at ffffff90084cdaf0 > #8 [ffffffc087d27ff0] el0_svc_naked at ffffff9008084ffc > > So, it need to check the directory status in the fscrypt_ioctl_set_policy(). > Okay, this is a real bug, thanks for reporting this! So ext4_rmdir() sets i_size = 0, then ext4_empty_dir() reports an error because 'inode->i_size < EXT4_DIR_REC_LEN(1) + EXT4_DIR_REC_LEN(2)'. Note that it's actually an ext4 error, not necessarily a panic. But the fs may be mounted with errors=panic. This could also be fixed by updating ext4_empty_dir() to allow i_size == 0. But we might as well check IS_DEADDIR() in fscrypt_ioctl_set_policy() either way. Can you please update the commit message to describe the problem, and add: Fixes: 9bd8212f981e ("ext4 crypto: add encryption policy and password salt support") Cc: <stable@vger.kernel.org> # v4.1+ (Another comment below) > > > > > > --- > > > fs/crypto/policy.c | 7 +++++++ > > > 1 file changed, 7 insertions(+) > > > > > > diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c > > > index bd7eaf9..82900a4 100644 > > > --- a/fs/crypto/policy.c > > > +++ b/fs/crypto/policy.c > > > @@ -77,6 +77,12 @@ int fscrypt_ioctl_set_policy(struct file *filp, const void __user > > *arg) > > > > > > inode_lock(inode); > > > > > > + /* don't set policy for a dead directory */ > > > + if (IS_DEADDIR(inode)) { > > > + ret = -ENOENT; > > > + goto deaddir_out; > > > + } > > > + This seems a bit misplaced given the actual purpose of the check, and the comment doesn't help explain it. How about moving this to just before the ->empty_dir() call, so it's only done when actually setting a new policy? I think that would make it more obvious: diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c index d536889ac31bf..4941fe8471cef 100644 --- a/fs/crypto/policy.c +++ b/fs/crypto/policy.c @@ -81,6 +81,8 @@ int fscrypt_ioctl_set_policy(struct file *filp, const void __user *arg) if (ret == -ENODATA) { if (!S_ISDIR(inode->i_mode)) ret = -ENOTDIR; + else if (IS_DEADDIR(inode)) + ret = -ENOENT; else if (!inode->i_sb->s_cop->empty_dir(inode)) ret = -ENOTEMPTY; else (Then the label below wouldn't be needed, of course.) > > > ret = inode->i_sb->s_cop->get_context(inode, &ctx, sizeof(ctx)); > > > if (ret == -ENODATA) { > > > if (!S_ISDIR(inode->i_mode)) > > > @@ -96,6 +102,7 @@ int fscrypt_ioctl_set_policy(struct file *filp, const void __user > > *arg) > > > ret = -EEXIST; > > > } > > > > > > +deaddir_out: > > > inode_unlock(inode); > > > > Call this label 'out_unlock' instead? > > > > > > > > mnt_drop_write_file(filp); > > > -- Thanks, - Eric
> From: Eric Biggers [mailto:ebiggers@kernel.org] > Sent: Wednesday, May 08, 2019 11:55 AM > To: Fang Hongjie(方洪杰) > Cc: tytso@mit.edu; jaegeuk@kernel.org; linux-fscrypt@vger.kernel.org; > linux-ext4@vger.kernel.org > Subject: Re: [PATCH] fscrypt: don't set policy for a dead directory > > [+Cc linux-ext4] > > On Wed, May 08, 2019 at 02:11:10AM +0000, Fang Hongjie(方洪杰) wrote: > > > > > -----Original Message----- > > > From: Eric Biggers [mailto:ebiggers@kernel.org] > > > Sent: Tuesday, May 07, 2019 11:56 PM > > > To: Fang Hongjie(方洪杰) > > > Cc: tytso@mit.edu; jaegeuk@kernel.org; linux-fscrypt@vger.kernel.org > > > Subject: Re: [PATCH] fscrypt: don't set policy for a dead directory > > > > > > Hi, > > > > > > On Tue, May 07, 2019 at 12:41:48PM +0800, hongjiefang wrote: > > > > if the directory had been removed, should not set policy for it. > > > > > > > > Signed-off-by: hongjiefang <hongjiefang@asrmicro.com> > > > > > > Can you explain the motivation for this change? It makes some sense, but I > > > don't see why it's really needed. If you look at all the other IS_DEADDIR() > > > checks in the kernel, they're not for operations on the directory inode itself, > > > but rather for creating/finding/listing entries in the directory. I think > > > FS_IOC_SET_ENCRYPTION_POLICY is more like the former (though it does have > to > > > check whether the directory is empty). > > > > I met a panic issue when run the syzkaller on kernel 4.14.81(EXT4 FBE enabled). > > the flow of case as follow: > > r0 = openat$dir(0xffffffffffffff9c, &(0x7f0000000000)='.\x00', 0x0, 0x0) > > mkdirat(r0, &(0x7f0000000040)='./file0\x00', 0x0) > > r1 = openat$dir(0xffffffffffffff9c, &(0x7f0000000140)='./file0\x00', 0x0, 0x0) > > unlinkat(r0, &(0x7f0000000240)='./file0\x00', 0x200) > > ioctl$FS_IOC_SET_ENCRYPTION_POLICY(r1, 0x800c6613, &(0x7f00000000c0) > > ={0x0, @aes128, 0x0, "8acc73da97d6accc"}) > > > > The file0 directory maybe removed before doing > FS_IOC_SET_ENCRYPTION_POLICY. > > In this case, fscrypt_ioctl_set_policy()-> ext4_empty_dir() will return the > > " invalid size " and trigger a panic when check the i_size of inode. > > the panic stack as follow: > > PID: 2682 TASK: ffffffc087d18080 CPU: 3 COMMAND: "syz-executor" > > #0 [ffffffc087d26fc0] panic at ffffff90080dc04c > > #1 [ffffffc087d27260] ext4_handle_error at ffffff9008689b08 > > #2 [ffffffc087d27290] __ext4_error_inode at ffffff9008689e90 > > #3 [ffffffc087d273f0] ext4_empty_dir at ffffff900865b064 > > #4 [ffffffc087d274d0] fscrypt_ioctl_set_policy at ffffff9008565d70 > > #5 [ffffffc087d27630] ext4_ioctl at ffffff900863105c > > #6 [ffffffc087d27b00] do_vfs_ioctl at ffffff90084cc440 > > #7 [ffffffc087d27e80] sys_ioctl at ffffff90084cdaf0 > > #8 [ffffffc087d27ff0] el0_svc_naked at ffffff9008084ffc > > > > So, it need to check the directory status in the fscrypt_ioctl_set_policy(). > > > > Okay, this is a real bug, thanks for reporting this! So ext4_rmdir() sets > i_size = 0, then ext4_empty_dir() reports an error because 'inode->i_size < > EXT4_DIR_REC_LEN(1) + EXT4_DIR_REC_LEN(2)'. Note that it's actually an ext4 > error, not necessarily a panic. But the fs may be mounted with errors=panic. > > This could also be fixed by updating ext4_empty_dir() to allow i_size == 0. But > we might as well check IS_DEADDIR() in fscrypt_ioctl_set_policy() either way. > > Can you please update the commit message to describe the problem, and add: > > Fixes: 9bd8212f981e ("ext4 crypto: add encryption policy and password salt > support") > Cc: <stable@vger.kernel.org> # v4.1+ > Okay，I will update it. > (Another comment below) > > > > > > > > > > --- > > > > fs/crypto/policy.c | 7 +++++++ > > > > 1 file changed, 7 insertions(+) > > > > > > > > diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c > > > > index bd7eaf9..82900a4 100644 > > > > --- a/fs/crypto/policy.c > > > > +++ b/fs/crypto/policy.c > > > > @@ -77,6 +77,12 @@ int fscrypt_ioctl_set_policy(struct file *filp, const void > __user > > > *arg) > > > > > > > > inode_lock(inode); > > > > > > > > + /* don't set policy for a dead directory */ > > > > + if (IS_DEADDIR(inode)) { > > > > + ret = -ENOENT; > > > > + goto deaddir_out; > > > > + } > > > > + > > This seems a bit misplaced given the actual purpose of the check, and the > comment doesn't help explain it. How about moving this to just before the > ->empty_dir() call, so it's only done when actually setting a new policy? > I think that would make it more obvious: > > diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c > index d536889ac31bf..4941fe8471cef 100644 > --- a/fs/crypto/policy.c > +++ b/fs/crypto/policy.c > @@ -81,6 +81,8 @@ int fscrypt_ioctl_set_policy(struct file *filp, const void __user > *arg) > if (ret == -ENODATA) { > if (!S_ISDIR(inode->i_mode)) > ret = -ENOTDIR; > + else if (IS_DEADDIR(inode)) > + ret = -ENOENT; > else if (!inode->i_sb->s_cop->empty_dir(inode)) > ret = -ENOTEMPTY; > else > > (Then the label below wouldn't be needed, of course.) > Yeah, this is a more appropriate way. Thanks for the comment. > > > > ret = inode->i_sb->s_cop->get_context(inode, &ctx, sizeof(ctx)); > > > > if (ret == -ENODATA) { > > > > if (!S_ISDIR(inode->i_mode)) > > > > @@ -96,6 +102,7 @@ int fscrypt_ioctl_set_policy(struct file *filp, const void > __user > > > *arg) > > > > ret = -EEXIST; > > > > } > > > > > > > > +deaddir_out: > > > > inode_unlock(inode); > > > > > > Call this label 'out_unlock' instead? > > > > > > > > > > > mnt_drop_write_file(filp); > > > > -- > > Thanks, > > - Eric B&R Hongjie
diff --git a/fs/crypto/policy.c b/fs/crypto/policy.c index bd7eaf9..82900a4 100644 --- a/fs/crypto/policy.c +++ b/fs/crypto/policy.c @@ -77,6 +77,12 @@ int fscrypt_ioctl_set_policy(struct file *filp, const void __user *arg) inode_lock(inode); + /* don't set policy for a dead directory */ + if (IS_DEADDIR(inode)) { + ret = -ENOENT; + goto deaddir_out; + } + ret = inode->i_sb->s_cop->get_context(inode, &ctx, sizeof(ctx)); if (ret == -ENODATA) { if (!S_ISDIR(inode->i_mode)) @@ -96,6 +102,7 @@ int fscrypt_ioctl_set_policy(struct file *filp, const void __user *arg) ret = -EEXIST; } +deaddir_out: inode_unlock(inode); mnt_drop_write_file(filp);
if the directory had been removed, should not set policy for it. Signed-off-by: hongjiefang <hongjiefang@asrmicro.com> --- fs/crypto/policy.c | 7 +++++++ 1 file changed, 7 insertions(+)