diff mbox series

[net,stable,v3] net: cdc_ncm: correct overhead in delayed_ndp_size

Message ID 20210105045249.5590-1-jks@iki.fi (mailing list archive)
State Accepted
Delegated to: Netdev Maintainers
Headers show
Series [net,stable,v3] net: cdc_ncm: correct overhead in delayed_ndp_size | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net
netdev/subject_prefix success Link
netdev/cc_maintainers success CCed 6 of 6 maintainers
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 43 this patch: 43
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 20 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 35 this patch: 35
netdev/header_inline success Link
netdev/stable success Stable not CCed

Commit Message

Jouni Seppänen Jan. 5, 2021, 4:52 a.m. UTC 
From: Jouni K. Seppänen <jks@iki.fi>

Aligning to tx_ndp_modulus is not sufficient because the next align
call can be cdc_ncm_align_tail, which can add up to ctx->tx_modulus +
ctx->tx_remainder - 1 bytes. This used to lead to occasional crashes
on a Huawei 909s-120 LTE module as follows:

- the condition marked /* if there is a remaining skb [...] */ is true
  so the swaps happen
- skb_out is set from ctx->tx_curr_skb
- skb_out->len is exactly 0x3f52
- ctx->tx_curr_size is 0x4000 and delayed_ndp_size is 0xac
  (note that the sum of skb_out->len and delayed_ndp_size is 0x3ffe)
- the for loop over n is executed once
- the cdc_ncm_align_tail call marked /* align beginning of next frame */
  increases skb_out->len to 0x3f56 (the sum is now 0x4002)
- the condition marked /* check if we had enough room left [...] */ is
  false so we break out of the loop
- the condition marked /* If requested, put NDP at end of frame. */ is
  true so the NDP is written into skb_out
- now skb_out->len is 0x4002, so padding_count is minus two interpreted
  as an unsigned number, which is used as the length argument to memset,
  leading to a crash with various symptoms but usually including

> Call Trace:
>  <IRQ>
>  cdc_ncm_fill_tx_frame+0x83a/0x970 [cdc_ncm]
>  cdc_mbim_tx_fixup+0x1d9/0x240 [cdc_mbim]
>  usbnet_start_xmit+0x5d/0x720 [usbnet]

The cdc_ncm_align_tail call first aligns on a ctx->tx_modulus
boundary (adding at most ctx->tx_modulus-1 bytes), then adds
ctx->tx_remainder bytes. Alternatively, the next alignment call can
occur in cdc_ncm_ndp16 or cdc_ncm_ndp32, in which case at most
ctx->tx_ndp_modulus-1 bytes are added.

A similar problem has occurred before, and the code is nontrivial to
reason about, so add a guard before the crashing call. By that time it
is too late to prevent any memory corruption (we'll have written past
the end of the buffer already) but we can at least try to get a warning
written into an on-disk log by avoiding the hard crash caused by padding
past the buffer with a huge number of zeros.

Signed-off-by: Jouni K. Seppänen <jks@iki.fi>
Fixes: 4a0e3e989d66 ("cdc_ncm: Add support for moving NDP to end of NCM frame")
Link: https://bugzilla.kernel.org/show_bug.cgi?id=209407
Reported-by: kernel test robot <lkp@intel.com>
Reviewed-by: Bjørn Mork <bjorn@mork.no>
---
v2: cast arguments to max() to same type, because otherwise integer
    promotion can result in different types
v3: use max_t

 drivers/net/usb/cdc_ncm.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

Comments

David Miller Jan. 6, 2021, 12:52 a.m. UTC | #1 
From: Jouni Seppänen <jks@iki.fi>
Date: Tue,  5 Jan 2021 06:52:49 +0200

> From: Jouni K. Seppänen <jks@iki.fi>
> 
> Aligning to tx_ndp_modulus is not sufficient because the next align
> call can be cdc_ncm_align_tail, which can add up to ctx->tx_modulus +
> ctx->tx_remainder - 1 bytes. This used to lead to occasional crashes
> on a Huawei 909s-120 LTE module as follows:
> 
> - the condition marked /* if there is a remaining skb [...] */ is true
>   so the swaps happen
> - skb_out is set from ctx->tx_curr_skb
> - skb_out->len is exactly 0x3f52
> - ctx->tx_curr_size is 0x4000 and delayed_ndp_size is 0xac
>   (note that the sum of skb_out->len and delayed_ndp_size is 0x3ffe)
> - the for loop over n is executed once
> - the cdc_ncm_align_tail call marked /* align beginning of next frame */
>   increases skb_out->len to 0x3f56 (the sum is now 0x4002)
> - the condition marked /* check if we had enough room left [...] */ is
>   false so we break out of the loop
> - the condition marked /* If requested, put NDP at end of frame. */ is
>   true so the NDP is written into skb_out
> - now skb_out->len is 0x4002, so padding_count is minus two interpreted
>   as an unsigned number, which is used as the length argument to memset,
>   leading to a crash with various symptoms but usually including
> 
>> Call Trace:
>>  <IRQ>
>>  cdc_ncm_fill_tx_frame+0x83a/0x970 [cdc_ncm]
>>  cdc_mbim_tx_fixup+0x1d9/0x240 [cdc_mbim]
>>  usbnet_start_xmit+0x5d/0x720 [usbnet]
> 
> The cdc_ncm_align_tail call first aligns on a ctx->tx_modulus
> boundary (adding at most ctx->tx_modulus-1 bytes), then adds
> ctx->tx_remainder bytes. Alternatively, the next alignment call can
> occur in cdc_ncm_ndp16 or cdc_ncm_ndp32, in which case at most
> ctx->tx_ndp_modulus-1 bytes are added.
> 
> A similar problem has occurred before, and the code is nontrivial to
> reason about, so add a guard before the crashing call. By that time it
> is too late to prevent any memory corruption (we'll have written past
> the end of the buffer already) but we can at least try to get a warning
> written into an on-disk log by avoiding the hard crash caused by padding
> past the buffer with a huge number of zeros.
> 
> Signed-off-by: Jouni K. Seppänen <jks@iki.fi>
> Fixes: 4a0e3e989d66 ("cdc_ncm: Add support for moving NDP to end of NCM frame")
> Link: https://bugzilla.kernel.org/show_bug.cgi?id=209407
> Reported-by: kernel test robot <lkp@intel.com>
> Reviewed-by: Bjørn Mork <bjorn@mork.no>

Applied and queued up for -stable, thanks.
diff mbox series

Patch

diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
index e04f588538cc..d1346c50d237 100644
--- a/drivers/net/usb/cdc_ncm.c
+++ b/drivers/net/usb/cdc_ncm.c
@@ -1199,7 +1199,10 @@  cdc_ncm_fill_tx_frame(struct usbnet *dev, struct sk_buff *skb, __le32 sign)
 	 * accordingly. Otherwise, we should check here.
 	 */
 	if (ctx->drvflags & CDC_NCM_FLAG_NDP_TO_END)
-		delayed_ndp_size = ALIGN(ctx->max_ndp_size, ctx->tx_ndp_modulus);
+		delayed_ndp_size = ctx->max_ndp_size +
+			max_t(u32,
+			      ctx->tx_ndp_modulus,
+			      ctx->tx_modulus + ctx->tx_remainder) - 1;
 	else
 		delayed_ndp_size = 0;
 
@@ -1410,7 +1413,8 @@  cdc_ncm_fill_tx_frame(struct usbnet *dev, struct sk_buff *skb, __le32 sign)
 	if (!(dev->driver_info->flags & FLAG_SEND_ZLP) &&
 	    skb_out->len > ctx->min_tx_pkt) {
 		padding_count = ctx->tx_curr_size - skb_out->len;
-		skb_put_zero(skb_out, padding_count);
+		if (!WARN_ON(padding_count > ctx->tx_curr_size))
+			skb_put_zero(skb_out, padding_count);
 	} else if (skb_out->len < ctx->tx_curr_size &&
 		   (skb_out->len % dev->maxpacket) == 0) {
 		skb_put_u8(skb_out, 0);	/* force short packet */