[PATCH] Enforce buffer boundaries on RNDIS USB Gadget

[PATCH] Enforce buffer boundaries on RNDIS USB Gadget

[Szymon Heidrich]

Prevent access to arbitrary memory locations in gen_ndis_set_resp
via manipulation of buf->InformationBufferOffset. Lack of validation
of BufOffset could be exploited to dump arbitrary memory contents
via NDIS packet filter.

Signed-off-by: Szymon Heidrich <szymon.heidrich@gmail.com>
---
 drivers/usb/gadget/rndis.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/rndis.c b/drivers/usb/gadget/rndis.c
index 13c327ea38..3948f2cc9a 100644
--- a/drivers/usb/gadget/rndis.c
+++ b/drivers/usb/gadget/rndis.c
@@ -855,14 +855,17 @@ static int rndis_set_response(int configNr, rndis_set_msg_type *buf)
 	rndis_set_cmplt_type	*resp;
 	rndis_resp_t		*r;
 
+	BufLength = get_unaligned_le32(&buf->InformationBufferLength);
+	BufOffset = get_unaligned_le32(&buf->InformationBufferOffset);
+	if ((BufOffset > RNDIS_MAX_TOTAL_SIZE - 8) ||
+	    (BufLength > RNDIS_MAX_TOTAL_SIZE - 8 - BufOffset))
+		return -EINVAL;
+
 	r = rndis_add_response(configNr, sizeof(rndis_set_cmplt_type));
 	if (!r)
 		return -ENOMEM;
 	resp = (rndis_set_cmplt_type *) r->buf;
 
-	BufLength = get_unaligned_le32(&buf->InformationBufferLength);
-	BufOffset = get_unaligned_le32(&buf->InformationBufferOffset);
-
 #ifdef	VERBOSE
 	debug("%s: Length: %d\n", __func__, BufLength);
 	debug("%s: Offset: %d\n", __func__, BufOffset);
-- 
2.38.1

Re: [PATCH] Enforce buffer boundaries on RNDIS USB Gadget

[Fabio Estevam]

Szymon,

On Thu, Nov 17, 2022 at 4:46 PM Szymon Heidrich
<szymon.heidrich@gmail.com> wrote:
>
> Prevent access to arbitrary memory locations in gen_ndis_set_resp
> via manipulation of buf->InformationBufferOffset. Lack of validation
> of BufOffset could be exploited to dump arbitrary memory contents
> via NDIS packet filter.
>
> Signed-off-by: Szymon Heidrich <szymon.heidrich@gmail.com>

Please run ./scripts/get_maintainer.pl on your patch and copy the maintainers.


> ---
>  drivers/usb/gadget/rndis.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/usb/gadget/rndis.c b/drivers/usb/gadget/rndis.c
> index 13c327ea38..3948f2cc9a 100644
> --- a/drivers/usb/gadget/rndis.c
> +++ b/drivers/usb/gadget/rndis.c
> @@ -855,14 +855,17 @@ static int rndis_set_response(int configNr, rndis_set_msg_type *buf)
>         rndis_set_cmplt_type    *resp;
>         rndis_resp_t            *r;
>
> +       BufLength = get_unaligned_le32(&buf->InformationBufferLength);
> +       BufOffset = get_unaligned_le32(&buf->InformationBufferOffset);
> +       if ((BufOffset > RNDIS_MAX_TOTAL_SIZE - 8) ||
> +           (BufLength > RNDIS_MAX_TOTAL_SIZE - 8 - BufOffset))
> +               return -EINVAL;
> +
>         r = rndis_add_response(configNr, sizeof(rndis_set_cmplt_type));
>         if (!r)
>                 return -ENOMEM;
>         resp = (rndis_set_cmplt_type *) r->buf;
>
> -       BufLength = get_unaligned_le32(&buf->InformationBufferLength);
> -       BufOffset = get_unaligned_le32(&buf->InformationBufferOffset);
> -
>  #ifdef VERBOSE
>         debug("%s: Length: %d\n", __func__, BufLength);
>         debug("%s: Offset: %d\n", __func__, BufOffset);
> --
> 2.38.1
>

Re: [PATCH] Enforce buffer boundaries on RNDIS USB Gadget

[Szymon Heidrich]

On 20/11/2022 16:02, Fabio Estevam wrote:
> Szymon,
> 
> On Thu, Nov 17, 2022 at 4:46 PM Szymon Heidrich
> <szymon.heidrich@gmail.com> wrote:
>>
>> Prevent access to arbitrary memory locations in gen_ndis_set_resp
>> via manipulation of buf->InformationBufferOffset. Lack of validation
>> of BufOffset could be exploited to dump arbitrary memory contents
>> via NDIS packet filter.
>>
>> Signed-off-by: Szymon Heidrich <szymon.heidrich@gmail.com>
> 
> Please run ./scripts/get_maintainer.pl on your patch and copy the maintainers.
> 

Hello Fabio,

Sorry I missed adding Lukasz and Marek - I'll keep that in mind for future.

Is there anything else missing from my side?

Best regards,
Szymon

> 
>> ---
>>  drivers/usb/gadget/rndis.c | 9 ++++++---
>>  1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/rndis.c b/drivers/usb/gadget/rndis.c
>> index 13c327ea38..3948f2cc9a 100644
>> --- a/drivers/usb/gadget/rndis.c
>> +++ b/drivers/usb/gadget/rndis.c
>> @@ -855,14 +855,17 @@ static int rndis_set_response(int configNr, rndis_set_msg_type *buf)
>>         rndis_set_cmplt_type    *resp;
>>         rndis_resp_t            *r;
>>
>> +       BufLength = get_unaligned_le32(&buf->InformationBufferLength);
>> +       BufOffset = get_unaligned_le32(&buf->InformationBufferOffset);
>> +       if ((BufOffset > RNDIS_MAX_TOTAL_SIZE - 8) ||
>> +           (BufLength > RNDIS_MAX_TOTAL_SIZE - 8 - BufOffset))
>> +               return -EINVAL;
>> +
>>         r = rndis_add_response(configNr, sizeof(rndis_set_cmplt_type));
>>         if (!r)
>>                 return -ENOMEM;
>>         resp = (rndis_set_cmplt_type *) r->buf;
>>
>> -       BufLength = get_unaligned_le32(&buf->InformationBufferLength);
>> -       BufOffset = get_unaligned_le32(&buf->InformationBufferOffset);
>> -
>>  #ifdef VERBOSE
>>         debug("%s: Length: %d\n", __func__, BufLength);
>>         debug("%s: Offset: %d\n", __func__, BufOffset);
>> --
>> 2.38.1
>>

Re: [PATCH] Enforce buffer boundaries on RNDIS USB Gadget

[Marek Vasut]

On 12/3/22 15:59, Szymon Heidrich wrote:
> On 20/11/2022 16:02, Fabio Estevam wrote:
>> Szymon,
>>
>> On Thu, Nov 17, 2022 at 4:46 PM Szymon Heidrich
>> <szymon.heidrich@gmail.com> wrote:
>>>
>>> Prevent access to arbitrary memory locations in gen_ndis_set_resp
>>> via manipulation of buf->InformationBufferOffset. Lack of validation
>>> of BufOffset could be exploited to dump arbitrary memory contents
>>> via NDIS packet filter.
>>>
>>> Signed-off-by: Szymon Heidrich <szymon.heidrich@gmail.com>
>>
>> Please run ./scripts/get_maintainer.pl on your patch and copy the maintainers.
>>
> 
> Hello Fabio,
> 
> Sorry I missed adding Lukasz and Marek - I'll keep that in mind for future.
> 
> Is there anything else missing from my side?

There have been various security fixes recently which broke other 
things, so I am being careful now.

>>> diff --git a/drivers/usb/gadget/rndis.c b/drivers/usb/gadget/rndis.c
>>> index 13c327ea38..3948f2cc9a 100644
>>> --- a/drivers/usb/gadget/rndis.c
>>> +++ b/drivers/usb/gadget/rndis.c
>>> @@ -855,14 +855,17 @@ static int rndis_set_response(int configNr, rndis_set_msg_type *buf)
>>>          rndis_set_cmplt_type    *resp;
>>>          rndis_resp_t            *r;
>>>
>>> +       BufLength = get_unaligned_le32(&buf->InformationBufferLength);
>>> +       BufOffset = get_unaligned_le32(&buf->InformationBufferOffset);
>>> +       if ((BufOffset > RNDIS_MAX_TOTAL_SIZE - 8) ||
>>> +           (BufLength > RNDIS_MAX_TOTAL_SIZE - 8 - BufOffset))
>>> +               return -EINVAL;
>>> +
>>>          r = rndis_add_response(configNr, sizeof(rndis_set_cmplt_type));
>>>          if (!r)
>>>                  return -ENOMEM;
>>>          resp = (rndis_set_cmplt_type *) r->buf;
>>>
>>> -       BufLength = get_unaligned_le32(&buf->InformationBufferLength);
>>> -       BufOffset = get_unaligned_le32(&buf->InformationBufferOffset);
>>> -

Reading through the RNDIS code, do you think the rndis_query_response 
and others which use buffer/offset data from the message should also be 
sanitized the same way ? I can imagine the query can be used to do test 
for 1bit of data all over the memory too.

Re: [PATCH] Enforce buffer boundaries on RNDIS USB Gadget

[Szymon Heidrich]

On 04/12/2022 20:12, Marek Vasut wrote:
> On 12/3/22 15:59, Szymon Heidrich wrote:
>> On 20/11/2022 16:02, Fabio Estevam wrote:
>>> Szymon,
>>>
>>> On Thu, Nov 17, 2022 at 4:46 PM Szymon Heidrich
>>> <szymon.heidrich@gmail.com> wrote:
>>>>
>>>> Prevent access to arbitrary memory locations in gen_ndis_set_resp
>>>> via manipulation of buf->InformationBufferOffset. Lack of validation
>>>> of BufOffset could be exploited to dump arbitrary memory contents
>>>> via NDIS packet filter.
>>>>
>>>> Signed-off-by: Szymon Heidrich <szymon.heidrich@gmail.com>
>>>
>>> Please run ./scripts/get_maintainer.pl on your patch and copy the maintainers.
>>>
>>
>> Hello Fabio,
>>
>> Sorry I missed adding Lukasz and Marek - I'll keep that in mind for future.
>>
>> Is there anything else missing from my side?
> 
> There have been various security fixes recently which broke other things, so I am being careful now.
> 

Sure, I completely understand that.
Thank you for your time and review.

>>>> diff --git a/drivers/usb/gadget/rndis.c b/drivers/usb/gadget/rndis.c
>>>> index 13c327ea38..3948f2cc9a 100644
>>>> --- a/drivers/usb/gadget/rndis.c
>>>> +++ b/drivers/usb/gadget/rndis.c
>>>> @@ -855,14 +855,17 @@ static int rndis_set_response(int configNr, rndis_set_msg_type *buf)
>>>>          rndis_set_cmplt_type    *resp;
>>>>          rndis_resp_t            *r;
>>>>
>>>> +       BufLength = get_unaligned_le32(&buf->InformationBufferLength);
>>>> +       BufOffset = get_unaligned_le32(&buf->InformationBufferOffset);
>>>> +       if ((BufOffset > RNDIS_MAX_TOTAL_SIZE - 8) ||
>>>> +           (BufLength > RNDIS_MAX_TOTAL_SIZE - 8 - BufOffset))
>>>> +               return -EINVAL;
>>>> +
>>>>          r = rndis_add_response(configNr, sizeof(rndis_set_cmplt_type));
>>>>          if (!r)
>>>>                  return -ENOMEM;
>>>>          resp = (rndis_set_cmplt_type *) r->buf;
>>>>
>>>> -       BufLength = get_unaligned_le32(&buf->InformationBufferLength);
>>>> -       BufOffset = get_unaligned_le32(&buf->InformationBufferOffset);
>>>> -
> 
> Reading through the RNDIS code, do you think the rndis_query_response and others which use buffer/offset data from the message should also be sanitized the same way ? I can imagine the query can be used to do test for 1bit of data all over the memory too.


I added the extra validation in rndis_set_response as with the current implementation
it is possible to manipulate InformationBufferOffset to exploit OID_GEN_CURRENT_PACKET_FILTER 
to set arbitrary memory contents within a 32byte offset as the devices packet filter. 
This value may be next retrieved using gen_ndis_query_resp so one may extract specific memory 
regions two bytes a time.

As for rndis_query_response I didn't touch it as the buffer offset and length passed to gen_ndis_query_resp
are not used. Please let me know in case I'm missing something.

Re: [PATCH] Enforce buffer boundaries on RNDIS USB Gadget

[Marek Vasut]

On 12/4/22 21:36, Szymon Heidrich wrote:

Hi,

sorry for the delays.

>>>>> diff --git a/drivers/usb/gadget/rndis.c b/drivers/usb/gadget/rndis.c
>>>>> index 13c327ea38..3948f2cc9a 100644
>>>>> --- a/drivers/usb/gadget/rndis.c
>>>>> +++ b/drivers/usb/gadget/rndis.c
>>>>> @@ -855,14 +855,17 @@ static int rndis_set_response(int configNr, rndis_set_msg_type *buf)
>>>>>           rndis_set_cmplt_type    *resp;
>>>>>           rndis_resp_t            *r;
>>>>>
>>>>> +       BufLength = get_unaligned_le32(&buf->InformationBufferLength);
>>>>> +       BufOffset = get_unaligned_le32(&buf->InformationBufferOffset);
>>>>> +       if ((BufOffset > RNDIS_MAX_TOTAL_SIZE - 8) ||
>>>>> +           (BufLength > RNDIS_MAX_TOTAL_SIZE - 8 - BufOffset))
>>>>> +               return -EINVAL;
>>>>> +
>>>>>           r = rndis_add_response(configNr, sizeof(rndis_set_cmplt_type));
>>>>>           if (!r)
>>>>>                   return -ENOMEM;
>>>>>           resp = (rndis_set_cmplt_type *) r->buf;
>>>>>
>>>>> -       BufLength = get_unaligned_le32(&buf->InformationBufferLength);
>>>>> -       BufOffset = get_unaligned_le32(&buf->InformationBufferOffset);
>>>>> -
>>
>> Reading through the RNDIS code, do you think the rndis_query_response and others which use buffer/offset data from the message should also be sanitized the same way ? I can imagine the query can be used to do test for 1bit of data all over the memory too.
> 
> 
> I added the extra validation in rndis_set_response as with the current implementation
> it is possible to manipulate InformationBufferOffset to exploit OID_GEN_CURRENT_PACKET_FILTER
> to set arbitrary memory contents within a 32byte offset as the devices packet filter.
> This value may be next retrieved using gen_ndis_query_resp so one may extract specific memory
> regions two bytes a time.

I see, can you please add that ^ explanation into the commit message and 
send V2 ? I'll pick it into the current MR.

Thanks

> As for rndis_query_response I didn't touch it as the buffer offset and length passed to gen_ndis_query_resp
> are not used. Please let me know in case I'm missing something.

[...]

[PATCH v2] usb: gadget: rndis: Prevent InformationBufferOffset manipulation

[Szymon Heidrich]

Prevent access to arbitrary memory locations in gen_ndis_set_resp
via manipulation of buf->InformationBufferOffset. Original
implementation permits manipulation of InformationBufferOffset to
exploit OID_GEN_CURRENT_PACKET_FILTER to set arbitrary memory contents
within a 32byte offset as the devices packet filter. The packet filter
value may be next retrieved using gen_ndis_query_resp so it is possible
to extract specific memory regions two bytes a time.

The rndis_query_response was not modified as neither the buffer offset
nor length passed to gen_ndis_query_resp is used.

Signed-off-by: Szymon Heidrich <szymon.heidrich@gmail.com>
---
V1 -> V2: Updated commit message

 drivers/usb/gadget/rndis.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/usb/gadget/rndis.c b/drivers/usb/gadget/rndis.c
index 13c327ea38..3948f2cc9a 100644
--- a/drivers/usb/gadget/rndis.c
+++ b/drivers/usb/gadget/rndis.c
@@ -855,14 +855,17 @@ static int rndis_set_response(int configNr, rndis_set_msg_type *buf)
 	rndis_set_cmplt_type	*resp;
 	rndis_resp_t		*r;
 
+	BufLength = get_unaligned_le32(&buf->InformationBufferLength);
+	BufOffset = get_unaligned_le32(&buf->InformationBufferOffset);
+	if ((BufOffset > RNDIS_MAX_TOTAL_SIZE - 8) ||
+	    (BufLength > RNDIS_MAX_TOTAL_SIZE - 8 - BufOffset))
+		return -EINVAL;
+
 	r = rndis_add_response(configNr, sizeof(rndis_set_cmplt_type));
 	if (!r)
 		return -ENOMEM;
 	resp = (rndis_set_cmplt_type *) r->buf;
 
-	BufLength = get_unaligned_le32(&buf->InformationBufferLength);
-	BufOffset = get_unaligned_le32(&buf->InformationBufferOffset);
-
 #ifdef	VERBOSE
 	debug("%s: Length: %d\n", __func__, BufLength);
 	debug("%s: Offset: %d\n", __func__, BufOffset);
-- 
2.38.1

Re: [PATCH v2] usb: gadget: rndis: Prevent InformationBufferOffset manipulation

[Marek Vasut]

On 12/5/22 10:28, Szymon Heidrich wrote:
> Prevent access to arbitrary memory locations in gen_ndis_set_resp
> via manipulation of buf->InformationBufferOffset. Original
> implementation permits manipulation of InformationBufferOffset to
> exploit OID_GEN_CURRENT_PACKET_FILTER to set arbitrary memory contents
> within a 32byte offset as the devices packet filter. The packet filter
> value may be next retrieved using gen_ndis_query_resp so it is possible
> to extract specific memory regions two bytes a time.
> 
> The rndis_query_response was not modified as neither the buffer offset
> nor length passed to gen_ndis_query_resp is used.
> 
> Signed-off-by: Szymon Heidrich <szymon.heidrich@gmail.com>
> ---
> V1 -> V2: Updated commit message
> 
>   drivers/usb/gadget/rndis.c | 9 ++++++---
>   1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/gadget/rndis.c b/drivers/usb/gadget/rndis.c
> index 13c327ea38..3948f2cc9a 100644
> --- a/drivers/usb/gadget/rndis.c
> +++ b/drivers/usb/gadget/rndis.c
> @@ -855,14 +855,17 @@ static int rndis_set_response(int configNr, rndis_set_msg_type *buf)
>   	rndis_set_cmplt_type	*resp;
>   	rndis_resp_t		*r;
>   
> +	BufLength = get_unaligned_le32(&buf->InformationBufferLength);
> +	BufOffset = get_unaligned_le32(&buf->InformationBufferOffset);
> +	if ((BufOffset > RNDIS_MAX_TOTAL_SIZE - 8) ||
> +	    (BufLength > RNDIS_MAX_TOTAL_SIZE - 8 - BufOffset))
> +		return -EINVAL;
> +
>   	r = rndis_add_response(configNr, sizeof(rndis_set_cmplt_type));
>   	if (!r)
>   		return -ENOMEM;
>   	resp = (rndis_set_cmplt_type *) r->buf;
>   
> -	BufLength = get_unaligned_le32(&buf->InformationBufferLength);
> -	BufOffset = get_unaligned_le32(&buf->InformationBufferOffset);
> -
>   #ifdef	VERBOSE
>   	debug("%s: Length: %d\n", __func__, BufLength);
>   	debug("%s: Offset: %d\n", __func__, BufOffset);

Applied to usb/master, thanks

Re: [PATCH v2] usb: gadget: rndis: Prevent InformationBufferOffset manipulation

[Szymon Heidrich]

On 09/12/2022 02:56, Marek Vasut wrote:
> On 12/5/22 10:28, Szymon Heidrich wrote:
>> Prevent access to arbitrary memory locations in gen_ndis_set_resp
>> via manipulation of buf->InformationBufferOffset. Original
>> implementation permits manipulation of InformationBufferOffset to
>> exploit OID_GEN_CURRENT_PACKET_FILTER to set arbitrary memory contents
>> within a 32byte offset as the devices packet filter. The packet filter
>> value may be next retrieved using gen_ndis_query_resp so it is possible
>> to extract specific memory regions two bytes a time.
>>
>> The rndis_query_response was not modified as neither the buffer offset
>> nor length passed to gen_ndis_query_resp is used.
>>
>> Signed-off-by: Szymon Heidrich <szymon.heidrich@gmail.com>
>> ---
>> V1 -> V2: Updated commit message
>>
>>   drivers/usb/gadget/rndis.c | 9 ++++++---
>>   1 file changed, 6 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/usb/gadget/rndis.c b/drivers/usb/gadget/rndis.c
>> index 13c327ea38..3948f2cc9a 100644
>> --- a/drivers/usb/gadget/rndis.c
>> +++ b/drivers/usb/gadget/rndis.c
>> @@ -855,14 +855,17 @@ static int rndis_set_response(int configNr, rndis_set_msg_type *buf)
>>       rndis_set_cmplt_type    *resp;
>>       rndis_resp_t        *r;
>>   +    BufLength = get_unaligned_le32(&buf->InformationBufferLength);
>> +    BufOffset = get_unaligned_le32(&buf->InformationBufferOffset);
>> +    if ((BufOffset > RNDIS_MAX_TOTAL_SIZE - 8) ||
>> +        (BufLength > RNDIS_MAX_TOTAL_SIZE - 8 - BufOffset))
>> +        return -EINVAL;
>> +
>>       r = rndis_add_response(configNr, sizeof(rndis_set_cmplt_type));
>>       if (!r)
>>           return -ENOMEM;
>>       resp = (rndis_set_cmplt_type *) r->buf;
>>   -    BufLength = get_unaligned_le32(&buf->InformationBufferLength);
>> -    BufOffset = get_unaligned_le32(&buf->InformationBufferOffset);
>> -
>>   #ifdef    VERBOSE
>>       debug("%s: Length: %d\n", __func__, BufLength);
>>       debug("%s: Offset: %d\n", __func__, BufOffset);
> 
> Applied to usb/master, thanks

Thank you very much for your time and review.