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

[Greg KH <gregkh@linuxfoundation.org>]

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