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:   Fri, 11 Feb 2022 08:17:37 +0100
From:   Bjørn Mork <bjorn@...k.no>
To:     Alan Stern <stern@...land.harvard.edu>
Cc:     Hans Petter Selasky <hps@...asky.org>,
        Oliver Neukum <oneukum@...e.com>, linux-usb@...r.kernel.org,
        netdev@...r.kernel.org
Subject: Re: [RFC] CDC-NCM: avoid overflow in sanity checking

Alan Stern <stern@...land.harvard.edu> writes:

> First, since offset and len are initialized by converting 16- or 32-bit 
> unsigned values from little-endian to cpu-endian, they should be 
> unsigned themselves.
>
> Second, once they are unsigned there is obviously no point in testing 
> whether they are < 0.
>
> Third, if you want to make sure that skb_in's buffer contains the entire 
> interval from offset to offset + len, the proper tests are:
>
> 	if (offset <= skb_in->len && len <= skb_in->len - offset) ...
>
> The first test demonstrates that the start of the interval is in range 
> and the second test demonstrates that the end of the interval is in 
> range.  Furthermore, success of the first test proves that the 
> computation in the second test can't overflow to a negative value.

Thanks.  That detailed explanation makes perfect sense even to me.
Adding the additional offset <= skb_in->len test to Oliver's patch
is sufficient and the best solution.

Only  is that the existing code wants the inverted result:

 	if (offset > skb_in->len || len > skb_in->len - offset) ...

with all values unsigned.

> IMO, working with unsigned values is simpler than working with 
> signed values.  But it does require some discipline to ensure that 
> intermediate computations don't overflow or yield negative values.

And there you point out my problem:  discipline :-)


Bjørn