From: Jakub Kicinski <kuba@kernel.org>
To: Lee Jones <lee.jones@linaro.org>
Cc: linux-kernel@vger.kernel.org, stable@vger.kernel.org,
Oliver Neukum <oliver@neukum.org>,
"David S. Miller" <davem@davemloft.net>,
linux-usb@vger.kernel.org, netdev@vger.kernel.org, bjorn@mork.no
Subject: Re: [PATCH 1/1] net: cdc_ncm: Allow for dwNtbOutMaxSize to be unset or zero
Date: Thu, 2 Dec 2021 17:51:34 -0800 [thread overview]
Message-ID: <20211202175134.5b463e18@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com> (raw)
In-Reply-To: <20211202143437.1411410-1-lee.jones@linaro.org>
On Thu, 2 Dec 2021 14:34:37 +0000 Lee Jones wrote:
> Currently, due to the sequential use of min_t() and clamp_t() macros,
> in cdc_ncm_check_tx_max(), if dwNtbOutMaxSize is not set, the logic
> sets tx_max to 0. This is then used to allocate the data area of the
> SKB requested later in cdc_ncm_fill_tx_frame().
>
> This does not cause an issue presently because when memory is
> allocated during initialisation phase of SKB creation, more memory
> (512b) is allocated than is required for the SKB headers alone (320b),
> leaving some space (512b - 320b = 192b) for CDC data (172b).
>
> However, if more elements (for example 3 x u64 = [24b]) were added to
> one of the SKB header structs, say 'struct skb_shared_info',
> increasing its original size (320b [320b aligned]) to something larger
> (344b [384b aligned]), then suddenly the CDC data (172b) no longer
> fits in the spare SKB data area (512b - 384b = 128b).
>
> Consequently the SKB bounds checking semantics fails and panics:
>
> skbuff: skb_over_panic: text:ffffffff830a5b5f len:184 put:172 \
> head:ffff888119227c00 data:ffff888119227c00 tail:0xb8 end:0x80 dev:<NULL>
>
> ------------[ cut here ]------------
> kernel BUG at net/core/skbuff.c:110!
> RIP: 0010:skb_panic+0x14f/0x160 net/core/skbuff.c:106
> <snip>
> Call Trace:
> <IRQ>
> skb_over_panic+0x2c/0x30 net/core/skbuff.c:115
> skb_put+0x205/0x210 net/core/skbuff.c:1877
> skb_put_zero include/linux/skbuff.h:2270 [inline]
> cdc_ncm_ndp16 drivers/net/usb/cdc_ncm.c:1116 [inline]
> cdc_ncm_fill_tx_frame+0x127f/0x3d50 drivers/net/usb/cdc_ncm.c:1293
> cdc_ncm_tx_fixup+0x98/0xf0 drivers/net/usb/cdc_ncm.c:1514
>
> By overriding the max value with the default CDC_NCM_NTB_MAX_SIZE_TX
> when not offered through the system provided params, we ensure enough
> data space is allocated to handle the CDC data, meaning no crash will
> occur.
>
> Cc: stable@vger.kernel.org
> Cc: Oliver Neukum <oliver@neukum.org>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: linux-usb@vger.kernel.org
> Cc: netdev@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Fixes: 289507d3364f9 ("net: cdc_ncm: use sysfs for rx/tx aggregation tuning")
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
CC: bjorn@mork.no
Please make sure you CC the authors of all blamed commits as they are
likely to have the most context.
> diff --git a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c
> index 24753a4da7e60..e303b522efb50 100644
> --- a/drivers/net/usb/cdc_ncm.c
> +++ b/drivers/net/usb/cdc_ncm.c
> @@ -181,6 +181,8 @@ static u32 cdc_ncm_check_tx_max(struct usbnet *dev, u32 new_tx)
> min = ctx->max_datagram_size + ctx->max_ndp_size + sizeof(struct usb_cdc_ncm_nth32);
>
> max = min_t(u32, CDC_NCM_NTB_MAX_SIZE_TX, le32_to_cpu(ctx->ncm_parm.dwNtbOutMaxSize));
> + if (max == 0)
> + max = CDC_NCM_NTB_MAX_SIZE_TX; /* dwNtbOutMaxSize not set */
>
> /* some devices set dwNtbOutMaxSize too low for the above default */
> min = min(min, max);
next prev parent reply other threads:[~2021-12-03 1:51 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-12-02 14:34 [PATCH 1/1] net: cdc_ncm: Allow for dwNtbOutMaxSize to be unset or zero Lee Jones
2021-12-03 1:51 ` Jakub Kicinski [this message]
2021-12-03 10:29 ` Bjørn Mork
2021-12-03 11:25 ` Lee Jones
2021-12-03 12:57 ` Bjørn Mork
2021-12-03 13:39 ` Lee Jones
2021-12-03 14:36 ` Bjørn Mork
2021-12-03 14:46 ` Lee Jones
2021-12-03 14:52 ` Bjørn Mork
2021-12-04 0:57 ` Jakub Kicinski
2023-05-17 13:38 ` [PATCH] net: cdc_ncm: Deal with too low values of dwNtbOutMaxSize Tudor Ambarus
2023-05-17 13:38 ` Tudor Ambarus
2023-05-18 16:42 ` Simon Horman
2023-05-19 3:10 ` patchwork-bot+netdevbpf
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20211202175134.5b463e18@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com \
--to=kuba@kernel.org \
--cc=bjorn@mork.no \
--cc=davem@davemloft.net \
--cc=lee.jones@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-usb@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=oliver@neukum.org \
--cc=stable@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).