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

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

[Greg Kroah-Hartman]

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
negative.  The first time this is read, the value is properly tested for
a negative value BUT the next time through the loop, this type of check
is missed entirely.

Fix this up by checking for a negative value when dwNextNdpIndex is read
again in the bottom of the loop to match the top check.

Commit 8d2b1a1ec9f5 ("CDC-NCM: avoid overflow in sanity checking") fixed
a similar signed-overflow issue in the datagram offset checks of the
same function.

Cc: Oliver Neukum <oliver@neukum.org>
Cc: Andrew Lunn <andrew+netdev@lunn.ch>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Jakub Kicinski <kuba@kernel.org>
Cc: Paolo Abeni <pabeni@redhat.com>
Fixes: 0fa81b304a79 ("cdc_ncm: Implement the 32-bit version of NCM Transfer Block")
Cc: stable <stable@kernel.org>
Assisted-by: gregkh_clanker_t1000
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/net/usb/cdc_ncm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index bb9929727eb9..b2d08c4aae54 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -1835,7 +1835,7 @@ int cdc_ncm_rx_fixup(struct usbnet *dev, struct sk_buff *skb_in)
 	else
 		ndpoffset = le32_to_cpu(ndp.ndp32->dwNextNdpIndex);
 
-	if (ndpoffset && loopcount--)
+	if (ndpoffset > 0 && loopcount--)
 		goto next_ndp;
 
 	/* update stats */
-- 
2.53.0

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

[Oliver Neukum]

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?

	Regards
		Oliver

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

[Greg Kroah-Hartman]

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.

So I really think this patch is the correct thing to do unless you want
to add another temp variable here just for the sign -> unsigned
transition and but that might be even messier.

thanks,

greg k-h

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

[Oliver Neukum]

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?

	Regards
		Oliver
  

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

[Greg Kroah-Hartman]

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 :)

greg k-h

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

[Bjørn Mork]

Greg Kroah-Hartman <gregkh@linuxfoundation.org> 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

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

[Greg Kroah-Hartman]

On Mon, Apr 13, 2026 at 06:20:40PM +0200, Bjørn Mork wrote:
> Greg Kroah-Hartman <gregkh@linuxfoundation.org> 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.

Ok thanks for the background, I'll rework this after the merge window is
over.

greg k-h