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@xxxxxxxx> --- 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
