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@xxxxxxxxxxxxxxx
> > > > Signed-off-by: Aldo Ariel Panzardo <qwe.aldo@xxxxxxxxx>
> > > > ---
> > > >  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@xxxxxxxxxxx>
> > > 
> > > Thanks,
> > > Slava.
> > 
> > Acked-by: Ryusuke Konishi <konishi.ryusuke@xxxxxxxxx>
> > 
> > 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@xxxxxxxxxx # Non-fatal bug fix; defer backport
> until a real issue is reported
> 

Applied with exchanged CC.

Thanks,
Slava.