[lkml]   [2021]   [Jan]   [3]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
Messages in this thread
    Patch in this message
    Subject[PATCH net,stable v2] net: cdc_ncm: correct overhead in delayed_ndp_size
    From: Jouni K. Seppänen <>

    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 <>
    Fixes: 4a0e3e989d66 ("cdc_ncm: Add support for moving NDP to end of NCM frame")
    Reported-by: kernel test robot <>
    v2: cast arguments to max() to the same type because integer promotion can
    result in different types; reported by the kernel test robot

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

    diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
    index e04f588538cc..4d8a1f50190c 100644
    --- a/drivers/net/usb/cdc_ncm.c
    +++ b/drivers/net/usb/cdc_ncm.c
    @@ -1199,7 +1199,9 @@ 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((u32)ctx->tx_ndp_modulus,
    + (u32)ctx->tx_modulus + ctx->tx_remainder) - 1;
    delayed_ndp_size = 0;

    @@ -1410,7 +1412,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 */
     \ /
      Last update: 2021-01-03 21:25    [W:3.094 / U:0.008 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site