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.