On Fri, 03 Dec 2021, Bjørn Mork wrote:
Hello Lee!
Jakub Kicinski kuba@kernel.org writes:
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.
Just out of curiouslity: Is this a real device, or was this the result of fuzzing around?
This is the result of "fuzzing around" on qemu. :)
https://syzkaller.appspot.com/bug?extid=2c9b6751e87ab8706cb3
Not that it matters - it's obviously a bug to fix in any case. Good catch!
(We probably have many more of the same, assuming the device presents semi-sane values in the NCM parameter struct)
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);
It's been a while since I looked at this, so excuse me if I read it wrongly. But I think we need to catch more illegal/impossible values than just zero here? Any buffer size which cannot hold a single datagram is pointless.
Trying to figure out what I possible meant to do with that
min = min(min, max);
I don't think it makes any sense? Does it? The "min" value we've carefully calculated allow one max sized datagram and headers. I don't think we should ever continue with a smaller buffer than that
I was more confused with the comment you added to that code:
/* some devices set dwNtbOutMaxSize too low for the above default */ min = min(min, max);
... which looks as though it should solve the issue of an inadequate dwNtbOutMaxSize, but it almost does the opposite. I initially changed this segment to use the max() macro instead, but the subsequent clamp_t() macro simply chooses 'max' (0) value over the now sane 'min' one.
Which is why I chose
Or are there cases where this is valid?
I'm not an expert on the SKB code, but in my simple view of the world, if you wish to use a buffer for any amount of data, you should allocate space for it.
So that really should haven been catching this bug with a
max = max(min, max)
I tried this. It didn't work either.
See the subsequent clamp_t() call a few lines down.
or maybe more readable
if (max < min) max = min
What do you think?
So the data that is added to the SKB is ctx->max_ndp_size, which is allocated in cdc_ncm_init(). The code that does it looks like:
if (ctx->is_ndp16) ctx->max_ndp_size = sizeof(struct usb_cdc_ncm_ndp16) + (ctx->tx_max_datagrams + 1) * sizeof(struct usb_cdc_ncm_dpe16); else ctx->max_ndp_size = sizeof(struct usb_cdc_ncm_ndp32) + (ctx->tx_max_datagrams + 1) * sizeof(struct usb_cdc_ncm_dpe32);
So this should be the size of the allocation too, right?
Why would the platform ever need to over-ride this? The platform can't make the data area smaller since there won't be enough room. It could perhaps make it bigger, but the min_t() and clamp_t() macros will end up choosing the above allocation anyway.
This leaves me feeling a little perplexed.
If there isn't a good reason for over-riding then I could simplify cdc_ncm_check_tx_max() greatly.
What do *you* think? :)