diff mbox series

ext4: add casefolded file check

Message ID 20240530074150.4192102-1-lizhi.xu@windriver.com (mailing list archive)
State Not Applicable
Headers show
Series ext4: add casefolded file check | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch

Commit Message

Lizhi Xu May 30, 2024, 7:41 a.m. UTC
The file name that needs to calculate the siphash must have both flags casefolded
and dir at the same time, so before calculating it, confirm that the flag meets
the conditions.

Reported-by: syzbot+340581ba9dceb7e06fb3@syzkaller.appspotmail.com
Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
---
 fs/ext4/hash.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Eric Biggers May 31, 2024, 1:05 a.m. UTC | #1
On Thu, May 30, 2024 at 03:41:50PM +0800, 'Lizhi Xu' via syzkaller-bugs wrote:
> The file name that needs to calculate the siphash must have both flags casefolded
> and dir at the same time, so before calculating it, confirm that the flag meets
> the conditions.
> 
> Reported-by: syzbot+340581ba9dceb7e06fb3@syzkaller.appspotmail.com
> Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
> ---
>  fs/ext4/hash.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/fs/ext4/hash.c b/fs/ext4/hash.c
> index deabe29da7fb..c8840cfc01dd 100644
> --- a/fs/ext4/hash.c
> +++ b/fs/ext4/hash.c
> @@ -265,6 +265,10 @@ static int __ext4fs_dirhash(const struct inode *dir, const char *name, int len,
>  		__u64	combined_hash;
>  
>  		if (fscrypt_has_encryption_key(dir)) {
> +			if (!IS_CASEFOLDED(dir)) {
> +				ext4_warning_inode(dir, "Siphash requires Casefolded file");
> +				return -2;
> +			}
>  			combined_hash = fscrypt_fname_siphash(dir, &qname);
>  		} else {
>  			ext4_warning_inode(dir, "Siphash requires key");

First, this needs to be sent to the ext4 mailing list (and not to irrelevant
mailing lists such as netdev).  Please use ./scripts/get_maintainer.pl, as is
recommended by Documentation/process/submitting-patches.rst.

Second, ext4 already checks for the directory being casefolded before allowing
siphash.  This is done by dx_probe().  Evidently syzbot found some way around
that, so what needs to be done is figure out why that happened and what is the
best fix to prevent it.  This is not necessarily the patch you've proposed, as
the real issue might actually be a missing check at some earlier time like when
reading the inode from disk or when mounting the filesystem.

- Eric
Lizhi Xu May 31, 2024, 1:47 a.m. UTC | #2
On Thu, 30 May 2024 18:05:13 -0700, Eric Biggers wrote:
> > The file name that needs to calculate the siphash must have both flags casefolded
> > and dir at the same time, so before calculating it, confirm that the flag meets
> > the conditions.
> >
> > Reported-by: syzbot+340581ba9dceb7e06fb3@syzkaller.appspotmail.com
> > Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
> > ---
> >  fs/ext4/hash.c | 4 ++++
> >  1 file changed, 4 insertions(+)
> >
> > diff --git a/fs/ext4/hash.c b/fs/ext4/hash.c
> > index deabe29da7fb..c8840cfc01dd 100644
> > --- a/fs/ext4/hash.c
> > +++ b/fs/ext4/hash.c
> > @@ -265,6 +265,10 @@ static int __ext4fs_dirhash(const struct inode *dir, const char *name, int len,
> >  		__u64	combined_hash;
> >
> >  		if (fscrypt_has_encryption_key(dir)) {
> > +			if (!IS_CASEFOLDED(dir)) {
> > +				ext4_warning_inode(dir, "Siphash requires Casefolded file");
> > +				return -2;
> > +			}
> >  			combined_hash = fscrypt_fname_siphash(dir, &qname);
> >  		} else {
> >  			ext4_warning_inode(dir, "Siphash requires key");
> 
> First, this needs to be sent to the ext4 mailing list (and not to irrelevant
> mailing lists such as netdev).  Please use ./scripts/get_maintainer.pl, as is
> recommended by Documentation/process/submitting-patches.rst.
> 
> Second, ext4 already checks for the directory being casefolded before allowing
> siphash.  This is done by dx_probe().  Evidently syzbot found some way around
> that, so what needs to be done is figure out why that happened and what is the
> best fix to prevent it.  This is not necessarily the patch you've proposed, as
> the real issue might actually be a missing check at some earlier time like when
> reading the inode from disk or when mounting the filesystem.
I have confirmed that there is no casefolded feature when creating the directory.
I agree with your statement that it should be checked for casefold features when
mounting or reading from disk.

Lizhi
Eric Biggers May 31, 2024, 2:20 a.m. UTC | #3
On Fri, May 31, 2024 at 09:47:47AM +0800, 'Lizhi Xu' via syzkaller-bugs wrote:
> On Thu, 30 May 2024 18:05:13 -0700, Eric Biggers wrote:
> > > The file name that needs to calculate the siphash must have both flags casefolded
> > > and dir at the same time, so before calculating it, confirm that the flag meets
> > > the conditions.
> > >
> > > Reported-by: syzbot+340581ba9dceb7e06fb3@syzkaller.appspotmail.com
> > > Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
> > > ---
> > >  fs/ext4/hash.c | 4 ++++
> > >  1 file changed, 4 insertions(+)
> > >
> > > diff --git a/fs/ext4/hash.c b/fs/ext4/hash.c
> > > index deabe29da7fb..c8840cfc01dd 100644
> > > --- a/fs/ext4/hash.c
> > > +++ b/fs/ext4/hash.c
> > > @@ -265,6 +265,10 @@ static int __ext4fs_dirhash(const struct inode *dir, const char *name, int len,
> > >  		__u64	combined_hash;
> > >
> > >  		if (fscrypt_has_encryption_key(dir)) {
> > > +			if (!IS_CASEFOLDED(dir)) {
> > > +				ext4_warning_inode(dir, "Siphash requires Casefolded file");
> > > +				return -2;
> > > +			}
> > >  			combined_hash = fscrypt_fname_siphash(dir, &qname);
> > >  		} else {
> > >  			ext4_warning_inode(dir, "Siphash requires key");
> > 
> > First, this needs to be sent to the ext4 mailing list (and not to irrelevant
> > mailing lists such as netdev).  Please use ./scripts/get_maintainer.pl, as is
> > recommended by Documentation/process/submitting-patches.rst.
> > 
> > Second, ext4 already checks for the directory being casefolded before allowing
> > siphash.  This is done by dx_probe().  Evidently syzbot found some way around
> > that, so what needs to be done is figure out why that happened and what is the
> > best fix to prevent it.  This is not necessarily the patch you've proposed, as
> > the real issue might actually be a missing check at some earlier time like when
> > reading the inode from disk or when mounting the filesystem.
> I have confirmed that there is no casefolded feature when creating the directory.
> I agree with your statement that it should be checked for casefold features when
> mounting or reading from disk.
> 

I haven't looked at the syzbot reproducer, but I'm guessing that the
DX_HASH_SIPHASH is coming from s_def_hash_version in the filesystem superblock.
It's not valid to have DX_HASH_SIPHASH there, and it probably would make more
sense to validate that at mount time.

- Eric
Lizhi Xu May 31, 2024, 3:30 a.m. UTC | #4
On Thu, 30 May 2024 20:11:33 -0700, Eric Biggers wrote:
> > Due to the current file system not supporting the casefolded feature, only 
> > i_crypt_info was initialized when creating encrypted information, without actually
> > setting the sighash. Therefore, when creating an inode, if the system does not 
> > support the casefolded feature, encrypted information will not be created.
> > 
> > Reported-by: syzbot+340581ba9dceb7e06fb3@syzkaller.appspotmail.com
> > Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
> > ---
> >  fs/ext4/ialloc.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
> > index e9bbb1da2d0a..47b75589fdf4 100644
> > --- a/fs/ext4/ialloc.c
> > +++ b/fs/ext4/ialloc.c
> > @@ -983,7 +983,8 @@ struct inode *__ext4_new_inode(struct mnt_idmap *idmap,
> >  		ei->i_projid = make_kprojid(&init_user_ns, EXT4_DEF_PROJID);
> >  
> >  	if (!(i_flags & EXT4_EA_INODE_FL)) {
> > -		err = fscrypt_prepare_new_inode(dir, inode, &encrypt);
> > +		if (ext4_has_feature_casefold(inode->i_sb))
> > +			err = fscrypt_prepare_new_inode(dir, inode, &encrypt);
> >  		if (err)
> >  			goto out;
> 
> No, this is not correct at all.  This just disables encryption on filesystems
> with the casefold feature.
If filesystems not support casefold feature, Why do I need to setup encrypted
information when creating a directory? Can encrypted information not include *hash?
> 
> As I said before, please also use the correct mailing lists.
Added.

Lizhi
Eric Biggers May 31, 2024, 3:34 a.m. UTC | #5
On Fri, May 31, 2024 at 11:30:44AM +0800, 'Lizhi Xu' via syzkaller-bugs wrote:
> On Thu, 30 May 2024 20:11:33 -0700, Eric Biggers wrote:
> > > Due to the current file system not supporting the casefolded feature, only 
> > > i_crypt_info was initialized when creating encrypted information, without actually
> > > setting the sighash. Therefore, when creating an inode, if the system does not 
> > > support the casefolded feature, encrypted information will not be created.
> > > 
> > > Reported-by: syzbot+340581ba9dceb7e06fb3@syzkaller.appspotmail.com
> > > Signed-off-by: Lizhi Xu <lizhi.xu@windriver.com>
> > > ---
> > >  fs/ext4/ialloc.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/fs/ext4/ialloc.c b/fs/ext4/ialloc.c
> > > index e9bbb1da2d0a..47b75589fdf4 100644
> > > --- a/fs/ext4/ialloc.c
> > > +++ b/fs/ext4/ialloc.c
> > > @@ -983,7 +983,8 @@ struct inode *__ext4_new_inode(struct mnt_idmap *idmap,
> > >  		ei->i_projid = make_kprojid(&init_user_ns, EXT4_DEF_PROJID);
> > >  
> > >  	if (!(i_flags & EXT4_EA_INODE_FL)) {
> > > -		err = fscrypt_prepare_new_inode(dir, inode, &encrypt);
> > > +		if (ext4_has_feature_casefold(inode->i_sb))
> > > +			err = fscrypt_prepare_new_inode(dir, inode, &encrypt);
> > >  		if (err)
> > >  			goto out;
> > 
> > No, this is not correct at all.  This just disables encryption on filesystems
> > with the casefold feature.
> If filesystems not support casefold feature, Why do I need to setup encrypted
> information when creating a directory? Can encrypted information not include *hash?

Encryption is a separate feature.  It is supported both with and without
casefold.

- Eric
diff mbox series

Patch

diff --git a/fs/ext4/hash.c b/fs/ext4/hash.c
index deabe29da7fb..c8840cfc01dd 100644
--- a/fs/ext4/hash.c
+++ b/fs/ext4/hash.c
@@ -265,6 +265,10 @@  static int __ext4fs_dirhash(const struct inode *dir, const char *name, int len,
 		__u64	combined_hash;
 
 		if (fscrypt_has_encryption_key(dir)) {
+			if (!IS_CASEFOLDED(dir)) {
+				ext4_warning_inode(dir, "Siphash requires Casefolded file");
+				return -2;
+			}
 			combined_hash = fscrypt_fname_siphash(dir, &qname);
 		} else {
 			ext4_warning_inode(dir, "Siphash requires key");