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

[Szymon Heidrich <szymon.heidrich@gmail.com>]

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.