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

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

[tobgaertner]

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")
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) {
 		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) {
 		netif_dbg(dev, rx_err, dev->net, "Invalid nframes = %d\n", ret);
 		ret = -EINVAL;
-- 
2.43.0

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

[Simon Horman]

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