Re: [PATCH] fs/jfs: prevent potential integer overflow in dbMapFileSizeToMapSize

[Dave Kleikamp]

On 2/25/25 1:52PM, Fedor Pchelkin wrote:
> On Tue, 25. Feb 14:41, Rand Deeb wrote:
> > In dbMapFileSizeToMapSize(), the calculation involving 'complete' and
> > 'LPERCTL' can result in a 32-bit integer overflow when handling large
> > filesystems. Specifically, multiplying 'complete' by LPERCTL * LPERCTL
> > (1,048,576) can exceed the 32-bit integer limit if 'complete' is greater
> > than approximately 2,047.
> >
> > While there is no evidence that 'complete' can exceed this threshold,
> > theoretically, this is possible. To ensure robustness and maintainability,
> > this patch casts only 'complete' to s64 (64-bit integer) before performing
> > the multiplication. This guarantees that the arithmetic is conducted in
> > 64-bit space, accommodating larger values without overflow.
> >
> > This change enhances the reliability of the JFS filesystem when managing
> > large volumes and preemptively addresses potential overflow issues.
> >
> > Found by Linux Verification Center (linuxtesting.org) with SVACE.
> >
> > Signed-off-by: Rand Deeb <rand.sec96@xxxxxxxxx>
> > ---
> >   fs/jfs/jfs_dmap.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/fs/jfs/jfs_dmap.c b/fs/jfs/jfs_dmap.c
> > index edb22cf9521a..380e73c516ee 100644
> > --- a/fs/jfs/jfs_dmap.c
> > +++ b/fs/jfs/jfs_dmap.c
> > @@ -4059,7 +4059,7 @@ s64 dbMapFileSizeToMapSize(struct inode * ipbmap)
> >   		factor =
> >   		    (i == 2) ? MAXL1PAGES : ((i == 1) ? MAXL0PAGES : 1);
> >   		complete = (u32) npages / factor;
>
> Can it really overflow the 32bit arithmetic considering how 'factor' and
> 'complete' values are calculated above?

I don't really like the cast in the calculation of complete. I think it 
may be cleaner to just change the type of complete to s64 and get rid of 
that cast. Even if it's not necessary, it would be cleaner all around.

Shaggy

> /*
>   * maximum number of map pages at each level including control pages
>   */
> #define MAXL0PAGES	(1 + LPERCTL)
> #define MAXL1PAGES	(1 + LPERCTL * MAXL0PAGES)
>
> > -		ndmaps += complete * ((i == 2) ? LPERCTL * LPERCTL :
> > +		ndmaps += (s64)complete * ((i == 2) ? LPERCTL * LPERCTL :
> >   				      ((i == 1) ? LPERCTL : 1));
> >   
> >   		/* pages in last/incomplete child */
> > --
> > 2.34.1