> -----Original Message-----
> From: Eric Biggers [mailto:ebiggers@xxxxxxxxxx]
> Sent: Tuesday, May 07, 2019 11:56 PM
> To: Fang Hongjie(方洪杰)
> Cc: tytso@xxxxxxx; jaegeuk@xxxxxxxxxx; linux-fscrypt@xxxxxxxxxxxxxxx
> 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@xxxxxxxxxxxx>
>
> 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
|