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

["Bjørn Mork" <bjorn@mork.no>]

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