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@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=96d5d14c47d97015c624
Signed-off-by: Edward Adam Davis <eadavis@qq.com>
---
 fs/nilfs2/namei.c | 3 +++
 1 file changed, 3 insertions(+)

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

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@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=96d5d14c47d97015c624
> Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> ---
>  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
>

On Tue, 12 Nov 2024 23:38:11 +0900, Ryusuke Konishi wrote:
> 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@syzkaller.appspotmail.com
> > Closes: https://syzkaller.appspot.com/bug?extid=96d5d14c47d97015c624
> > Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> > ---
> >  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".
Yes, you are right, I ignore the type of i_size.
> 
> 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);
Yes, now I noticed dir_pages().
>         ..
> 
>         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)
> 
I have noticed nilfs_last_byte(), I have other concerns about it, such
as the chance of last_byte overflowing when i_size is too small and page_nr
is too large, or that it will be negative after being type-adjusted to loff_t.
So, maybe following fix is more rigorous.

diff --git a/fs/nilfs2/dir.c b/fs/nilfs2/dir.c
index a8602729586a..0dbcf91538fd 100644
--- a/fs/nilfs2/dir.c
+++ b/fs/nilfs2/dir.c
@@ -70,9 +70,10 @@ 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_nr << PAGE_SHIFT)
+               last_byte -= page_nr << PAGE_SHIFT;
        if (last_byte > PAGE_SIZE)
                last_byte = PAGE_SIZE;
        return last_byte;
BR,
Edward
> 
> 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
> >

On Wed, Nov 13, 2024 at 11:28 AM Edward Adam Davis wrote:
>
> On Tue, 12 Nov 2024 23:38:11 +0900, Ryusuke Konishi wrote:
> > 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@syzkaller.appspotmail.com
> > > Closes: https://syzkaller.appspot.com/bug?extid=96d5d14c47d97015c624
> > > Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> > > ---
> > >  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".
> Yes, you are right, I ignore the type of i_size.
> >
> > 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);
> Yes, now I noticed dir_pages().
> >         ..
> >
> >         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)
> >
> I have noticed nilfs_last_byte(), I have other concerns about it, such
> as the chance of last_byte overflowing when i_size is too small and page_nr
> is too large, or that it will be negative after being type-adjusted to loff_t.
> So, maybe following fix is more rigorous.
>
> diff --git a/fs/nilfs2/dir.c b/fs/nilfs2/dir.c
> index a8602729586a..0dbcf91538fd 100644
> --- a/fs/nilfs2/dir.c
> +++ b/fs/nilfs2/dir.c
> @@ -70,9 +70,10 @@ 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_nr << PAGE_SHIFT)
> +               last_byte -= page_nr << PAGE_SHIFT;
>         if (last_byte > PAGE_SIZE)
>                 last_byte = PAGE_SIZE;
>         return last_byte;
> BR,
> Edward

nilfs_last_byte itself does not return an error and is a function that
assumes that i_size is larger than the offset calculated from page_nr,
so let's limit the modification of this function to correcting bit
loss in assignment.

If any caller is missing the necessary range check, add that check to
the caller. I will check again for omissions, but please let me know
if there are any callers that seem to have problems (I hope there
aren't any).

To extend the bits of last_byte, declare last_byte as "u64" instead of "loff_t".
In assignments, the bit pattern is maintained regardless of whether it
is signed or not, and declaring it as u64 also avoids the problem of
negative i_size here.

Comparisons between unsigned and signed integers may introduce
warnings in syntax checks at build time such as "make W=2" depending
on the environment, and may be reported by bots at a later date, so I
would like to maintain comparisons between unsigned integers.
(PAGE_SIZE is an unsigned constant)

If the problem of negative i_size is actually a problem, I think we
should add a sanity check for i_size_read(inode) < 0 to the function
that reads inodes from block devices (such as
nilfs_read_inode_common).  So, I would like to deal with that
separately.

I have already tested a change that modifies only the last_byte type
to "u64" with syzbot, but if you could proceed with creating a patch
that includes the commit log in this direction, I would like to adopt
it.

Thanks,
Ryusuke Konishi

On Wed, 13 Nov 2024 23:54:39 +0900, Ryusuke Konishi wrote:
> On Wed, Nov 13, 2024 at 11:28 AM Edward Adam Davis wrote:
> >
> > On Tue, 12 Nov 2024 23:38:11 +0900, Ryusuke Konishi wrote:
> > > 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@syzkaller.appspotmail.com
> > > > Closes: https://syzkaller.appspot.com/bug?extid=96d5d14c47d97015c624
> > > > Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> > > > ---
> > > >  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".
> > Yes, you are right, I ignore the type of i_size.
> > >
> > > 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);
> > Yes, now I noticed dir_pages().
> > >         ..
> > >
> > >         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)
> > >
> > I have noticed nilfs_last_byte(), I have other concerns about it, such
> > as the chance of last_byte overflowing when i_size is too small and page_nr
> > is too large, or that it will be negative after being type-adjusted to loff_t.
> > So, maybe following fix is more rigorous.
> >
> > diff --git a/fs/nilfs2/dir.c b/fs/nilfs2/dir.c
> > index a8602729586a..0dbcf91538fd 100644
> > --- a/fs/nilfs2/dir.c
> > +++ b/fs/nilfs2/dir.c
> > @@ -70,9 +70,10 @@ 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_nr << PAGE_SHIFT)
> > +               last_byte -= page_nr << PAGE_SHIFT;
> >         if (last_byte > PAGE_SIZE)
> >                 last_byte = PAGE_SIZE;
> >         return last_byte;
> > BR,
> > Edward
> 
> nilfs_last_byte itself does not return an error and is a function that
> assumes that i_size is larger than the offset calculated from page_nr,
> so let's limit the modification of this function to correcting bit
> loss in assignment.
> 
> If any caller is missing the necessary range check, add that check to
> the caller. I will check again for omissions, but please let me know
> if there are any callers that seem to have problems (I hope there
> aren't any).
Yes, I agree.
> 
> To extend the bits of last_byte, declare last_byte as "u64" instead of "loff_t".
> In assignments, the bit pattern is maintained regardless of whether it
> is signed or not, and declaring it as u64 also avoids the problem of
> negative i_size here.
> 
> Comparisons between unsigned and signed integers may introduce
> warnings in syntax checks at build time such as "make W=2" depending
> on the environment, and may be reported by bots at a later date, so I
> would like to maintain comparisons between unsigned integers.
> (PAGE_SIZE is an unsigned constant)
> 
> If the problem of negative i_size is actually a problem, I think we
> should add a sanity check for i_size_read(inode) < 0 to the function
> that reads inodes from block devices (such as
> nilfs_read_inode_common).  So, I would like to deal with that
> separately.
> 
> I have already tested a change that modifies only the last_byte type
> to "u64" with syzbot, but if you could proceed with creating a patch
> that includes the commit log in this direction, I would like to adopt
> it.
You are such a nice person.
If I did that, I personally feel that you would suffer a loss.
There will be another chance in the future. I look forward to the next time.

BR,
Edward

On Thu, Nov 14, 2024 at 9:01 PM Edward Adam Davis wrote:
>
> On Wed, 13 Nov 2024 23:54:39 +0900, Ryusuke Konishi wrote:
> > On Wed, Nov 13, 2024 at 11:28 AM Edward Adam Davis wrote:
> > >
> > > On Tue, 12 Nov 2024 23:38:11 +0900, Ryusuke Konishi wrote:
> > > > 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@syzkaller.appspotmail.com
> > > > > Closes: https://syzkaller.appspot.com/bug?extid=96d5d14c47d97015c624
> > > > > Signed-off-by: Edward Adam Davis <eadavis@qq.com>
> > > > > ---
> > > > >  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".
> > > Yes, you are right, I ignore the type of i_size.
> > > >
> > > > 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);
> > > Yes, now I noticed dir_pages().
> > > >         ..
> > > >
> > > >         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)
> > > >
> > > I have noticed nilfs_last_byte(), I have other concerns about it, such
> > > as the chance of last_byte overflowing when i_size is too small and page_nr
> > > is too large, or that it will be negative after being type-adjusted to loff_t.
> > > So, maybe following fix is more rigorous.
> > >
> > > diff --git a/fs/nilfs2/dir.c b/fs/nilfs2/dir.c
> > > index a8602729586a..0dbcf91538fd 100644
> > > --- a/fs/nilfs2/dir.c
> > > +++ b/fs/nilfs2/dir.c
> > > @@ -70,9 +70,10 @@ 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_nr << PAGE_SHIFT)
> > > +               last_byte -= page_nr << PAGE_SHIFT;
> > >         if (last_byte > PAGE_SIZE)
> > >                 last_byte = PAGE_SIZE;
> > >         return last_byte;
> > > BR,
> > > Edward
> >
> > nilfs_last_byte itself does not return an error and is a function that
> > assumes that i_size is larger than the offset calculated from page_nr,
> > so let's limit the modification of this function to correcting bit
> > loss in assignment.
> >
> > If any caller is missing the necessary range check, add that check to
> > the caller. I will check again for omissions, but please let me know
> > if there are any callers that seem to have problems (I hope there
> > aren't any).
> Yes, I agree.
> >
> > To extend the bits of last_byte, declare last_byte as "u64" instead of "loff_t".
> > In assignments, the bit pattern is maintained regardless of whether it
> > is signed or not, and declaring it as u64 also avoids the problem of
> > negative i_size here.
> >
> > Comparisons between unsigned and signed integers may introduce
> > warnings in syntax checks at build time such as "make W=2" depending
> > on the environment, and may be reported by bots at a later date, so I
> > would like to maintain comparisons between unsigned integers.
> > (PAGE_SIZE is an unsigned constant)
> >
> > If the problem of negative i_size is actually a problem, I think we
> > should add a sanity check for i_size_read(inode) < 0 to the function
> > that reads inodes from block devices (such as
> > nilfs_read_inode_common).  So, I would like to deal with that
> > separately.
> >
> > I have already tested a change that modifies only the last_byte type
> > to "u64" with syzbot, but if you could proceed with creating a patch
> > that includes the commit log in this direction, I would like to adopt
> > it.
> You are such a nice person.
> If I did that, I personally feel that you would suffer a loss.
> There will be another chance in the future. I look forward to the next time.
>
> BR,
> Edward

Okay, I'll handle this bug fix.
I don't mind either way, but maybe it was a superfluous suggestion. Never mind.
Well then, maybe another time.

Thanks,
Ryusuke Konishi