Hans Petter Selasky <hps@xxxxxxxxxxx> 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@xxxxxxxxxxxx>
> 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
