On Fri, 03 Dec 2021, Bjørn Mork wrote: > Hello Lee! > > Jakub Kicinski <kuba@xxxxxxxxxx> 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? :) -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
