[PATCH] rndis_wlan: Prevent buffer overflow in rndis_query_oid

[PATCH] rndis_wlan: Prevent buffer overflow in rndis_query_oid

[Szymon Heidrich]

Since resplen and respoffs are signed integers sufficiently
large values of unsigned int len and offset members of RNDIS
response will result in negative values of prior variables.
This may be utilized to bypass implemented security checks
to either extract memory contents by manipulating offset or
overflow the data buffer via memcpy by manipulating both
offset and len.

Additionally assure that sum of resplen and respoffs does not
overflow so buffer boundaries are kept.

Fixes: 80f8c5b434f9 ("rndis_wlan: copy only useful data from rndis_command respond")
Signed-off-by: Szymon Heidrich <szymon.heidrich@gmail.com>
---
 drivers/net/wireless/rndis_wlan.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/rndis_wlan.c b/drivers/net/wireless/rndis_wlan.c
index 82a7458e0..d7fc05328 100644
--- a/drivers/net/wireless/rndis_wlan.c
+++ b/drivers/net/wireless/rndis_wlan.c
@@ -697,7 +697,7 @@ static int rndis_query_oid(struct usbnet *dev, u32 oid, void *data, int *len)
 		struct rndis_query_c	*get_c;
 	} u;
 	int ret, buflen;
-	int resplen, respoffs, copylen;
+	u32 resplen, respoffs, copylen;
 
 	buflen = *len + sizeof(*u.get);
 	if (buflen < CONTROL_BUFFER_SIZE)
@@ -740,7 +740,7 @@ static int rndis_query_oid(struct usbnet *dev, u32 oid, void *data, int *len)
 			goto exit_unlock;
 		}
 
-		if ((resplen + respoffs) > buflen) {
+		if (resplen > (buflen - respoffs)) {
 			/* Device would have returned more data if buffer would
 			 * have been big enough. Copy just the bits that we got.
 			 */
-- 
2.38.2

Re: [PATCH] rndis_wlan: Prevent buffer overflow in rndis_query_oid

[Alexander H Duyck]

On Tue, 2023-01-10 at 18:30 +0100, Szymon Heidrich wrote:
> Since resplen and respoffs are signed integers sufficiently
> large values of unsigned int len and offset members of RNDIS
> response will result in negative values of prior variables.
> This may be utilized to bypass implemented security checks
> to either extract memory contents by manipulating offset or
> overflow the data buffer via memcpy by manipulating both
> offset and len.
> 
> Additionally assure that sum of resplen and respoffs does not
> overflow so buffer boundaries are kept.
> 
> Fixes: 80f8c5b434f9 ("rndis_wlan: copy only useful data from rndis_command respond")
> Signed-off-by: Szymon Heidrich <szymon.heidrich@gmail.com>
> ---
>  drivers/net/wireless/rndis_wlan.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/rndis_wlan.c b/drivers/net/wireless/rndis_wlan.c
> index 82a7458e0..d7fc05328 100644
> --- a/drivers/net/wireless/rndis_wlan.c
> +++ b/drivers/net/wireless/rndis_wlan.c
> @@ -697,7 +697,7 @@ static int rndis_query_oid(struct usbnet *dev, u32 oid, void *data, int *len)
>  		struct rndis_query_c	*get_c;
>  	} u;
>  	int ret, buflen;
> -	int resplen, respoffs, copylen;
> +	u32 resplen, respoffs, copylen;

Rather than a u32 why not just make it an size_t? The advantage is that
is the native type for all the memory allocation and copying that takes
place in the function so it would avoid having to cast between u32 and
size_t.

Also why not move buflen over to the unsigned integer category with the
other values you stated were at risk of overflow?

>  
>  	buflen = *len + sizeof(*u.get);
>  	if (buflen < CONTROL_BUFFER_SIZE)

For example, this line here is comparing buflen to a fixed constant. If
we are concerned about overflows this could be triggering an integer
overflow resulting in truncation assuming *len is close to the roll-
over threshold.

By converting to a size_t we would most likely end up blowing up on the
kmalloc and instead returning an -ENOMEM.

> @@ -740,7 +740,7 @@ static int rndis_query_oid(struct usbnet *dev, u32 oid, void *data, int *len)

Also with any type change such as this I believe you would also need to
update the netdev_dbg statement that displays respoffs and the like to
account for the fact that you are now using an unsigned value.
Otherwise I believe %d will display the value as a signed integer
value.

>  			goto exit_unlock;
>  		}
>  
> -		if ((resplen + respoffs) > buflen) {
> +		if (resplen > (buflen - respoffs)) {
>  			/* Device would have returned more data if buffer would
>  			 * have been big enough. Copy just the bits that we got.
>  			 */

Actually you should be able to simplfy this further. Assuming resplen,
buflen and respoffs all the same type this entire if statement could be
broken down into:
		copylen = min(resplen, buflen - respoffs);

Re: [PATCH] rndis_wlan: Prevent buffer overflow in rndis_query_oid

[Szymon Heidrich]

On 10/01/2023 20:39, Alexander H Duyck wrote:
> On Tue, 2023-01-10 at 18:30 +0100, Szymon Heidrich wrote:
>> Since resplen and respoffs are signed integers sufficiently
>> large values of unsigned int len and offset members of RNDIS
>> response will result in negative values of prior variables.
>> This may be utilized to bypass implemented security checks
>> to either extract memory contents by manipulating offset or
>> overflow the data buffer via memcpy by manipulating both
>> offset and len.
>>
>> Additionally assure that sum of resplen and respoffs does not
>> overflow so buffer boundaries are kept.
>>
>> Fixes: 80f8c5b434f9 ("rndis_wlan: copy only useful data from rndis_command respond")
>> Signed-off-by: Szymon Heidrich <szymon.heidrich@gmail.com>
>> ---
>>  drivers/net/wireless/rndis_wlan.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/wireless/rndis_wlan.c b/drivers/net/wireless/rndis_wlan.c
>> index 82a7458e0..d7fc05328 100644
>> --- a/drivers/net/wireless/rndis_wlan.c
>> +++ b/drivers/net/wireless/rndis_wlan.c
>> @@ -697,7 +697,7 @@ static int rndis_query_oid(struct usbnet *dev, u32 oid, void *data, int *len)
>>  		struct rndis_query_c	*get_c;
>>  	} u;
>>  	int ret, buflen;
>> -	int resplen, respoffs, copylen;
>> +	u32 resplen, respoffs, copylen;
> 
> Rather than a u32 why not just make it an size_t? The advantage is that
> is the native type for all the memory allocation and copying that takes
> place in the function so it would avoid having to cast between u32 and
> size_t.
 
My sole intention with this patch was to address the exploitable overflow
with minimal chance of introducing any extra issues.
Sure some things probably could be done differently, but I would stick to
the choices made by original authors of this driver, especially since Greg
mentioned that RNDIS support generally should be dropped at some point.

> Also why not move buflen over to the unsigned integer category with the
> other values you stated were at risk of overflow?
> 
>>  
>>  	buflen = *len + sizeof(*u.get);
>>  	if (buflen < CONTROL_BUFFER_SIZE)
> 
> For example, this line here is comparing buflen to a fixed constant. If
> we are concerned about overflows this could be triggering an integer
> overflow resulting in truncation assuming *len is close to the roll-
> over threshold.

I'm not sure how this would be exploitable since len is controlled by the
developer rather than potential attacker, at least in existing code. Please
correct me in case I'm wrong.
 
> By converting to a size_t we would most likely end up blowing up on the
> kmalloc and instead returning an -ENOMEM.
> 
>> @@ -740,7 +740,7 @@ static int rndis_query_oid(struct usbnet *dev, u32 oid, void *data, int *len)
> 
> Also with any type change such as this I believe you would also need to
> update the netdev_dbg statement that displays respoffs and the like to
> account for the fact that you are now using an unsigned value.
> Otherwise I believe %d will display the value as a signed integer
> value.
> 
>>  			goto exit_unlock;
>>  		}
>>  
>> -		if ((resplen + respoffs) > buflen) {
>> +		if (resplen > (buflen - respoffs)) {
>>  			/* Device would have returned more data if buffer would
>>  			 * have been big enough. Copy just the bits that we got.
>>  			 */
> 
> Actually you should be able to simplfy this further. Assuming resplen,
> buflen and respoffs all the same type this entire if statement could be
> broken down into:
> 		copylen = min(resplen, buflen - respoffs);
> 
> 

Agree, yet I would prefer to avoid any non-essential changes to keep the risk
of introducing errors as low as possible. I intentionally refrained from any
additional modifications. Is this acceptable?

Thank you for your review, I really appreciate all the suggestions.

Re: [PATCH] rndis_wlan: Prevent buffer overflow in rndis_query_oid

[Alexander Duyck]

On Wed, Jan 11, 2023 at 1:54 AM Szymon Heidrich
<szymon.heidrich@gmail.com> wrote:
>
> On 10/01/2023 20:39, Alexander H Duyck wrote:
> > On Tue, 2023-01-10 at 18:30 +0100, Szymon Heidrich wrote:
> >> Since resplen and respoffs are signed integers sufficiently
> >> large values of unsigned int len and offset members of RNDIS
> >> response will result in negative values of prior variables.
> >> This may be utilized to bypass implemented security checks
> >> to either extract memory contents by manipulating offset or
> >> overflow the data buffer via memcpy by manipulating both
> >> offset and len.
> >>
> >> Additionally assure that sum of resplen and respoffs does not
> >> overflow so buffer boundaries are kept.
> >>
> >> Fixes: 80f8c5b434f9 ("rndis_wlan: copy only useful data from rndis_command respond")
> >> Signed-off-by: Szymon Heidrich <szymon.heidrich@gmail.com>
> >> ---
> >>  drivers/net/wireless/rndis_wlan.c | 4 ++--
> >>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/drivers/net/wireless/rndis_wlan.c b/drivers/net/wireless/rndis_wlan.c
> >> index 82a7458e0..d7fc05328 100644
> >> --- a/drivers/net/wireless/rndis_wlan.c
> >> +++ b/drivers/net/wireless/rndis_wlan.c
> >> @@ -697,7 +697,7 @@ static int rndis_query_oid(struct usbnet *dev, u32 oid, void *data, int *len)
> >>              struct rndis_query_c    *get_c;
> >>      } u;
> >>      int ret, buflen;
> >> -    int resplen, respoffs, copylen;
> >> +    u32 resplen, respoffs, copylen;
> >
> > Rather than a u32 why not just make it an size_t? The advantage is that
> > is the native type for all the memory allocation and copying that takes
> > place in the function so it would avoid having to cast between u32 and
> > size_t.
>
> My sole intention with this patch was to address the exploitable overflow
> with minimal chance of introducing any extra issues.
> Sure some things probably could be done differently, but I would stick to
> the choices made by original authors of this driver, especially since Greg
> mentioned that RNDIS support generally should be dropped at some point.

My main concern was that your change will introduce a comparison
between signed and unsigned integer expressions. If you build with W=3
you should find that your changes add new warnings when they trigger
the "-Wsign-compare" check. Based on the comment earlier that you were
concerned about integer roll-over I thought that it might be good to
address that as well.

Basically my initial thought was that buflen should be a u32, but I
had opted to suggest size_t since that is the native type for the size
of memory regions in the kernel.

> > Also why not move buflen over to the unsigned integer category with the
> > other values you stated were at risk of overflow?
> >
> >>
> >>      buflen = *len + sizeof(*u.get);
> >>      if (buflen < CONTROL_BUFFER_SIZE)
> >
> > For example, this line here is comparing buflen to a fixed constant. If
> > we are concerned about overflows this could be triggering an integer
> > overflow resulting in truncation assuming *len is close to the roll-
> > over threshold.
>
> I'm not sure how this would be exploitable since len is controlled by the
> developer rather than potential attacker, at least in existing code. Please
> correct me in case I'm wrong.

The fact that w/ buflen signed and your other variables unsigned it
can lead to mix-ups between the comparisons below as it has to promote
one side or the other so that the types match before making the
comparison.

> > By converting to a size_t we would most likely end up blowing up on the
> > kmalloc and instead returning an -ENOMEM.
> >
> >> @@ -740,7 +740,7 @@ static int rndis_query_oid(struct usbnet *dev, u32 oid, void *data, int *len)
> >
> > Also with any type change such as this I believe you would also need to
> > update the netdev_dbg statement that displays respoffs and the like to
> > account for the fact that you are now using an unsigned value.
> > Otherwise I believe %d will display the value as a signed integer
> > value.
> >
> >>                      goto exit_unlock;
> >>              }
> >>
> >> -            if ((resplen + respoffs) > buflen) {
> >> +            if (resplen > (buflen - respoffs)) {
> >>                      /* Device would have returned more data if buffer would
> >>                       * have been big enough. Copy just the bits that we got.
> >>                       */
> >
> > Actually you should be able to simplfy this further. Assuming resplen,
> > buflen and respoffs all the same type this entire if statement could be
> > broken down into:
> >               copylen = min(resplen, buflen - respoffs);
> >
> >
>
> Agree, yet I would prefer to avoid any non-essential changes to keep the risk
> of introducing errors as low as possible. I intentionally refrained from any
> additional modifications. Is this acceptable?
>
> Thank you for your review, I really appreciate all the suggestions.

What I was getting at is that with this change the use of min should
result in almost exactly the same assembler code. If you look at the
min macro all it is doing is a comparison followed by an assignment,
and in your case you are working with only the two values "resplen"
and "buflen - respoffs" so it just saves space to make use of the
macro.

If you opt to not use the macro at a minimum you can get rid of the
parenthesis around "(buflen - respoffs)" since the order of operations
will complete the subtraction first before the comparison.

[PATCH v2] rndis_wlan: Prevent buffer overflow in rndis_query_oid

[Szymon Heidrich]

Since resplen and respoffs are signed integers sufficiently
large values of unsigned int len and offset members of RNDIS
response will result in negative values of prior variables.
This may be utilized to bypass implemented security checks
to either extract memory contents by manipulating offset or
overflow the data buffer via memcpy by manipulating both
offset and len.

Additionally assure that sum of resplen and respoffs does not
overflow so buffer boundaries are kept.

Fixes: 80f8c5b434f9 ("rndis_wlan: copy only useful data from rndis_command respond")
Signed-off-by: Szymon Heidrich <szymon.heidrich@gmail.com>
---
V1 -> V2: Use size_t and min macro, fix netdev_dbg format

 drivers/net/wireless/rndis_wlan.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/drivers/net/wireless/rndis_wlan.c b/drivers/net/wireless/rndis_wlan.c
index 82a7458e0..bf72e5fd3 100644
--- a/drivers/net/wireless/rndis_wlan.c
+++ b/drivers/net/wireless/rndis_wlan.c
@@ -696,8 +696,8 @@ static int rndis_query_oid(struct usbnet *dev, u32 oid, void *data, int *len)
 		struct rndis_query	*get;
 		struct rndis_query_c	*get_c;
 	} u;
-	int ret, buflen;
-	int resplen, respoffs, copylen;
+	int ret;
+	size_t buflen, resplen, respoffs, copylen;
 
 	buflen = *len + sizeof(*u.get);
 	if (buflen < CONTROL_BUFFER_SIZE)
@@ -732,22 +732,15 @@ static int rndis_query_oid(struct usbnet *dev, u32 oid, void *data, int *len)
 
 		if (respoffs > buflen) {
 			/* Device returned data offset outside buffer, error. */
-			netdev_dbg(dev->net, "%s(%s): received invalid "
-				"data offset: %d > %d\n", __func__,
-				oid_to_string(oid), respoffs, buflen);
+			netdev_dbg(dev->net,
+				   "%s(%s): received invalid data offset: %zu > %zu\n",
+				   __func__, oid_to_string(oid), respoffs, buflen);
 
 			ret = -EINVAL;
 			goto exit_unlock;
 		}
 
-		if ((resplen + respoffs) > buflen) {
-			/* Device would have returned more data if buffer would
-			 * have been big enough. Copy just the bits that we got.
-			 */
-			copylen = buflen - respoffs;
-		} else {
-			copylen = resplen;
-		}
+		copylen = min(resplen, buflen - respoffs);
 
 		if (copylen > *len)
 			copylen = *len;
-- 
2.39.0

Re: [PATCH v2] rndis_wlan: Prevent buffer overflow in rndis_query_oid

[Alexander Duyck]

On Wed, Jan 11, 2023 at 9:51 AM Szymon Heidrich
<szymon.heidrich@gmail.com> wrote:
>
> Since resplen and respoffs are signed integers sufficiently
> large values of unsigned int len and offset members of RNDIS
> response will result in negative values of prior variables.
> This may be utilized to bypass implemented security checks
> to either extract memory contents by manipulating offset or
> overflow the data buffer via memcpy by manipulating both
> offset and len.
>
> Additionally assure that sum of resplen and respoffs does not
> overflow so buffer boundaries are kept.
>
> Fixes: 80f8c5b434f9 ("rndis_wlan: copy only useful data from rndis_command respond")
> Signed-off-by: Szymon Heidrich <szymon.heidrich@gmail.com>
> ---
> V1 -> V2: Use size_t and min macro, fix netdev_dbg format
>
>  drivers/net/wireless/rndis_wlan.c | 19 ++++++-------------
>  1 file changed, 6 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/net/wireless/rndis_wlan.c b/drivers/net/wireless/rndis_wlan.c
> index 82a7458e0..bf72e5fd3 100644
> --- a/drivers/net/wireless/rndis_wlan.c
> +++ b/drivers/net/wireless/rndis_wlan.c
> @@ -696,8 +696,8 @@ static int rndis_query_oid(struct usbnet *dev, u32 oid, void *data, int *len)
>                 struct rndis_query      *get;
>                 struct rndis_query_c    *get_c;
>         } u;
> -       int ret, buflen;
> -       int resplen, respoffs, copylen;
> +       int ret;
> +       size_t buflen, resplen, respoffs, copylen;
>
>         buflen = *len + sizeof(*u.get);
>         if (buflen < CONTROL_BUFFER_SIZE)
> @@ -732,22 +732,15 @@ static int rndis_query_oid(struct usbnet *dev, u32 oid, void *data, int *len)
>
>                 if (respoffs > buflen) {
>                         /* Device returned data offset outside buffer, error. */
> -                       netdev_dbg(dev->net, "%s(%s): received invalid "
> -                               "data offset: %d > %d\n", __func__,
> -                               oid_to_string(oid), respoffs, buflen);
> +                       netdev_dbg(dev->net,
> +                                  "%s(%s): received invalid data offset: %zu > %zu\n",
> +                                  __func__, oid_to_string(oid), respoffs, buflen);
>
>                         ret = -EINVAL;
>                         goto exit_unlock;
>                 }
>
> -               if ((resplen + respoffs) > buflen) {
> -                       /* Device would have returned more data if buffer would
> -                        * have been big enough. Copy just the bits that we got.
> -                        */
> -                       copylen = buflen - respoffs;
> -               } else {
> -                       copylen = resplen;
> -               }
> +               copylen = min(resplen, buflen - respoffs);
>
>                 if (copylen > *len)
>                         copylen = *len;

Looks good to me.

Reviewed-by: Alexander Duyck <alexanderduyck@fb.com>

Re: [PATCH v2] rndis_wlan: Prevent buffer overflow in rndis_query_oid

[Szymon Heidrich]

On 11/01/2023 19:28, Alexander Duyck wrote:
> On Wed, Jan 11, 2023 at 9:51 AM Szymon Heidrich
> <szymon.heidrich@gmail.com> wrote:
>>
>> Since resplen and respoffs are signed integers sufficiently
>> large values of unsigned int len and offset members of RNDIS
>> response will result in negative values of prior variables.
>> This may be utilized to bypass implemented security checks
>> to either extract memory contents by manipulating offset or
>> overflow the data buffer via memcpy by manipulating both
>> offset and len.
>>
>> Additionally assure that sum of resplen and respoffs does not
>> overflow so buffer boundaries are kept.
>>
>> Fixes: 80f8c5b434f9 ("rndis_wlan: copy only useful data from rndis_command respond")
>> Signed-off-by: Szymon Heidrich <szymon.heidrich@gmail.com>
>> ---
>> V1 -> V2: Use size_t and min macro, fix netdev_dbg format
>>
>>  drivers/net/wireless/rndis_wlan.c | 19 ++++++-------------
>>  1 file changed, 6 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/net/wireless/rndis_wlan.c b/drivers/net/wireless/rndis_wlan.c
>> index 82a7458e0..bf72e5fd3 100644
>> --- a/drivers/net/wireless/rndis_wlan.c
>> +++ b/drivers/net/wireless/rndis_wlan.c
>> @@ -696,8 +696,8 @@ static int rndis_query_oid(struct usbnet *dev, u32 oid, void *data, int *len)
>>                 struct rndis_query      *get;
>>                 struct rndis_query_c    *get_c;
>>         } u;
>> -       int ret, buflen;
>> -       int resplen, respoffs, copylen;
>> +       int ret;
>> +       size_t buflen, resplen, respoffs, copylen;
>>
>>         buflen = *len + sizeof(*u.get);
>>         if (buflen < CONTROL_BUFFER_SIZE)
>> @@ -732,22 +732,15 @@ static int rndis_query_oid(struct usbnet *dev, u32 oid, void *data, int *len)
>>
>>                 if (respoffs > buflen) {
>>                         /* Device returned data offset outside buffer, error. */
>> -                       netdev_dbg(dev->net, "%s(%s): received invalid "
>> -                               "data offset: %d > %d\n", __func__,
>> -                               oid_to_string(oid), respoffs, buflen);
>> +                       netdev_dbg(dev->net,
>> +                                  "%s(%s): received invalid data offset: %zu > %zu\n",
>> +                                  __func__, oid_to_string(oid), respoffs, buflen);
>>
>>                         ret = -EINVAL;
>>                         goto exit_unlock;
>>                 }
>>
>> -               if ((resplen + respoffs) > buflen) {
>> -                       /* Device would have returned more data if buffer would
>> -                        * have been big enough. Copy just the bits that we got.
>> -                        */
>> -                       copylen = buflen - respoffs;
>> -               } else {
>> -                       copylen = resplen;
>> -               }
>> +               copylen = min(resplen, buflen - respoffs);
>>
>>                 if (copylen > *len)
>>                         copylen = *len;
> 
> Looks good to me.
> 
> Reviewed-by: Alexander Duyck <alexanderduyck@fb.com>

Awesome, thank you very much.

Re: [PATCH] rndis_wlan: Prevent buffer overflow in rndis_query_oid

[cahu]

[-- Attachment #1: Type: text/plain, Size: 2121 bytes --]

Mitre assigned CVE-2023-23559 for this.

On Tue, 2023-01-10 at 18:30 +0100, Szymon Heidrich wrote:
> Since resplen and respoffs are signed integers sufficiently
> large values of unsigned int len and offset members of RNDIS
> response will result in negative values of prior variables.
> This may be utilized to bypass implemented security checks
> to either extract memory contents by manipulating offset or
> overflow the data buffer via memcpy by manipulating both
> offset and len.
> 
> Additionally assure that sum of resplen and respoffs does not
> overflow so buffer boundaries are kept.
> 
> Fixes: 80f8c5b434f9 ("rndis_wlan: copy only useful data from
> rndis_command respond")
> Signed-off-by: Szymon Heidrich <szymon.heidrich@gmail.com>
> ---
>  drivers/net/wireless/rndis_wlan.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/rndis_wlan.c
> b/drivers/net/wireless/rndis_wlan.c
> index 82a7458e0..d7fc05328 100644
> --- a/drivers/net/wireless/rndis_wlan.c
> +++ b/drivers/net/wireless/rndis_wlan.c
> @@ -697,7 +697,7 @@ static int rndis_query_oid(struct usbnet *dev,
> u32 oid, void *data, int *len)
>                 struct rndis_query_c    *get_c;
>         } u;
>         int ret, buflen;
> -       int resplen, respoffs, copylen;
> +       u32 resplen, respoffs, copylen;
>  
>         buflen = *len + sizeof(*u.get);
>         if (buflen < CONTROL_BUFFER_SIZE)
> @@ -740,7 +740,7 @@ static int rndis_query_oid(struct usbnet *dev,
> u32 oid, void *data, int *len)
>                         goto exit_unlock;
>                 }
>  
> -               if ((resplen + respoffs) > buflen) {
> +               if (resplen > (buflen - respoffs)) {
>                         /* Device would have returned more data if
> buffer would
>                          * have been big enough. Copy just the bits
> that we got.
>                          */


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [v2] wifi: rndis_wlan: Prevent buffer overflow in rndis_query_oid

[Kalle Valo]

Szymon Heidrich <szymon.heidrich@gmail.com> wrote:

> Since resplen and respoffs are signed integers sufficiently
> large values of unsigned int len and offset members of RNDIS
> response will result in negative values of prior variables.
> This may be utilized to bypass implemented security checks
> to either extract memory contents by manipulating offset or
> overflow the data buffer via memcpy by manipulating both
> offset and len.
> 
> Additionally assure that sum of resplen and respoffs does not
> overflow so buffer boundaries are kept.
> 
> Fixes: 80f8c5b434f9 ("rndis_wlan: copy only useful data from rndis_command respond")
> Signed-off-by: Szymon Heidrich <szymon.heidrich@gmail.com>
> Reviewed-by: Alexander Duyck <alexanderduyck@fb.com>

Patch applied to wireless.git, thanks.

b870e73a56c4 wifi: rndis_wlan: Prevent buffer overflow in rndis_query_oid

-- 
https://patchwork.kernel.org/project/linux-wireless/patch/20230111175031.7049-1-szymon.heidrich@gmail.com/

https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches