Re: [PATCH] nilfs2: validate segment number in nilfs_sufile_get_suinfo()

Re: [PATCH] nilfs2: validate segment number in nilfs_sufile_get_suinfo()

[Aldo Ariel Panzardo]

Hi Slava,

Good catch, no need for a separate variable. I'll reuse nsegs in v2.

Aldo

[PATCH v2] nilfs2: validate segment number in nilfs_sufile_get_suinfo()

[Aldo Ariel Panzardo]

nilfs_sufile_get_suinfo() subtracts the caller-provided segment number
from the total number of segments without first checking its range. If
the requested number is greater than the total, the unsigned subtraction
wraps and the function may process segment numbers outside the filesystem.

Cache the total while holding the metadata semaphore and return no entries
when the starting segment number is at or beyond the end.

Fixes: 6c98cd4ecb0a ("nilfs2: segment usage file")
Cc: stable@vger.kernel.org
Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
---
 fs/nilfs2/sufile.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/fs/nilfs2/sufile.c b/fs/nilfs2/sufile.c
index eceedca026..573646c5f7 100644
--- a/fs/nilfs2/sufile.c
+++ b/fs/nilfs2/sufile.c
@@ -870,10 +870,14 @@ ssize_t nilfs_sufile_get_suinfo(struct inode *sufile, __u64 segnum, void *buf,
 
 	down_read(&NILFS_MDT(sufile)->mi_sem);
 
+	nsegs = nilfs_sufile_get_nsegments(sufile);
+	if (segnum >= nsegs) {
+		ret = 0;
+		goto out;
+	}
+
 	segusages_per_block = nilfs_sufile_segment_usages_per_block(sufile);
-	nsegs = min_t(unsigned long,
-		      nilfs_sufile_get_nsegments(sufile) - segnum,
-		      nsi);
+	nsegs = min_t(unsigned long, nsegs - segnum, nsi);
 	for (i = 0; i < nsegs; i += n, segnum += n) {
 		n = min_t(unsigned long,
 			  segusages_per_block -
-- 
2.43.0

Re: [PATCH v2] nilfs2: validate segment number in nilfs_sufile_get_suinfo()

[Viacheslav Dubeyko]

On Tue, 2026-09-15 at 16:52 -0300, Aldo Ariel Panzardo wrote:
> nilfs_sufile_get_suinfo() subtracts the caller-provided segment
> number
> from the total number of segments without first checking its range.
> If
> the requested number is greater than the total, the unsigned
> subtraction
> wraps and the function may process segment numbers outside the
> filesystem.
> 
> Cache the total while holding the metadata semaphore and return no
> entries
> when the starting segment number is at or beyond the end.
> 
> Fixes: 6c98cd4ecb0a ("nilfs2: segment usage file")
> Cc: stable@vger.kernel.org
> Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
> ---
>  fs/nilfs2/sufile.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/nilfs2/sufile.c b/fs/nilfs2/sufile.c
> index eceedca026..573646c5f7 100644
> --- a/fs/nilfs2/sufile.c
> +++ b/fs/nilfs2/sufile.c
> @@ -870,10 +870,14 @@ ssize_t nilfs_sufile_get_suinfo(struct inode
> *sufile, __u64 segnum, void *buf,
>  
>  	down_read(&NILFS_MDT(sufile)->mi_sem);
>  
> +	nsegs = nilfs_sufile_get_nsegments(sufile);
> +	if (segnum >= nsegs) {
> +		ret = 0;
> +		goto out;
> +	}
> +
>  	segusages_per_block =
> nilfs_sufile_segment_usages_per_block(sufile);
> -	nsegs = min_t(unsigned long,
> -		      nilfs_sufile_get_nsegments(sufile) - segnum,
> -		      nsi);
> +	nsegs = min_t(unsigned long, nsegs - segnum, nsi);
>  	for (i = 0; i < nsegs; i += n, segnum += n) {
>  		n = min_t(unsigned long,
>  			  segusages_per_block -

Looks good to me.

Reviewed-by: Viacheslav Dubeyko <slava@dubeyko.com>

Thanks,
Slava.

Re: [PATCH v2] nilfs2: validate segment number in nilfs_sufile_get_suinfo()

[Ryusuke Konishi]

On Thu, Sep 17, 2026 at 4:06 AM Viacheslav Dubeyko wrote:
>
> On Tue, 2026-09-15 at 16:52 -0300, Aldo Ariel Panzardo wrote:
> > nilfs_sufile_get_suinfo() subtracts the caller-provided segment
> > number
> > from the total number of segments without first checking its range.
> > If
> > the requested number is greater than the total, the unsigned
> > subtraction
> > wraps and the function may process segment numbers outside the
> > filesystem.
> >
> > Cache the total while holding the metadata semaphore and return no
> > entries
> > when the starting segment number is at or beyond the end.
> >
> > Fixes: 6c98cd4ecb0a ("nilfs2: segment usage file")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
> > ---
> >  fs/nilfs2/sufile.c | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/fs/nilfs2/sufile.c b/fs/nilfs2/sufile.c
> > index eceedca026..573646c5f7 100644
> > --- a/fs/nilfs2/sufile.c
> > +++ b/fs/nilfs2/sufile.c
> > @@ -870,10 +870,14 @@ ssize_t nilfs_sufile_get_suinfo(struct inode
> > *sufile, __u64 segnum, void *buf,
> >
> >       down_read(&NILFS_MDT(sufile)->mi_sem);
> >
> > +     nsegs = nilfs_sufile_get_nsegments(sufile);
> > +     if (segnum >= nsegs) {
> > +             ret = 0;
> > +             goto out;
> > +     }
> > +
> >       segusages_per_block =
> > nilfs_sufile_segment_usages_per_block(sufile);
> > -     nsegs = min_t(unsigned long,
> > -                   nilfs_sufile_get_nsegments(sufile) - segnum,
> > -                   nsi);
> > +     nsegs = min_t(unsigned long, nsegs - segnum, nsi);
> >       for (i = 0; i < nsegs; i += n, segnum += n) {
> >               n = min_t(unsigned long,
> >                         segusages_per_block -
>
> Looks good to me.
>
> Reviewed-by: Viacheslav Dubeyko <slava@dubeyko.com>
>
> Thanks,
> Slava.

Acked-by: Ryusuke Konishi <konishi.ryusuke@gmail.com>

Hi Viacheslav,

Please take this directly.

I also agree that this change makes sense, and my local test results
are as expected.

However, please drop the 'Cc: stable' tag since it does not cause any
fatal issues at the moment.
If a real-world problem is found or reported, I think it would be fine
to request a manual backport then.

While acquiring information on out-of-range segments due to
wrap-around is indeed possible, sufile is also a metadata management
file.
When accessing out-of-range areas, it simply hits as a hole block and
returns empty data.

Neither memory overrun in user space or kernel space, nor
out-of-bounds block device access or information leaks will occur.

Well-behaved userland tools read segment information based on the
statistics obtained via nilfs_get_sustat(), so no problem will occur
(the difference in behavior was visually indistinguishable).

Therefore, I modified the nilfs-utils to perform ill-behaved calls and
tested it.
Although out-of-range non-existent segments were displayed as empty
data, no critical issue that compromises kernel stability occurred.

At present, it does not appear to meet the basic criteria for stable
backporting, so when backporting, we need to be able to organize and
demonstrate the reasoning why it is necessary.

Thanks,
Ryusuke Konishi

Re: [PATCH v2] nilfs2: validate segment number in nilfs_sufile_get_suinfo()

[Ryusuke Konishi]

On Fri, Sep 18, 2026 at 12:46 AM Ryusuke Konishi  wrote:
>
> On Thu, Sep 17, 2026 at 4:06 AM Viacheslav Dubeyko wrote:
> >
> > On Tue, 2026-09-15 at 16:52 -0300, Aldo Ariel Panzardo wrote:
> > > nilfs_sufile_get_suinfo() subtracts the caller-provided segment
> > > number
> > > from the total number of segments without first checking its range.
> > > If
> > > the requested number is greater than the total, the unsigned
> > > subtraction
> > > wraps and the function may process segment numbers outside the
> > > filesystem.
> > >
> > > Cache the total while holding the metadata semaphore and return no
> > > entries
> > > when the starting segment number is at or beyond the end.
> > >
> > > Fixes: 6c98cd4ecb0a ("nilfs2: segment usage file")
> > > Cc: stable@vger.kernel.org
> > > Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
> > > ---
> > >  fs/nilfs2/sufile.c | 10 +++++++---
> > >  1 file changed, 7 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/fs/nilfs2/sufile.c b/fs/nilfs2/sufile.c
> > > index eceedca026..573646c5f7 100644
> > > --- a/fs/nilfs2/sufile.c
> > > +++ b/fs/nilfs2/sufile.c
> > > @@ -870,10 +870,14 @@ ssize_t nilfs_sufile_get_suinfo(struct inode
> > > *sufile, __u64 segnum, void *buf,
> > >
> > >       down_read(&NILFS_MDT(sufile)->mi_sem);
> > >
> > > +     nsegs = nilfs_sufile_get_nsegments(sufile);
> > > +     if (segnum >= nsegs) {
> > > +             ret = 0;
> > > +             goto out;
> > > +     }
> > > +
> > >       segusages_per_block =
> > > nilfs_sufile_segment_usages_per_block(sufile);
> > > -     nsegs = min_t(unsigned long,
> > > -                   nilfs_sufile_get_nsegments(sufile) - segnum,
> > > -                   nsi);
> > > +     nsegs = min_t(unsigned long, nsegs - segnum, nsi);
> > >       for (i = 0; i < nsegs; i += n, segnum += n) {
> > >               n = min_t(unsigned long,
> > >                         segusages_per_block -
> >
> > Looks good to me.
> >
> > Reviewed-by: Viacheslav Dubeyko <slava@dubeyko.com>
> >
> > Thanks,
> > Slava.
>
> Acked-by: Ryusuke Konishi <konishi.ryusuke@gmail.com>
>
> Hi Viacheslav,
>
> Please take this directly.
>
> I also agree that this change makes sense, and my local test results
> are as expected.
>
> However, please drop the 'Cc: stable' tag since it does not cause any
> fatal issues at the moment.
> If a real-world problem is found or reported, I think it would be fine
> to request a manual backport then.
>
> While acquiring information on out-of-range segments due to
> wrap-around is indeed possible, sufile is also a metadata management
> file.
> When accessing out-of-range areas, it simply hits as a hole block and
> returns empty data.
>
> Neither memory overrun in user space or kernel space, nor
> out-of-bounds block device access or information leaks will occur.
>
> Well-behaved userland tools read segment information based on the
> statistics obtained via nilfs_get_sustat(), so no problem will occur
> (the difference in behavior was visually indistinguishable).
>
> Therefore, I modified the nilfs-utils to perform ill-behaved calls and
> tested it.
> Although out-of-range non-existent segments were displayed as empty
> data, no critical issue that compromises kernel stability occurred.
>
> At present, it does not appear to meet the basic criteria for stable
> backporting, so when backporting, we need to be able to organize and
> demonstrate the reasoning why it is necessary.
>
> Thanks,
> Ryusuke Konishi

Hi Viacheslav,

To follow up on my previous email regarding the stable tag: since the
patch includes a Fixes tag, simply dropping 'Cc: stable' will not
prevent automatic pickup by AUTOSEL.

To strictly prevent automatic backporting while keeping it tracked,
please replace the tag with the following instead of dropping it
entirely:

Cc: stable+noautosel@kernel.org # Non-fatal bug fix; defer backport
until a real issue is reported

Thanks,
Ryusuke Konishi

Re: [PATCH v2] nilfs2: validate segment number in nilfs_sufile_get_suinfo()

[Viacheslav Dubeyko]

On Fri, 2026-09-18 at 01:05 +0900, Ryusuke Konishi wrote:
> On Fri, Sep 18, 2026 at 12:46 AM Ryusuke Konishi  wrote:
> > 
> > On Thu, Sep 17, 2026 at 4:06 AM Viacheslav Dubeyko wrote:
> > > 
> > > On Tue, 2026-09-15 at 16:52 -0300, Aldo Ariel Panzardo wrote:
> > > > nilfs_sufile_get_suinfo() subtracts the caller-provided segment
> > > > number
> > > > from the total number of segments without first checking its
> > > > range.
> > > > If
> > > > the requested number is greater than the total, the unsigned
> > > > subtraction
> > > > wraps and the function may process segment numbers outside the
> > > > filesystem.
> > > > 
> > > > Cache the total while holding the metadata semaphore and return
> > > > no
> > > > entries
> > > > when the starting segment number is at or beyond the end.
> > > > 
> > > > Fixes: 6c98cd4ecb0a ("nilfs2: segment usage file")
> > > > Cc: stable@vger.kernel.org
> > > > Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@gmail.com>
> > > > ---
> > > >  fs/nilfs2/sufile.c | 10 +++++++---
> > > >  1 file changed, 7 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/fs/nilfs2/sufile.c b/fs/nilfs2/sufile.c
> > > > index eceedca026..573646c5f7 100644
> > > > --- a/fs/nilfs2/sufile.c
> > > > +++ b/fs/nilfs2/sufile.c
> > > > @@ -870,10 +870,14 @@ ssize_t nilfs_sufile_get_suinfo(struct
> > > > inode
> > > > *sufile, __u64 segnum, void *buf,
> > > > 
> > > >       down_read(&NILFS_MDT(sufile)->mi_sem);
> > > > 
> > > > +     nsegs = nilfs_sufile_get_nsegments(sufile);
> > > > +     if (segnum >= nsegs) {
> > > > +             ret = 0;
> > > > +             goto out;
> > > > +     }
> > > > +
> > > >       segusages_per_block =
> > > > nilfs_sufile_segment_usages_per_block(sufile);
> > > > -     nsegs = min_t(unsigned long,
> > > > -                   nilfs_sufile_get_nsegments(sufile) -
> > > > segnum,
> > > > -                   nsi);
> > > > +     nsegs = min_t(unsigned long, nsegs - segnum, nsi);
> > > >       for (i = 0; i < nsegs; i += n, segnum += n) {
> > > >               n = min_t(unsigned long,
> > > >                         segusages_per_block -
> > > 
> > > Looks good to me.
> > > 
> > > Reviewed-by: Viacheslav Dubeyko <slava@dubeyko.com>
> > > 
> > > Thanks,
> > > Slava.
> > 
> > Acked-by: Ryusuke Konishi <konishi.ryusuke@gmail.com>
> > 
> > Hi Viacheslav,
> > 
> > Please take this directly.
> > 
> > I also agree that this change makes sense, and my local test
> > results
> > are as expected.
> > 
> > However, please drop the 'Cc: stable' tag since it does not cause
> > any
> > fatal issues at the moment.
> > If a real-world problem is found or reported, I think it would be
> > fine
> > to request a manual backport then.
> > 
> > While acquiring information on out-of-range segments due to
> > wrap-around is indeed possible, sufile is also a metadata
> > management
> > file.
> > When accessing out-of-range areas, it simply hits as a hole block
> > and
> > returns empty data.
> > 
> > Neither memory overrun in user space or kernel space, nor
> > out-of-bounds block device access or information leaks will occur.
> > 
> > Well-behaved userland tools read segment information based on the
> > statistics obtained via nilfs_get_sustat(), so no problem will
> > occur
> > (the difference in behavior was visually indistinguishable).
> > 
> > Therefore, I modified the nilfs-utils to perform ill-behaved calls
> > and
> > tested it.
> > Although out-of-range non-existent segments were displayed as empty
> > data, no critical issue that compromises kernel stability occurred.
> > 
> > At present, it does not appear to meet the basic criteria for
> > stable
> > backporting, so when backporting, we need to be able to organize
> > and
> > demonstrate the reasoning why it is necessary.
> > 
> > Thanks,
> > Ryusuke Konishi
> 
> Hi Viacheslav,
> 
> To follow up on my previous email regarding the stable tag: since the
> patch includes a Fixes tag, simply dropping 'Cc: stable' will not
> prevent automatic pickup by AUTOSEL.
> 
> To strictly prevent automatic backporting while keeping it tracked,
> please replace the tag with the following instead of dropping it
> entirely:
> 
> Cc: stable+noautosel@kernel.org # Non-fatal bug fix; defer backport
> until a real issue is reported
> 

Applied with exchanged CC.

Thanks,
Slava.