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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <fe074446-aeca-4766-ae3a-eb34ffab867e@oracle.com>
Date: Tue, 11 Mar 2025 11:21:07 -0500
From: Dave Kleikamp <dave.kleikamp@...cle.com>
To: Fedor Pchelkin <pchelkin@...ras.ru>, Rand Deeb <rand.sec96@...il.com>
Cc: jfs-discussion@...ts.sourceforge.net, linux-kernel@...r.kernel.org,
        voskresenski.stanislav@...fident.ru, deeb.rand@...fident.ru,
        lvc-project@...uxtesting.org
Subject: Re: [PATCH] fs/jfs: prevent potential integer overflow in
 dbMapFileSizeToMapSize

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@...il.com>
>> ---
>>   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