possible integer overflow in CDC-NCM checks

possible integer overflow in CDC-NCM checks

[Oliver Neukum]

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@suse.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

Re: possible integer overflow in CDC-NCM checks

[Bjørn Mork]

Oliver Neukum <oneukum@suse.com> writes:

> 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@suse.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

I don't mind taking a look at it, but that's beyond unreadable...

Could you please resend using "git send-email" or something else that
works?



Bjørn

Re: possible integer overflow in CDC-NCM checks

[Greg KH]

On Thu, Feb 10, 2022 at 04:23:58PM +0100, Oliver Neukum wrote:
> 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@suse.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
> 

Your fix is impossible to read :(