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]

Message-ID: <87v8xmocng.fsf@miraculix.mork.no>
Date:   Thu, 10 Feb 2022 18:27:15 +0100
From:   Bjørn Mork <bjorn@...k.no>
To:     Hans Petter Selasky <hps@...asky.org>
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

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;
		}

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