On Tue, Nov 12, 2024 at 7:56 PM Edward Adam Davis wrote: > > The i_size value of the directory "cgroup.controllers" opened by openat is 0, > which causes 0 to be returned when calculating the last valid byte in > nilfs_last_byte(), which ultimately causes kaddr to move forward by reclen > (its value is 32 in this case), which ultimately triggers the uaf when > accessing de->rec_len in nilfs_find_entry(). > > To avoid this issue, add a check for i_size in nilfs_lookup(). > > Reported-by: syzbot+96d5d14c47d97015c624@xxxxxxxxxxxxxxxxxxxxxxxxx > Closes: https://syzkaller.appspot.com/bug?extid=96d5d14c47d97015c624 > Signed-off-by: Edward Adam Davis <eadavis@xxxxxx> > --- > fs/nilfs2/namei.c | 3 +++ > 1 file changed, 3 insertions(+) Hi Edward, thanks for the debugging help and patch suggestion. But this fix is incorrect. Reproducers are not creating the situation where i_size == 0. In my debug message output inserted in the while loop of nilfs_find_entry(), i_size was a corrupted large value like this: NILFS (loop0): nilfs_find_entry: isize=422212465065984, npages=103079215104, n=0, last_byte=0, reclen=32 This is different from your debug result, because the type of i_size in the debug patch you sent to syzbot is "%u". The type of inode->i_size is "loff_t", which is "long long". Therefore, the output format specification for i_size in the debug output should be "%lld". If you look at the beginning of nilfs_find_entry(), you can see that your check is double-checked: struct nilfs_dir_entry *nilfs_find_entry(struct inode *dir, const struct qstr *qstr, struct folio **foliop) { ... unsigned long npages = dir_pages(dir); .. if (npages == 0) goto out; ... Here, dir_pages() returns 0 if i_size is 0, so it jumps to "out" and returns ERR_PTR(-ENOENT). I'm still debugging, but one problem is that the implementation of nilfs_last_byte() is incorrect. In the following part, the local variable "last_byte" is not of type "loff_t", so depending on the value, it may be truncated and return a wrong value (0 in this case): static unsigned int nilfs_last_byte(struct inode *inode, unsigned long page_nr) { unsigned int last_byte = inode->i_size; ... } If this is the only problem, the following fix will be effective. (To complete this fix, I think we need to think more carefully about whether it's okay for i_size to have any value, especially since loff_t is a signed type): diff --git a/fs/nilfs2/dir.c b/fs/nilfs2/dir.c index a8602729586a..6bc8f474a3e5 100644 --- a/fs/nilfs2/dir.c +++ b/fs/nilfs2/dir.c @@ -70,7 +70,7 @@ static inline unsigned int nilfs_chunk_size(struct inode *inode) */ static unsigned int nilfs_last_byte(struct inode *inode, unsigned long page_nr) { - unsigned int last_byte = inode->i_size; + loff_t last_byte = inode->i_size; last_byte -= page_nr << PAGE_SHIFT; if (last_byte > PAGE_SIZE) Regards, Ryusuke Konishi > > diff --git a/fs/nilfs2/namei.c b/fs/nilfs2/namei.c > index 9b108052d9f7..0b57bcd9c2c5 100644 > --- a/fs/nilfs2/namei.c > +++ b/fs/nilfs2/namei.c > @@ -60,6 +60,9 @@ nilfs_lookup(struct inode *dir, struct dentry *dentry, unsigned int flags) > if (dentry->d_name.len > NILFS_NAME_LEN) > return ERR_PTR(-ENAMETOOLONG); > > + if (!dir->i_size) > + return ERR_PTR(-EINVAL); > + > res = nilfs_inode_by_name(dir, &dentry->d_name, &ino); > if (res) { > if (res != -ENOENT) > -- > 2.43.0 >
