[RFC] CDC-NCM: avoid overflow in sanity checking

[RFC] CDC-NCM: avoid overflow in sanity checking

[Oliver Neukum]

A broken device may give an extreme offset like 0xFFF0
and a reasonable length for a fragment. In the sanity
check as formulated now, this will create an integer
overflow, defeating the sanity check. It needs to be
rewritten as a subtraction and the variables should be
unsigned.

Signed-off-by: Oliver Neukum <oneukum@suse.com>
---
 drivers/net/usb/cdc_ncm.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index e303b522efb5..f78fccbc4b93 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -1715,10 +1715,10 @@ int cdc_ncm_rx_fixup(struct usbnet *dev, struct sk_buff *skb_in)
 {
 	struct sk_buff *skb;
 	struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0];
-	int len;
+	unsigned int len;
 	int nframes;
 	int x;
-	int offset;
+	unsigned int offset;
 	union {
 		struct usb_cdc_ncm_ndp16 *ndp16;
 		struct usb_cdc_ncm_ndp32 *ndp32;
@@ -1791,7 +1791,7 @@ int cdc_ncm_rx_fixup(struct usbnet *dev, struct sk_buff *skb_in)
 		}
 
 		/* sanity checking */
-		if (((offset + len) > skb_in->len) ||
+		if ((offset > skb_in->len - len) ||
 				(len > ctx->rx_max) || (len < ETH_HLEN)) {
 			netif_dbg(dev, rx_err, dev->net,
 				  "invalid frame detected (ignored) offset[%u]=%u, length=%u, skb=%p\n",
-- 
2.34.1

Re: [RFC] CDC-NCM: avoid overflow in sanity checking

[Hans Petter Selasky]

On 2/10/22 16:54, Oliver Neukum wrote:
> A broken device may give an extreme offset like 0xFFF0
> and a reasonable length for a fragment. In the sanity
> check as formulated now, this will create an integer
> overflow, defeating the sanity check. It needs to be
> rewritten as a subtraction and the variables should be
> unsigned.
> 

Hi Oliver,

First of all I'd like to update:

Hans Petter Selasky <hans.petter.selasky@stericsson.com>

To:

Hans Petter Selasky <hselasky@freebsd.org>

Secondly,

"int" variables are 32-bit, so 0xFFF0 won't overflow.

The initial driver code written by me did only support 16-bit lengths 
and offset. Then integer overflow is not possible.

It looks like somebody else introduced this integer overflow :-(

commit 0fa81b304a7973a499f844176ca031109487dd31
Author: Alexander Bersenev <bay@hackerdom.ru>
Date:   Fri Mar 6 01:33:16 2020 +0500

     cdc_ncm: Implement the 32-bit version of NCM Transfer Block

     The NCM specification defines two formats of transfer blocks: with 
16-bit
     fields (NTB-16) and with 32-bit fields (NTB-32). Currently only 
NTB-16 is
     implemented.

....

With NCM 32, both "len" and "offset" must be checked, because these are 
now 32-bit and stored into regular "int".

The fix you propose is not fully correct!

--HPS

> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> ---
>   drivers/net/usb/cdc_ncm.c | 6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
> index e303b522efb5..f78fccbc4b93 100644
> --- a/drivers/net/usb/cdc_ncm.c
> +++ b/drivers/net/usb/cdc_ncm.c
> @@ -1715,10 +1715,10 @@ int cdc_ncm_rx_fixup(struct usbnet *dev, struct sk_buff *skb_in)
>   {
>   	struct sk_buff *skb;
>   	struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx *)dev->data[0];
> -	int len;
> +	unsigned int len;
>   	int nframes;
>   	int x;
> -	int offset;
> +	unsigned int offset;
>   	union {
>   		struct usb_cdc_ncm_ndp16 *ndp16;
>   		struct usb_cdc_ncm_ndp32 *ndp32;
> @@ -1791,7 +1791,7 @@ int cdc_ncm_rx_fixup(struct usbnet *dev, struct sk_buff *skb_in)
>   		}
>   
>   		/* sanity checking */
> -		if (((offset + len) > skb_in->len) ||
> +		if ((offset > skb_in->len - len) ||
>   				(len > ctx->rx_max) || (len < ETH_HLEN)) {
>   			netif_dbg(dev, rx_err, dev->net,
>   				  "invalid frame detected (ignored) offset[%u]=%u, length=%u, skb=%p\n",

Re: [RFC] CDC-NCM: avoid overflow in sanity checking

[Bjørn Mork]

Hans Petter Selasky <hps@selasky.org> writes:

> "int" variables are 32-bit, so 0xFFF0 won't overflow.
>
> The initial driver code written by me did only support 16-bit lengths
> and offset. Then integer overflow is not possible.
>
> It looks like somebody else introduced this integer overflow :-(
>
> commit 0fa81b304a7973a499f844176ca031109487dd31
> Author: Alexander Bersenev <bay@hackerdom.ru>
> Date:   Fri Mar 6 01:33:16 2020 +0500
>
>     cdc_ncm: Implement the 32-bit version of NCM Transfer Block
>
>     The NCM specification defines two formats of transfer blocks: with
>     16-bit
>     fields (NTB-16) and with 32-bit fields (NTB-32). Currently only
>     NTB-16 is
>     implemented.
>
> ....
>
> With NCM 32, both "len" and "offset" must be checked, because these
> are now 32-bit and stored into regular "int".
>
> The fix you propose is not fully correct!

Yes, there is still an issue if len > skb_in->len since
(skb_in->len - len) then ends up as a very large unsigned int.

I must admit that I have some problems tweaking my mind around these
subtle unsigned overflow thingies.  Which is why I suggest just
simplifying the whole thing with an additional test for the 32bit case
(which never will be used for any sane device):

		} else {
			offset = le32_to_cpu(dpe.dpe32->dwDatagramIndex);
			len = le32_to_cpu(dpe.dpe32->dwDatagramLength);
                        if (offset < 0 || len < 0)
                                goto err_ndp;
		}

And just keep the signed integers as-is.  You cannot possible use all
bits of these anyway.

Yes, OK, that won't prevent offset +  len from becoming negative, but
if will still work when compared to the unsigned skb_in->len.



Bjørn

Re: [RFC] CDC-NCM: avoid overflow in sanity checking

[Hans Petter Selasky]

On 2/10/22 18:27, Bjørn Mork wrote:
> Hans Petter Selasky <hps@selasky.org> writes:
> 
>> "int" variables are 32-bit, so 0xFFF0 won't overflow.
>>
>> The initial driver code written by me did only support 16-bit lengths
>> and offset. Then integer overflow is not possible.
>>
>> It looks like somebody else introduced this integer overflow :-(
>>
>> commit 0fa81b304a7973a499f844176ca031109487dd31
>> Author: Alexander Bersenev <bay@hackerdom.ru>
>> Date:   Fri Mar 6 01:33:16 2020 +0500
>>
>>      cdc_ncm: Implement the 32-bit version of NCM Transfer Block
>>
>>      The NCM specification defines two formats of transfer blocks: with
>>      16-bit
>>      fields (NTB-16) and with 32-bit fields (NTB-32). Currently only
>>      NTB-16 is
>>      implemented.
>>
>> ....
>>
>> With NCM 32, both "len" and "offset" must be checked, because these
>> are now 32-bit and stored into regular "int".
>>
>> The fix you propose is not fully correct!
> 
> Yes, there is still an issue if len > skb_in->len since
> (skb_in->len - len) then ends up as a very large unsigned int.
> 
> I must admit that I have some problems tweaking my mind around these
> subtle unsigned overflow thingies.  Which is why I suggest just
> simplifying the whole thing with an additional test for the 32bit case
> (which never will be used for any sane device):
> 
> 		} else {
> 			offset = le32_to_cpu(dpe.dpe32->dwDatagramIndex);
> 			len = le32_to_cpu(dpe.dpe32->dwDatagramLength);
>                          if (offset < 0 || len < 0)
>                                  goto err_ndp;
> 		}

Hi,

I think something like this would do the trick:

if (offset < 0 || offset > sbk_in->len ||
     len < 0 || len > sbk_in->len)

> 
> And just keep the signed integers as-is.  You cannot possible use all
> bits of these anyway.

Right.

> 
> Yes, OK, that won't prevent offset +  len from becoming negative, but
> if will still work when compared to the unsigned skb_in->len.
>

--HPS

Re: [RFC] CDC-NCM: avoid overflow in sanity checking

[Alan Stern]

On Thu, Feb 10, 2022 at 11:50:20PM +0100, Hans Petter Selasky wrote:
> On 2/10/22 18:27, Bjørn Mork wrote:
> > Hans Petter Selasky <hps@selasky.org> writes:
> > 
> > > "int" variables are 32-bit, so 0xFFF0 won't overflow.
> > > 
> > > The initial driver code written by me did only support 16-bit lengths
> > > and offset. Then integer overflow is not possible.
> > > 
> > > It looks like somebody else introduced this integer overflow :-(
> > > 
> > > commit 0fa81b304a7973a499f844176ca031109487dd31
> > > Author: Alexander Bersenev <bay@hackerdom.ru>
> > > Date:   Fri Mar 6 01:33:16 2020 +0500
> > > 
> > >      cdc_ncm: Implement the 32-bit version of NCM Transfer Block
> > > 
> > >      The NCM specification defines two formats of transfer blocks: with
> > >      16-bit
> > >      fields (NTB-16) and with 32-bit fields (NTB-32). Currently only
> > >      NTB-16 is
> > >      implemented.
> > > 
> > > ....
> > > 
> > > With NCM 32, both "len" and "offset" must be checked, because these
> > > are now 32-bit and stored into regular "int".
> > > 
> > > The fix you propose is not fully correct!
> > 
> > Yes, there is still an issue if len > skb_in->len since
> > (skb_in->len - len) then ends up as a very large unsigned int.
> > 
> > I must admit that I have some problems tweaking my mind around these
> > subtle unsigned overflow thingies.  Which is why I suggest just
> > simplifying the whole thing with an additional test for the 32bit case
> > (which never will be used for any sane device):
> > 
> > 		} else {
> > 			offset = le32_to_cpu(dpe.dpe32->dwDatagramIndex);
> > 			len = le32_to_cpu(dpe.dpe32->dwDatagramLength);
> >                          if (offset < 0 || len < 0)
> >                                  goto err_ndp;
> > 		}
> 
> Hi,
> 
> I think something like this would do the trick:
> 
> if (offset < 0 || offset > sbk_in->len ||
>     len < 0 || len > sbk_in->len)

Speaking as someone who is entirely unfamiliar with this code, a few
things seem clear.

First, since offset and len are initialized by converting 16- or 32-bit 
unsigned values from little-endian to cpu-endian, they should be 
unsigned themselves.

Second, once they are unsigned there is obviously no point in testing 
whether they are < 0.

Third, if you want to make sure that skb_in's buffer contains the entire 
interval from offset to offset + len, the proper tests are:

	if (offset <= skb_in->len && len <= skb_in->len - offset) ...

The first test demonstrates that the start of the interval is in range 
and the second test demonstrates that the end of the interval is in 
range.  Furthermore, success of the first test proves that the 
computation in the second test can't overflow to a negative value.

IMO, working with unsigned values is simpler than working with 
signed values.  But it does require some discipline to ensure that 
intermediate computations don't overflow or yield negative values.

Alan Stern

Re: [RFC] CDC-NCM: avoid overflow in sanity checking

[Bjørn Mork]

Alan Stern <stern@rowland.harvard.edu> writes:

> First, since offset and len are initialized by converting 16- or 32-bit 
> unsigned values from little-endian to cpu-endian, they should be 
> unsigned themselves.
>
> Second, once they are unsigned there is obviously no point in testing 
> whether they are < 0.
>
> Third, if you want to make sure that skb_in's buffer contains the entire 
> interval from offset to offset + len, the proper tests are:
>
> 	if (offset <= skb_in->len && len <= skb_in->len - offset) ...
>
> The first test demonstrates that the start of the interval is in range 
> and the second test demonstrates that the end of the interval is in 
> range.  Furthermore, success of the first test proves that the 
> computation in the second test can't overflow to a negative value.

Thanks.  That detailed explanation makes perfect sense even to me.
Adding the additional offset <= skb_in->len test to Oliver's patch
is sufficient and the best solution.

Only  is that the existing code wants the inverted result:

 	if (offset > skb_in->len || len > skb_in->len - offset) ...

with all values unsigned.

> IMO, working with unsigned values is simpler than working with 
> signed values.  But it does require some discipline to ensure that 
> intermediate computations don't overflow or yield negative values.

And there you point out my problem:  discipline :-)


Bjørn

Re: [RFC] CDC-NCM: avoid overflow in sanity checking

[Oliver Neukum]

On 11.02.22 08:17, Bjørn Mork wrote:
> Alan Stern <stern@rowland.harvard.edu> writes:
>
>
> Only  is that the existing code wants the inverted result:
>
>  	if (offset > skb_in->len || len > skb_in->len - offset) ...
>
> with all values unsigned.

Its logic is

if (!sane(fragment))
    continue;
process(fragment);

rather than

if (sane(fragment))
    process(fragment);

A simple matter of inversion.

> And there you point out my problem:  discipline :-)
>
Do we still agree that unsigned integers are the better option?

    Regards
        Oliver

Re: [RFC] CDC-NCM: avoid overflow in sanity checking

[Bjørn Mork]

Oliver Neukum <oneukum@suse.com> writes:

> Do we still agree that unsigned integers are the better option?

Yes.  What Alan said made perfect sense.  As always.


Bjørn