netdev - possible integer overflow in CDC-NCM checks

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <68332064-3c38-fe81-b659-613940a6cfb1@suse.com>
Date:   Thu, 10 Feb 2022 16:23:58 +0100
From:   Oliver Neukum <oneukum@...e.com>
To:     Bjørn Mork <bjorn@...k.no>
CC:     USB list <linux-usb@...r.kernel.org>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: possible integer overflow in CDC-NCM checks

Hi,

unfortunately there is no maintainer and you were among
the last to send fixes for this driver, so I am going to ask
you for review.

It looks to me like the sanity check in
cdc_ncm_rx_fixup() can be fooled by abusing integer overflows.
You cannot guarantee that the addition of offset and len will
fit into an integer and this gets worse if offset can be
negative.

As this is tricky, do you think this fix is correct?

    Regards
        Oliver

CDC-NCM: avoid overflow in sanity checking A broken device may give an
extreme offset like 0xFFF0 and a reasonable length for a fragment. In
the sanity check as formulated now, this will create an integer
overflow, defeating the sanity check. It needs to be rewritten as a
subtraction and the variables should be unsigned. Signed-off-by: Oliver
Neukum <oneukum@...e.com> --- drivers/net/usb/cdc_ncm.c | 6 +++--- 1
file changed, 3 insertions(+), 3 deletions(-) diff --git
a/drivers/net/usb/cdc_ncm.c b/drivers/net/usb/cdc_ncm.c index
e303b522efb5..f78fccbc4b93 100644 --- a/drivers/net/usb/cdc_ncm.c +++
b/drivers/net/usb/cdc_ncm.c @@ -1715,10 +1715,10 @@ int
cdc_ncm_rx_fixup(struct usbnet *dev, struct sk_buff *skb_in) { struct
sk_buff *skb; struct cdc_ncm_ctx *ctx = (struct cdc_ncm_ctx
*)dev->data[0]; - int len; + unsigned int len; int nframes; int x; - int
offset; + unsigned int offset; union { struct usb_cdc_ncm_ndp16 *ndp16;
struct usb_cdc_ncm_ndp32 *ndp32; @@ -1791,7 +1791,7 @@ int
cdc_ncm_rx_fixup(struct usbnet *dev, struct sk_buff *skb_in) } /* sanity
checking */ - if (((offset + len) > skb_in->len) || + if ((offset >
skb_in->len - len) || (len > ctx->rx_max) || (len < ETH_HLEN)) {
netif_dbg(dev, rx_err, dev->net, "invalid frame detected (ignored)
offset[%u]=%u, length=%u, skb=%p\n", -- 2.34.1