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

[Alan Stern <stern@rowland.harvard.edu>]

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