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

[...]