[PATCH] drivers: net: bsd_comp: fix integer overflow in bsd_decompress()

[PATCH] drivers: net: bsd_comp: fix integer overflow in bsd_decompress()

[Roman Smirnov]

The result of a bit shift has type int. If ibuf is greater than or
equal to 128, a sign switch will occur. After that, the higher 32
bits in accm will be set to 1.

Cast the result of the expression to unsigned long.

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

Signed-off-by: Roman Smirnov <r.smirnov@omp.ru>
---
 drivers/net/ppp/bsd_comp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ppp/bsd_comp.c b/drivers/net/ppp/bsd_comp.c
index 55954594e157..078fe8c9bee8 100644
--- a/drivers/net/ppp/bsd_comp.c
+++ b/drivers/net/ppp/bsd_comp.c
@@ -918,7 +918,7 @@ static int bsd_decompress (void *state, unsigned char *ibuf, int isize,
 	 */
 
 	bitno -= 8;
-	accm  |= *ibuf++ << bitno;
+	accm  |= (unsigned long)(*ibuf++) << bitno;
 	if (tgtbitno < bitno)
 	  {
 	    continue;
-- 
2.43.0

Re: [PATCH] drivers: net: bsd_comp: fix integer overflow in bsd_decompress()

[Sergey Shtylyov]

Hello!

   Failed to notice an issue with the subject in the internal review;
I think it should look like "net: ppp: bsd_comp: fix integer overflow
in bsd_decompress()"...

On 8/12/24 11:43 AM, Roman Smirnov wrote:

> The result of a bit shift has type int.

   So far, so good... :-)

> If ibuf is greater than or

   *ibuf maybe? :-)

> equal to 128, a sign switch will occur.

   I wonder whether you had looked at the .lsy file before writing
that...
   Actually, movzvl (%rdi),%eax is used when reading *buf, so no 
sign extension occurs at this point... it occurs when casting the 
result of shift to *unsignjed long*

> After that, the higher 32 bits in accm will be set to 1.

   Only if we have 1 in the bit 31 after shl %cl,%eax...

> Cast the result of the expression to unsigned long.

   I strongly suspect we should re-declare accm as *unsigned int*
instead...

> Found by Linux Verification Center (linuxtesting.org) with Svace.
> 
> Signed-off-by: Roman Smirnov <r.smirnov@omp.ru>
> ---
>  drivers/net/ppp/bsd_comp.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ppp/bsd_comp.c b/drivers/net/ppp/bsd_comp.c
> index 55954594e157..078fe8c9bee8 100644
> --- a/drivers/net/ppp/bsd_comp.c
> +++ b/drivers/net/ppp/bsd_comp.c
> @@ -918,7 +918,7 @@ static int bsd_decompress (void *state, unsigned char *ibuf, int isize,
>  	 */
>  
>  	bitno -= 8;
> -	accm  |= *ibuf++ << bitno;
> +	accm  |= (unsigned long)(*ibuf++) << bitno;
>  	if (tgtbitno < bitno)
>  	  {
>  	    continue;
> 

MBR, Sergey

Re: [PATCH] drivers: net: bsd_comp: fix integer overflow in bsd_decompress()

[Sergey Shtylyov]

On 8/12/24 12:41 PM, Sergey Shtylyov wrote:

[...]

> On 8/12/24 11:43 AM, Roman Smirnov wrote:
> 
>> The result of a bit shift has type int.
> 
>    So far, so good... :-)
> 
>> If ibuf is greater than or
> 
>    *ibuf maybe? :-)
> 
>> equal to 128, a sign switch will occur.
> 
>    I wonder whether you had looked at the .lsy file before writing
> that...
>    Actually, movzvl (%rdi),%eax is used when reading *buf, so no 

   It was movzbl, of course...

> sign extension occurs at this point... it occurs when casting the 
> result of shift to *unsignjed long*

   ... before ORing with accm.

[...]

MBR, Sergey