[PATCH 5.10.y] fs/jfs: cast inactags to s64 to prevent potential overflow

[PATCH 5.10.y] fs/jfs: cast inactags to s64 to prevent potential overflow

[Rand Deeb]

The expression "inactags << bmp->db_agl2size" in the function
dbFinalizeBmap() is computed using int operands. Although the
values (inactags and db_agl2size) are derived from filesystem
parameters and are usually small, there is a theoretical risk that
the shift could overflow a 32-bit int if extreme values occur.

According to the C standard, shifting a signed 32-bit int can lead
to undefined behavior if the result exceeds its range. In our
case, an overflow could miscalculate free blocks, potentially
leading to erroneous filesystem accounting.

To ensure the arithmetic is performed in 64-bit space, we cast
"inactags" to s64 before shifting. This defensive fix prevents any
risk of overflow and complies with kernel coding best practices.

Found by Linux Verification Center (linuxtesting.org) with SVACE.

Signed-off-by: Rand Deeb <rand.sec96@gmail.com>
---
 fs/jfs/jfs_dmap.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/jfs/jfs_dmap.c b/fs/jfs/jfs_dmap.c
index eedea23d70ff..3cc10f9bf9f8 100644
--- a/fs/jfs/jfs_dmap.c
+++ b/fs/jfs/jfs_dmap.c
@@ -3728,8 +3728,8 @@ void dbFinalizeBmap(struct inode *ipbmap)
 	 * system size is not a multiple of the group size).
 	 */
 	inactfree = (inactags && ag_rem) ?
-	    ((inactags - 1) << bmp->db_agl2size) + ag_rem
-	    : inactags << bmp->db_agl2size;
+	    (((s64)inactags - 1) << bmp->db_agl2size) + ag_rem
+	    : ((s64)inactags << bmp->db_agl2size);
 
 	/* determine how many free blocks are in the active
 	 * allocation groups plus the average number of free blocks
-- 
2.34.1

Re: [PATCH 5.10.y] fs/jfs: cast inactags to s64 to prevent potential overflow

[Fedor Pchelkin]

Hi,

On Wed, 19. Feb 10:25, Rand Deeb wrote:
> The expression "inactags << bmp->db_agl2size" in the function
> dbFinalizeBmap() is computed using int operands. Although the
> values (inactags and db_agl2size) are derived from filesystem
> parameters and are usually small, there is a theoretical risk that
> the shift could overflow a 32-bit int if extreme values occur.
> 
> According to the C standard, shifting a signed 32-bit int can lead
> to undefined behavior if the result exceeds its range. In our
> case, an overflow could miscalculate free blocks, potentially
> leading to erroneous filesystem accounting.
> 
> To ensure the arithmetic is performed in 64-bit space, we cast
> "inactags" to s64 before shifting. This defensive fix prevents any
> risk of overflow and complies with kernel coding best practices.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Signed-off-by: Rand Deeb <rand.sec96@gmail.com>
> ---

Why is the patch targeted only to 5.10.y? It should go to the mainline
first, no?

Please check https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html

>  fs/jfs/jfs_dmap.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/jfs/jfs_dmap.c b/fs/jfs/jfs_dmap.c
> index eedea23d70ff..3cc10f9bf9f8 100644
> --- a/fs/jfs/jfs_dmap.c
> +++ b/fs/jfs/jfs_dmap.c
> @@ -3728,8 +3728,8 @@ void dbFinalizeBmap(struct inode *ipbmap)
>  	 * system size is not a multiple of the group size).
>  	 */
>  	inactfree = (inactags && ag_rem) ?
> -	    ((inactags - 1) << bmp->db_agl2size) + ag_rem
> -	    : inactags << bmp->db_agl2size;
> +	    (((s64)inactags - 1) << bmp->db_agl2size) + ag_rem
> +	    : ((s64)inactags << bmp->db_agl2size);
>  
>  	/* determine how many free blocks are in the active
>  	 * allocation groups plus the average number of free blocks
> -- 
> 2.34.1

Re: [PATCH 5.10.y] fs/jfs: cast inactags to s64 to prevent potential overflow

[Fedor Pchelkin]

On Wed, 19. Feb 14:10, Rand Deeb wrote:
> I focused on 5.10 and added it to the subject to avoid confusion,
> since files differ across versions. But yes, all versions have the issue.
> In one of my past patches, maintainers couldn't apply it due to kernel
> version differences, which led to confusion. So I thought specifying
> the version upfront would help. My bad, I should have noted it after
> the commit message instead.

The officially supported stable kernel branches can take the patch if it
or its equivalent is already present in the mainline.
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html

> 
> I'll take this into account in future patches. Should I send another
> patch specifically for the mainline version now?

Yes. And not specifically, but deliberately (it's a requirement). The
existing problems should be fixed there at first.

Re: [PATCH 5.10.y] fs/jfs: cast inactags to s64 to prevent potential overflow

[Rand Deeb]

On Wed, Feb 19, 2025 at 2:37 PM Fedor Pchelkin <pchelkin@ispras.ru> wrote:
> Yes. And not specifically, but deliberately (it's a requirement). The
> existing problems should be fixed there at first.

Done. Thanks again for the feedback!