Re: [PATCH net] net: usb: cdc_ncm: reject negative chained NDP offsets

[Bjørn Mork]

Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> writes:
> On Mon, Apr 13, 2026 at 02:11:50PM +0200, Oliver Neukum wrote:
>> On 13.04.26 12:43, Greg Kroah-Hartman wrote:
>> > On Mon, Apr 13, 2026 at 10:36:19AM +0200, Oliver Neukum wrote:
>> > > 
>> > > 
>> > > On 11.04.26 12:53, Greg Kroah-Hartman wrote:
>> > > > cdc_ncm_rx_fixup() reads dwNextNdpIndex from each NDP32 to chain to the
>> > > > next one.  The 32-bit value from the device is stored into the signed
>> > > > int ndpoffset so that means values with the high bit set become
>> > > 
>> > > Well, then isn't the problem rather that you should not store an
>> > > unsigned value in a signed variable?
>> > 
>> > No.  well, yes.  but no.
>> > 
>> > cdc_ncm_rx_verify_nth16() returns an int, and is negative if something
>> > went wrong, so we need it that way, and then we need to check it, like
>> > we properly do at the top of the loop, it's just that at the bottom of
>> > the loop we also need to do the same exact thing.
>> 
>> Doesn't that suggest that cdc_ncm_rx_verify_nth16() is the problem?
>> To be precise, the way it indicates errors?
>> As this is an offset into a buffer and the header must be at the start
>> of the buffer, isn't 0 the natural indication of an error?
>
> Maybe?  I really don't know, sorry, parsing the cdc_ncm buffer is not
> something I looked too deeply into :)

Oliver is correct AFAICS. These functions could use 0 to indicate
errors.  This would make the code simpler and cleaner.

The negative error return is just a sloppy choice I made at a time we
only supported the 16bit versions.  Didn't anticipate 32bit support
since it is optional and pointless.  But as usual, hardware vendors do
surprising things.

Note that cdc_mbim.c must be updated if cdc_ncm_rx_verify_nth16() is
changed.


Bjørn