| Message ID | 20240530074150.4192102-1-lizhi.xu@windriver.com (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | ext4: add casefolded file check | expand |
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
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
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
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
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 --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");
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(+)