rndis_host: Check for integer overflows in rndis_rx_fixup()

[PATCH net] rndis_host: Check for integer overflows in rndis_rx_fixup()

Posted by Dan Carpenter 2 months, 2 weeks ago

The "data_offset" and "data_len" values come from received skb->data so
we don't trust them.  They are u32 types. Check that the "data_offset +
data_len + 8" addition does not have an integer overflow.

Fixes: 64e049102d3d ("[PATCH] USB: usbnet (8/9) module for RNDIS devices")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
 drivers/net/usb/rndis_host.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/usb/rndis_host.c b/drivers/net/usb/rndis_host.c
index 7b3739b29c8f..913aca6ff434 100644
--- a/drivers/net/usb/rndis_host.c
+++ b/drivers/net/usb/rndis_host.c
@@ -513,8 +513,9 @@ int rndis_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
 		data_len = le32_to_cpu(hdr->data_len);
 
 		/* don't choke if we see oob, per-packet data, etc */
-		if (unlikely(msg_type != RNDIS_MSG_PACKET || skb->len < msg_len
-				|| (data_offset + data_len + 8) > msg_len)) {
+		if (unlikely(msg_type != RNDIS_MSG_PACKET || skb->len < msg_len ||
+				size_add(data_offset, data_len) > U32_MAX - 8 ||
+				(data_offset + data_len + 8) > msg_len)) {
 			dev->net->stats.rx_frame_errors++;
 			netdev_dbg(dev->net, "bad rndis message %d/%d/%d/%d, len %d\n",
 				   le32_to_cpu(hdr->msg_type),
-- 
2.51.0

Re: [PATCH net] rndis_host: Check for integer overflows in rndis_rx_fixup()

Posted by Greg KH 2 months, 2 weeks ago

On Tue, Sep 30, 2025 at 03:35:19PM +0300, Dan Carpenter wrote:
> The "data_offset" and "data_len" values come from received skb->data so
> we don't trust them.  They are u32 types. Check that the "data_offset +
> data_len + 8" addition does not have an integer overflow.
> 
> Fixes: 64e049102d3d ("[PATCH] USB: usbnet (8/9) module for RNDIS devices")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
>  drivers/net/usb/rndis_host.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

David has passed away many years ago, odd that this was sent to him
given that get_maintainers.pl doesn't show it :(

> diff --git a/drivers/net/usb/rndis_host.c b/drivers/net/usb/rndis_host.c
> index 7b3739b29c8f..913aca6ff434 100644
> --- a/drivers/net/usb/rndis_host.c
> +++ b/drivers/net/usb/rndis_host.c
> @@ -513,8 +513,9 @@ int rndis_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
>  		data_len = le32_to_cpu(hdr->data_len);
>  
>  		/* don't choke if we see oob, per-packet data, etc */
> -		if (unlikely(msg_type != RNDIS_MSG_PACKET || skb->len < msg_len
> -				|| (data_offset + data_len + 8) > msg_len)) {
> +		if (unlikely(msg_type != RNDIS_MSG_PACKET || skb->len < msg_len ||
> +				size_add(data_offset, data_len) > U32_MAX - 8 ||
> +				(data_offset + data_len + 8) > msg_len)) {

Nice, I missed this in my old audit of this code (there's still lots of
other types of these bugs in this codebase, remember the rndis standard
says "there is no security", and should never be used by untrusted
devices.)

But will this work?  If size_add(x, y) wraps, it will return SIZE_MAX,
which we hope is bigger than (U32_MAX - 8)?  That feels fragile.

Then we do:
	skb_pull(skb, 8 + data_offset);
so if data_offset was huge, that doesn't really do anything, and then we
treat data_len independent of data_offset.  So even if that check
overflowed, I don't think anything "real" will happen here except a
packet is dropped.

or am I missing something elsewhere in this function?

thanks,

greg k-h

Re: [PATCH net] rndis_host: Check for integer overflows in rndis_rx_fixup()

Posted by Dan Carpenter 2 months, 2 weeks ago

On Tue, Sep 30, 2025 at 03:56:39PM +0200, Greg KH wrote:
> On Tue, Sep 30, 2025 at 03:35:19PM +0300, Dan Carpenter wrote:
> > The "data_offset" and "data_len" values come from received skb->data so
> > we don't trust them.  They are u32 types. Check that the "data_offset +
> > data_len + 8" addition does not have an integer overflow.
> > 
> > Fixes: 64e049102d3d ("[PATCH] USB: usbnet (8/9) module for RNDIS devices")
> > Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> > ---
> >  drivers/net/usb/rndis_host.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> David has passed away many years ago, odd that this was sent to him
> given that get_maintainers.pl doesn't show it :(
> 
> > diff --git a/drivers/net/usb/rndis_host.c b/drivers/net/usb/rndis_host.c
> > index 7b3739b29c8f..913aca6ff434 100644
> > --- a/drivers/net/usb/rndis_host.c
> > +++ b/drivers/net/usb/rndis_host.c
> > @@ -513,8 +513,9 @@ int rndis_rx_fixup(struct usbnet *dev, struct sk_buff *skb)
> >  		data_len = le32_to_cpu(hdr->data_len);
> >  
> >  		/* don't choke if we see oob, per-packet data, etc */
> > -		if (unlikely(msg_type != RNDIS_MSG_PACKET || skb->len < msg_len
> > -				|| (data_offset + data_len + 8) > msg_len)) {
> > +		if (unlikely(msg_type != RNDIS_MSG_PACKET || skb->len < msg_len ||
> > +				size_add(data_offset, data_len) > U32_MAX - 8 ||
> > +				(data_offset + data_len + 8) > msg_len)) {
> 
> Nice, I missed this in my old audit of this code (there's still lots of
> other types of these bugs in this codebase, remember the rndis standard
> says "there is no security", and should never be used by untrusted
> devices.)
> 
> But will this work?  If size_add(x, y) wraps, it will return SIZE_MAX,
> which we hope is bigger than (U32_MAX - 8)?  That feels fragile.
> 

SIZE_MAX is always going to be >= U32_MAX so it works.

Right now size_t is exactly the same as unsigned long.  I think we might
end up making it a separate thing so we can enforce stricter checking.

> Then we do:
> 	skb_pull(skb, 8 + data_offset);
> so if data_offset was huge, that doesn't really do anything, and then we
> treat data_len independent of data_offset.  So even if that check
> overflowed, I don't think anything "real" will happen here except a
> packet is dropped.
> 
> or am I missing something elsewhere in this function?

Yeah.  You're right.  I don't see anything very bad happening with an
integer overflow.

regards,
dan carpenter