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