1. Patchew
  2. linux
  3. View series

[PATCH] usb: gadget: functioni: Fix a oob problem in rndis

jackysliu posted 1 patch 2 months, 4 weeks ago
There is a newer version of this series
drivers/usb/gadget/function/rndis.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
[PATCH] usb: gadget: functioni: Fix a oob problem in rndis
Posted by jackysliu 2 months, 4 weeks ago 
A critical out-of-bounds memory access vulnerability exists in the RNDIS
(Remote Network Driver Interface Specification) implementation.
The vulnerability stems from insufficient boundary validation when
processing SET requests with user-controlled InformationBufferOffset
and InformationBufferLength parameters.

The vulnerability can be fixed by adding addtional boundary checks

Signed-off-by: jackysliu <1972843537@qq.com>
---
 drivers/usb/gadget/function/rndis.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/rndis.c b/drivers/usb/gadget/function/rndis.c
index afd75d72412c..cc522fb4c06c 100644
--- a/drivers/usb/gadget/function/rndis.c
+++ b/drivers/usb/gadget/function/rndis.c
@@ -641,7 +641,8 @@ static int rndis_set_response(struct rndis_params *params,
 	BufOffset = le32_to_cpu(buf->InformationBufferOffset);
 	if ((BufLength > RNDIS_MAX_TOTAL_SIZE) ||
 	    (BufOffset > RNDIS_MAX_TOTAL_SIZE) ||
-	    (BufOffset + 8 >= RNDIS_MAX_TOTAL_SIZE))
+	    (BufOffset + 8 >= RNDIS_MAX_TOTAL_SIZE) ||
+		(BufOffset + BufLength+8 > RNDIS_MAX_TOTAL_SIZE))
 		    return -EINVAL;
 
 	r = rndis_add_response(params, sizeof(rndis_set_cmplt_type));
-- 
2.43.5
Re: [PATCH] usb: gadget: functioni: Fix a oob problem in rndis
Posted by Greg KH 2 months, 4 weeks ago 
On Thu, Jul 10, 2025 at 04:14:18PM +0800, jackysliu wrote:
> A critical out-of-bounds memory access vulnerability exists in the RNDIS
> (Remote Network Driver Interface Specification) implementation.

It's not really "critical" as the specification never claims to be
secure at all, and we have said for years that you should never run this
on system that you do not fully trust (host and client.)

> The vulnerability stems from insufficient boundary validation when
> processing SET requests with user-controlled InformationBufferOffset
> and InformationBufferLength parameters.
> 
> The vulnerability can be fixed by adding addtional boundary checks
> 
> Signed-off-by: jackysliu <1972843537@qq.com>

Please use a full name, not just a one word alias.

And what commit id does this fix?

> ---
>  drivers/usb/gadget/function/rndis.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/function/rndis.c b/drivers/usb/gadget/function/rndis.c
> index afd75d72412c..cc522fb4c06c 100644
> --- a/drivers/usb/gadget/function/rndis.c
> +++ b/drivers/usb/gadget/function/rndis.c
> @@ -641,7 +641,8 @@ static int rndis_set_response(struct rndis_params *params,
>  	BufOffset = le32_to_cpu(buf->InformationBufferOffset);
>  	if ((BufLength > RNDIS_MAX_TOTAL_SIZE) ||
>  	    (BufOffset > RNDIS_MAX_TOTAL_SIZE) ||
> -	    (BufOffset + 8 >= RNDIS_MAX_TOTAL_SIZE))
> +	    (BufOffset + 8 >= RNDIS_MAX_TOTAL_SIZE) ||
> +		(BufOffset + BufLength+8 > RNDIS_MAX_TOTAL_SIZE))
>  		    return -EINVAL;
>  
>  	r = rndis_add_response(params, sizeof(rndis_set_cmplt_type));
> -- 
> 2.43.5
> 

As I asked before, please run scripts/checkpatch.pl on your patch and
fix the issue it found.  Can you do all of this and send a v2 patch?

thanks,

greg k-h
[PATCH v2] usb: gadget: functioni: Fix a oob problem in rndis
Posted by jackysliu 2 months, 4 weeks ago 
From: Siyang Liu <1972843537@qq.com>

An out-of-bounds memory access vulnerability exists in the RNDIS
(Remote Network Driver Interface Specification) implementation.
The vulnerability stems from insufficient boundary validation when
processing SET requests with user-controlled InformationBufferOffset
and InformationBufferLength parameters.

Fix on commit id:
commit 5f60d5f6bbc1 ("move asm/unaligned.h to linux/unaligned.h")

The vulnerability can be fixed by adding addtional boundary checks

Signed-off-by: Siyang Liu <1972843537@qq.com>
---
 drivers/usb/gadget/function/rndis.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/usb/gadget/function/rndis.c b/drivers/usb/gadget/function/rndis.c
index afd75d72412c..cc522fb4c06c 100644
--- a/drivers/usb/gadget/function/rndis.c
+++ b/drivers/usb/gadget/function/rndis.c
@@ -641,7 +641,8 @@ static int rndis_set_response(struct rndis_params *params,
 	BufOffset = le32_to_cpu(buf->InformationBufferOffset);
 	if ((BufLength > RNDIS_MAX_TOTAL_SIZE) ||
 	    (BufOffset > RNDIS_MAX_TOTAL_SIZE) ||
-	    (BufOffset + 8 >= RNDIS_MAX_TOTAL_SIZE))
+	    (BufOffset + 8 >= RNDIS_MAX_TOTAL_SIZE) ||
+		(BufOffset + BufLength+8 > RNDIS_MAX_TOTAL_SIZE))
 		    return -EINVAL;
 
 	r = rndis_add_response(params, sizeof(rndis_set_cmplt_type));
-- 
2.43.5
Re: [PATCH v2] usb: gadget: functioni: Fix a oob problem in rndis
Posted by Greg KH 2 months, 4 weeks ago 
On Thu, Jul 10, 2025 at 04:49:22PM +0800, jackysliu wrote:
> From: Siyang Liu <1972843537@qq.com>
> 
> An out-of-bounds memory access vulnerability exists in the RNDIS
> (Remote Network Driver Interface Specification) implementation.
> The vulnerability stems from insufficient boundary validation when
> processing SET requests with user-controlled InformationBufferOffset
> and InformationBufferLength parameters.
> 
> Fix on commit id:
> commit 5f60d5f6bbc1 ("move asm/unaligned.h to linux/unaligned.h")
> 
> The vulnerability can be fixed by adding addtional boundary checks
> 
> Signed-off-by: Siyang Liu <1972843537@qq.com>
> ---
>  drivers/usb/gadget/function/rndis.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/function/rndis.c b/drivers/usb/gadget/function/rndis.c
> index afd75d72412c..cc522fb4c06c 100644
> --- a/drivers/usb/gadget/function/rndis.c
> +++ b/drivers/usb/gadget/function/rndis.c
> @@ -641,7 +641,8 @@ static int rndis_set_response(struct rndis_params *params,
>  	BufOffset = le32_to_cpu(buf->InformationBufferOffset);
>  	if ((BufLength > RNDIS_MAX_TOTAL_SIZE) ||
>  	    (BufOffset > RNDIS_MAX_TOTAL_SIZE) ||
> -	    (BufOffset + 8 >= RNDIS_MAX_TOTAL_SIZE))
> +	    (BufOffset + 8 >= RNDIS_MAX_TOTAL_SIZE) ||
> +		(BufOffset + BufLength+8 > RNDIS_MAX_TOTAL_SIZE))

In digging in this further, I don't see how this actually changes
anything.  BufLength is used for nothing that I can determine, except in
some debugging code that is always compiled out (i.e. you can NOT enable
it unless you modify the kernel source.)

So what exactly is this check checking?

I can see that we really should be checking if the buffer is too small,
but that's not what you are doing here at all.

And all this buffer is used for is to read a 32bit value out of, so
verifying that the buffer really is big enough to hold that value SHOULD
be what we do here, not check to see if the buffer is too big.

Also, you can't trust that BufLength is even correct as it comes from
the other side, right?  Because of that, we should just be ignoring it
entirely and verifying that the message size really is as big as the
structure is supposed to be.  But that means passing down the message
size to the lower layers here, which gets into the issues that I have
raised before many years ago about this protocol, and this
implementation of this protocol.  I.e, it is COMPLETELY INSECURE and
should ONLY be used on systems where you trust both sides of the wire.

Again, how was this change tested?  And what exactly does it fix?  I'm
missing how this change is going to actually catch anything, can you
spell it out in detail for me?

thanks,

greg k-h
Re: [PATCH v2] usb: gadget: functioni: Fix a oob problem in rndis
Posted by jackysliu 2 months, 4 weeks ago 
On Thu, 10 Jul 2025 14:19:47 +0200, greg k-h wrote

>In digging in this further, I don't see how this actually changes
>anything.  BufLength is used for nothing that I can determine, except in
>some debugging code that is always compiled out (i.e. you can NOT enable
>it unless you modify the kernel source.)
>
>So what exactly is this check checking?
>
>I can see that we really should be checking if the buffer is too small,
>but that's not what you are doing here at all.
>
>And all this buffer is used for is to read a 32bit value out of, so
>verifying that the buffer really is big enough to hold that value SHOULD
>be what we do here, not check to see if the buffer is too big.
>
>Also, you can't trust that BufLength is even correct as it comes from
>the other side, right?  Because of that, we should just be ignoring it
>entirely and verifying that the message size really is as big as the
>structure is supposed to be.  But that means passing down the message
>size to the lower layers here, which gets into the issues that I have
>raised before many years ago about this protocol, and this
>implementation of this protocol.  I.e, it is COMPLETELY INSECURE and
>should ONLY be used on systems where you trust both sides of the wire.
>
>Again, how was this change tested?  And what exactly does it fix?  I'm
>missing how this change is going to actually catch anything, can you
>spell it out in detail for me?

BufOffset + BufLength can exceed buffer size even if each is < RNDIS_MAX_TOTAL_SIZE
oob is triggered by a function call to gen_ndis_set_resp.
I supposed to add an additional boundry check to avoid this issue.But That 
doesn't seem to be enough to fix the bug.I'll try to figure it out.

Thanks

Siyang Liu
Re: [PATCH v2] usb: gadget: functioni: Fix a oob problem in rndis
Posted by Greg KH 2 months, 4 weeks ago 
On Fri, Jul 11, 2025 at 11:46:35AM +0800, jackysliu wrote:
> On Thu, 10 Jul 2025 14:19:47 +0200, greg k-h wrote
> 
> >In digging in this further, I don't see how this actually changes
> >anything.  BufLength is used for nothing that I can determine, except in
> >some debugging code that is always compiled out (i.e. you can NOT enable
> >it unless you modify the kernel source.)
> >
> >So what exactly is this check checking?
> >
> >I can see that we really should be checking if the buffer is too small,
> >but that's not what you are doing here at all.
> >
> >And all this buffer is used for is to read a 32bit value out of, so
> >verifying that the buffer really is big enough to hold that value SHOULD
> >be what we do here, not check to see if the buffer is too big.
> >
> >Also, you can't trust that BufLength is even correct as it comes from
> >the other side, right?  Because of that, we should just be ignoring it
> >entirely and verifying that the message size really is as big as the
> >structure is supposed to be.  But that means passing down the message
> >size to the lower layers here, which gets into the issues that I have
> >raised before many years ago about this protocol, and this
> >implementation of this protocol.  I.e, it is COMPLETELY INSECURE and
> >should ONLY be used on systems where you trust both sides of the wire.
> >
> >Again, how was this change tested?  And what exactly does it fix?  I'm
> >missing how this change is going to actually catch anything, can you
> >spell it out in detail for me?
> 
> BufOffset + BufLength can exceed buffer size even if each is < RNDIS_MAX_TOTAL_SIZE

Sure, but again, BufLength is not used for anything, so the value of
that variable means nothing as far as I can tell.

> oob is triggered by a function call to gen_ndis_set_resp.

How exactly?  Again, BufLength isn't even used in that function.

> I supposed to add an additional boundry check to avoid this issue.But That 
> doesn't seem to be enough to fix the bug.I'll try to figure it out.

How was this tested?

And even more importantly, how did you find this bug?  What triggered
it?

thanks,

greg k-h
Re: [PATCH v2] usb: gadget: functioni: Fix a oob problem in rndis
Posted by jackysliu 2 months, 4 weeks ago 
On Fri, Jul 11, 2025 at 11:46:35AM +0800, greg k-h wrote
>Sure, but again, BufLength is not used for anything, so the value of
>that variable means nothing as far as I can tell.
>How exactly?  Again, BufLength isn't even used in that function
function contains below code:
if (gen_ndis_set_resp(params, le32_to_cpu(buf->OID),
			((u8 *)buf) + 8 + BufOffset, BufLength, r))
((u8 *)buf) + 8 + BufOffset determins base address of buffer 
and BufLength determins buflen.

>How was this tested?
>
>And even more importantly, how did you find this bug?  What triggered
>it?
I detected this problem through static analysis and calibrated
 the device via qemu emulation.

Anyway,this problem seems hard to fix, I'll try my best.

Thanks

Thanks

Siyang Liu
Re: [PATCH v2] usb: gadget: functioni: Fix a oob problem in rndis
Posted by Greg KH 2 months, 4 weeks ago 
On Fri, Jul 11, 2025 at 02:40:19PM +0800, jackysliu wrote:
> On Fri, Jul 11, 2025 at 11:46:35AM +0800, greg k-h wrote
> >Sure, but again, BufLength is not used for anything, so the value of
> >that variable means nothing as far as I can tell.
> >How exactly?  Again, BufLength isn't even used in that function
> function contains below code:
> if (gen_ndis_set_resp(params, le32_to_cpu(buf->OID),
> 			((u8 *)buf) + 8 + BufOffset, BufLength, r))
> ((u8 *)buf) + 8 + BufOffset determins base address of buffer 
> and BufLength determins buflen.

Yes, and then look to see what buf_len (not buflen) in
gen_ndis_set_resp() is used for.  I'll wait... :)


> >How was this tested?
> >
> >And even more importantly, how did you find this bug?  What triggered
> >it?
> I detected this problem through static analysis and calibrated
>  the device via qemu emulation.

What tool generated this static analysis?  You always have to mention
that as per our development rules.

And what qemu setup did you use to test this?  That would be helpful to
know so that I can verify it on my end.

thanks,

greg k-h
Re: [PATCH v2] usb: gadget: functioni: Fix a oob problem in rndis
Posted by jackysliu 2 months, 3 weeks ago 
On Fri, Jul 11 2025 08:51:30 +0200, greg k-h wrote:

>Yes, and then look to see what buf_len (not buflen) in
>gen_ndis_set_resp() is used for.  I'll wait... :)
Oh,my bad.It seem that buf_len will only be used for some debugging code..

>What tool generated this static analysis?  You always have to mention
>that as per our development rules.
The vulnerability is found by  is found by Wukong-Agent, a code security AI agent,
 through static code analysis.But It seems that this is a false positive..

>And what qemu setup did you use to test this?  That would be helpful to
>know so that I can verify it on my end.

I've add some web-usb device to test this model.But seems that I went into a wrong way.

I'll double check my commit in the future
Thanks for your time

Siyang Liu
Re: [PATCH v2] usb: gadget: functioni: Fix a oob problem in rndis
Posted by Greg KH 2 months, 3 weeks ago 
On Tue, Jul 15, 2025 at 04:20:09PM +0800, jackysliu wrote:
> On Fri, Jul 11 2025 08:51:30 +0200, greg k-h wrote:
> 
> >Yes, and then look to see what buf_len (not buflen) in
> >gen_ndis_set_resp() is used for.  I'll wait... :)
> Oh,my bad.It seem that buf_len will only be used for some debugging code..
> 
> >What tool generated this static analysis?  You always have to mention
> >that as per our development rules.
> The vulnerability is found by  is found by Wukong-Agent, a code security AI agent,
>  through static code analysis.But It seems that this is a false positive..

As per our documentation, you have to always disclose what tools you use
to find stuff.  Please always do that, otherwise your reports are going
to be ignored.

And then also properly TEST your change to verify that it works before
submitting it, that didn't happen here.

> >And what qemu setup did you use to test this?  That would be helpful to
> >know so that I can verify it on my end.
> 
> I've add some web-usb device to test this model.But seems that I went into a wrong way.

What is a "web-usb" device?  How does rndis work with that?

thanks,

greg k-h
Re: [PATCH v2] usb: gadget: functioni: Fix a oob problem in rndis
Posted by jackysliu 2 months, 3 weeks ago 
On Tue, Jul 15 2025 10:32:28 +0200, greg k-h wrote:

>As per our documentation, you have to always disclose what tools you use
>to find stuff.  Please always do that, otherwise your reports are going
>to be ignored.
>
>And then also properly TEST your change to verify that it works before
>submitting it, that didn't happen here.
OK , I'll mind that later.Thanks for your kind reply.

Siyang Liu
Re: [PATCH v2] usb: gadget: functioni: Fix a oob problem in rndis
Posted by Greg KH 2 months, 4 weeks ago 
On Thu, Jul 10, 2025 at 04:49:22PM +0800, jackysliu wrote:
> From: Siyang Liu <1972843537@qq.com>
> 
> An out-of-bounds memory access vulnerability exists in the RNDIS
> (Remote Network Driver Interface Specification) implementation.
> The vulnerability stems from insufficient boundary validation when
> processing SET requests with user-controlled InformationBufferOffset
> and InformationBufferLength parameters.
> 
> Fix on commit id:
> commit 5f60d5f6bbc1 ("move asm/unaligned.h to linux/unaligned.h")
> 
> The vulnerability can be fixed by adding addtional boundary checks
> 
> Signed-off-by: Siyang Liu <1972843537@qq.com>
> ---
>  drivers/usb/gadget/function/rndis.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/usb/gadget/function/rndis.c b/drivers/usb/gadget/function/rndis.c
> index afd75d72412c..cc522fb4c06c 100644
> --- a/drivers/usb/gadget/function/rndis.c
> +++ b/drivers/usb/gadget/function/rndis.c
> @@ -641,7 +641,8 @@ static int rndis_set_response(struct rndis_params *params,
>  	BufOffset = le32_to_cpu(buf->InformationBufferOffset);
>  	if ((BufLength > RNDIS_MAX_TOTAL_SIZE) ||
>  	    (BufOffset > RNDIS_MAX_TOTAL_SIZE) ||
> -	    (BufOffset + 8 >= RNDIS_MAX_TOTAL_SIZE))
> +	    (BufOffset + 8 >= RNDIS_MAX_TOTAL_SIZE) ||
> +		(BufOffset + BufLength+8 > RNDIS_MAX_TOTAL_SIZE))

Odd, this should be "BufLength + 8" as checkpatch says:

CHECK: spaces preferred around that '+' (ctx:VxV)
#121: FILE: drivers/usb/gadget/function/rndis.c:645:
+		(BufOffset + BufLength+8 > RNDIS_MAX_TOTAL_SIZE))
 		                     ^

I'll go fix this up by hand, but be more careful in the future...

thanks,

greg k-h
Re: [PATCH v2] usb: gadget: functioni: Fix a oob problem in rndis
Posted by jackysliu 2 months, 4 weeks ago 
On Thu, Jul 10, 2025 at 11:16:41 +0200, greg k-h wrote

>Odd, this should be "BufLength + 8" as checkpatch says:
>
>CHECK: spaces preferred around that '+' (ctx:VxV)
>#121: FILE: drivers/usb/gadget/function/rndis.c:645:
>+		(BufOffset + BufLength+8 > RNDIS_MAX_TOTAL_SIZE))
Oh I don't know why this issue wasn't reported.I'll mind that in the
future.
Thank you for your patient review.

Siyang Liu
Re: [PATCH v2] usb: gadget: functioni: Fix a oob problem in rndis
Posted by Greg KH 2 months, 4 weeks ago 
On Thu, Jul 10, 2025 at 11:16:41AM +0200, Greg KH wrote:
> On Thu, Jul 10, 2025 at 04:49:22PM +0800, jackysliu wrote:
> > From: Siyang Liu <1972843537@qq.com>
> > 
> > An out-of-bounds memory access vulnerability exists in the RNDIS
> > (Remote Network Driver Interface Specification) implementation.
> > The vulnerability stems from insufficient boundary validation when
> > processing SET requests with user-controlled InformationBufferOffset
> > and InformationBufferLength parameters.
> > 
> > Fix on commit id:
> > commit 5f60d5f6bbc1 ("move asm/unaligned.h to linux/unaligned.h")
> > 
> > The vulnerability can be fixed by adding addtional boundary checks
> > 
> > Signed-off-by: Siyang Liu <1972843537@qq.com>
> > ---
> >  drivers/usb/gadget/function/rndis.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/usb/gadget/function/rndis.c b/drivers/usb/gadget/function/rndis.c
> > index afd75d72412c..cc522fb4c06c 100644
> > --- a/drivers/usb/gadget/function/rndis.c
> > +++ b/drivers/usb/gadget/function/rndis.c
> > @@ -641,7 +641,8 @@ static int rndis_set_response(struct rndis_params *params,
> >  	BufOffset = le32_to_cpu(buf->InformationBufferOffset);
> >  	if ((BufLength > RNDIS_MAX_TOTAL_SIZE) ||
> >  	    (BufOffset > RNDIS_MAX_TOTAL_SIZE) ||
> > -	    (BufOffset + 8 >= RNDIS_MAX_TOTAL_SIZE))
> > +	    (BufOffset + 8 >= RNDIS_MAX_TOTAL_SIZE) ||
> > +		(BufOffset + BufLength+8 > RNDIS_MAX_TOTAL_SIZE))
> 
> Odd, this should be "BufLength + 8" as checkpatch says:
> 
> CHECK: spaces preferred around that '+' (ctx:VxV)
> #121: FILE: drivers/usb/gadget/function/rndis.c:645:
> +		(BufOffset + BufLength+8 > RNDIS_MAX_TOTAL_SIZE))
>  		                     ^

And the indentation needs to be fixed up too.  I'll do that...

How was this tested?

thanks,

greg k-h

Patchew 2.1-devel-b67e4c2

© 2016 - 2025 Red Hat, Inc.