Re: [PATCH] net: usb: cdc_ncm: add ndpoffset to nframes bounds check

[Simon Horman <horms@kernel.org>]

On Thu, Feb 26, 2026 at 11:22:41PM -0800, tobgaertner wrote:
> From: Tobi Gaertner <tob.gaertner@me.com>
> 
> cdc_ncm_rx_verify_ndp16() and cdc_ncm_rx_verify_ndp32() validate that
> the NDP header and its DPE entries fit within the skb. The first check
> correctly accounts for ndpoffset:
> 
>   if ((ndpoffset + sizeof(struct usb_cdc_ncm_ndp16)) > skb_in->len)
> 
> but the second check omits it:
> 
>   if ((sizeof(struct usb_cdc_ncm_ndp16) +
>        ret * (sizeof(struct usb_cdc_ncm_dpe16))) > skb_in->len)
> 
> This validates the DPE array size against the total skb length as if
> the NDP were at offset 0, rather than at ndpoffset. When the NDP is
> placed near the end of the NTB (large wNdpIndex), the DPE entries can
> extend past the skb data buffer even though the check passes.
> cdc_ncm_rx_fixup() then reads out-of-bounds memory when iterating
> the DPE array.
> 
> Add ndpoffset to the nframes bounds check in both the NDP16 and NDP32
> verification functions.
> 
> Found by coverage-guided fuzzing with QEMU system-mode emulation.
> 
> Fixes: ff06ab13a4cc ("net: cdc_ncm: splitting rx_fixup for code reuse")

Hi Tobi,

I think the tag above covers the ndp16 case.
But that it would be appropriate to also have the following tag
for the ndp32 case.

Fixes: 0fa81b304a79 ("cdc_ncm: Implement the 32-bit version of NCM Transfer Block")

Which does make me lean towards a patchset with two patches.

> Signed-off-by: Tobi Gaertner <tob.gaertner@me.com>
> ---
>  drivers/net/usb/cdc_ncm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
> index 5d123df0a..39d2ad95d 100644
> --- a/drivers/net/usb/cdc_ncm.c
> +++ b/drivers/net/usb/cdc_ncm.c
> @@ -1675,7 +1675,7 @@ int cdc_ncm_rx_verify_ndp16(struct sk_buff *skb_in, int ndpoffset)
>  					sizeof(struct usb_cdc_ncm_dpe16));
>  	ret--; /* we process NDP entries except for the last one */
>  
> -	if ((sizeof(struct usb_cdc_ncm_ndp16) +
> +	if ((ndpoffset + sizeof(struct usb_cdc_ncm_ndp16) +
>  	     ret * (sizeof(struct usb_cdc_ncm_dpe16))) > skb_in->len) {

Perhaps out of scope for a strict bug fix. But I think this can be
expressed more succinctly using struct_size_t and fewer parentheses.
(compile tested only!)

	if (ndpoffset + struct_size_t(struct usb_cdc_ncm_ndp16,
				      dpe16, ret) > skb_in->len) {


>  		netif_dbg(dev, rx_err, dev->net, "Invalid nframes = %d\n", ret);
>  		ret = -EINVAL;
> @@ -1711,7 +1711,7 @@ int cdc_ncm_rx_verify_ndp32(struct sk_buff *skb_in, int ndpoffset)
>  					sizeof(struct usb_cdc_ncm_dpe32));
>  	ret--; /* we process NDP entries except for the last one */
>  
> -	if ((sizeof(struct usb_cdc_ncm_ndp32) +
> +	if ((ndpoffset + sizeof(struct usb_cdc_ncm_ndp32) +
>  	     ret * (sizeof(struct usb_cdc_ncm_dpe32))) > skb_in->len) {

Likewise here.

>  		netif_dbg(dev, rx_err, dev->net, "Invalid nframes = %d\n", ret);
>  		ret = -EINVAL;
> -- 
> 2.43.0
> 
>