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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 10 Feb 2022 23:50:20 +0100
From:   Hans Petter Selasky <hps@...asky.org>
To:     Bjørn Mork <bjorn@...k.no>
Cc:     Oliver Neukum <oneukum@...e.com>, linux-usb@...r.kernel.org,
        netdev@...r.kernel.org
Subject: Re: [RFC] CDC-NCM: avoid overflow in sanity checking

On 2/10/22 18:27, Bjørn Mork wrote:
> Hans Petter Selasky <hps@...asky.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@...kerdom.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