On 2/10/22 18:27, Bjørn Mork wrote:
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;
}
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
