Thank you for your contribution! Sashiko AI review found 4 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 1 · Low: 3
- [Medium] Kernel memory leak in the kunit test helper
mctp_usblib_test_rx_split_header(): the sk_buff dequeued from…
- [Low] Undisclosed unrelated fixup: this patch silently removes the
unused `size_t i;` declaration from…
- [Low] Incorrect comment and a test that does not pin the real length
boundary.
- [Low] mctp_usblib_test_rx_invalid_dmtf_id() introduces a C99 `//`
comment ("// Make packet DMTF ID invalid"), which checkpatch.pl…
--
net: mctp: usblib: Add tests of invalid headers
Two new kunit cases are added to the mctp-usblib test module: one feeds a
USB transport header whose length field is below the minimum, the other
corrupts the DMTF ID byte. Both expect mctp_usblib_rx_complete() to
return -EPROTO and to drop the pending skb.
A few questions below.
> diff --git a/drivers/net/mctp/mctp-usblib-test.c
> b/drivers/net/mctp/mctp-usblib-test.c
> index a2ca79f61910..b0266be79e94 100644
> --- a/drivers/net/mctp/mctp-usblib-test.c
> +++ b/drivers/net/mctp/mctp-usblib-test.c
> @@ -442,7 +442,6 @@ static void mctp_usblib_test_rx_header_splits(struct
> kunit *test)
> struct mctp_usblib_test_dev *dev;
> struct mctp_usblib_test_ctx *ctx;
> struct mctp_usblib_rx *rx;
> - size_t i;
[Severity: Low]
Should this removal be squashed into the previous patch of the series? The
declaration of i in mctp_usblib_test_rx_header_splits() was added by "net:
mctp: usblib: Add test for splits inside headers", and at that commit the
function never references i because the loop is written out as eight
mctp_usblib_test_rx_split_header() calls. Building that intermediate commit
with CONFIG_MCTP_TRANSPORT_USBLIB_TEST=y should therefore warn with
-Wunused-variable, and the commit message here only mentions the two new
invalid-header tests, not this fixup.
>
> ctx = mctp_usblib_test_init(test);
> rx = mctp_usblib_test_rx_init(test, true);
[Severity: Medium]
While looking at this function, does the helper it calls leak the skb data
buffer? mctp_usblib_test_rx_split_header() ends with:
skb = __skb_dequeue(&dev->rx_pkts);
KUNIT_EXPECT_NOT_NULL(test, skb);
if (skb) {
KUNIT_EXPECT_EQ(test, skb->len, len - HDR_LEN);
kfree(skb);
}
kfree() returns only the struct sk_buff to the slab; skb_release_all() never
runs, so skb->head and the shinfo dataref are never freed. The buffer comes
from mctp_usblib_rx_prepare():
len = rx->span ? roundup(MCTP_USB_1_1_PKTLEN_MAX, rx->ep_pktlen)
: MCTP_USB_1_0_XFER_SIZE;
...
skb = __netdev_alloc_skb(netdev, len, gfp);
which is roundup(8191, 512) = 8192 bytes per prepared transfer.
mctp_usblib_test_rx_header_splits() calls the helper eight times, so each run
of the case would leak around 64 KiB, and kmemleak/KASAN would attribute it
to the MCTP driver. mctp_usblib_test_rx_single() and
mctp_usblib_test_rx_pkt_span() both use kfree_skb() here. The helper came in
with the earlier "net: mctp: usblib: Add test for splits inside headers"
patch and is unchanged at the end of the series, so would a kfree_skb() fixup
there be appropriate?
> @@ -461,11 +460,79 @@ static void mctp_usblib_test_rx_header_splits(struct
> kunit *test)
> mctp_usblib_test_rx_split_header(test, 8, dev, rx);
> }
>
> +/* Test the submission of a packet with an impossibly small value in the
> + * header's length field. Values less than HDR_LEN are invalid.
> + */
[Severity: Low]
Is "Values less than HDR_LEN are invalid" accurate? HDR_LEN is
sizeof(struct mctp_usb_hdr), which is 4:
include/linux/usb/mctp-usb.h:
struct mctp_usb_hdr {
__be16 id;
__be16 len;
} __packed;
but the check being exercised in mctp_usblib_rx_complete() is:
if (pkt_len < sizeof(*hdr) + sizeof(struct mctp_hdr)) {
netdev_dbg(netdev, "rx: invalid len %d\n", pkt_len);
rc = -EPROTO;
goto err_reset;
}
so the real minimum is 8, not 4.
> +static void mctp_usblib_test_rx_short_packet(struct kunit *test)
> +{
> + struct mctp_usblib_test_dev *dev;
> + struct mctp_usblib_test_ctx *ctx;
> + struct mctp_usblib_rx *rx;
> + size_t len, buflen;
> + u8 pktbuf[12];
> + void *buf;
> + int rc;
> +
> + ctx = mctp_usblib_test_init(test);
> + rx = mctp_usblib_test_rx_init(test, true);
> + dev = ctx->dev;
> +
> + len = sizeof(pktbuf);
> + mctp_usblib_test_init_pkt(pktbuf, len, HDR_LEN - 1);
[Severity: Low]
Following on from the comment above, would pinning the actual boundary give
more coverage here? HDR_LEN - 1 is 3, which is below both bounds, so the
4..7 range stays untested — including pkt_len == 4, where a header-only
packet would leave a zero-length MCTP packet after
skb_pull(sizeof(struct mctp_usb_hdr)) in mctp_usblib_rx(). If the driver
check were ever relaxed to the weaker bound the comment describes, this case
would still see -EPROTO and pass. Would rejecting 7 and accepting 8 be a
better test, along with naming HDR_LEN + sizeof(struct mctp_hdr) in the
comment?
> +
> + buflen = 0;
> + rc = mctp_usblib_rx_prepare(dev->ndev, rx, &buf, &buflen, GFP_KERNEL);
> + KUNIT_ASSERT_EQ(test, rc, 0);
> + KUNIT_ASSERT_GE(test, buflen, len);
> +
> + memcpy(buf, pktbuf, len);
> +
> + rc = mctp_usblib_rx_complete(dev->ndev, rx, len);
> + KUNIT_EXPECT_EQ(test, rc, -EPROTO);
> + KUNIT_EXPECT_NULL(test, rx->skb);
> + KUNIT_EXPECT_EQ(test, dev->rx_pkts.qlen, 0);
> +}
[ ... ]
> + len = sizeof(pktbuf);
> + mctp_usblib_test_init_pkt(pktbuf, len, len);
> +
> + // Make packet DMTF ID invalid
[Severity: Low]
This isn't a bug, but checkpatch.pl reports "ERROR: do not use C99 //
comments" for this line in mctp_usblib_test_rx_invalid_dmtf_id(). It is the
only // comment in drivers/net/mctp/mctp-usblib-test.c, and it is still
there at the end of the series; could it be converted to /* ... */?
> + pktbuf[1] = ~pktbuf[1];
[ ... ]
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260903-dev-mctp-usb-lib-test-v2-0-313cf15e0b56%40codeconstruct.com.au
