[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 at 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 at 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.