Message ID | 20221205092823.41742-1-szymon.heidrich@gmail.com |
---|---|
State | Accepted |
Commit | 51a0e215ec8ce9cc88f928373e9ce8303db1829b |
Delegated to: | Marek Vasut |
Headers | show |
Series | [v2] usb: gadget: rndis: Prevent InformationBufferOffset manipulation | expand |
On 12/5/22 10:28, Szymon Heidrich wrote: > Prevent access to arbitrary memory locations in gen_ndis_set_resp > via manipulation of buf->InformationBufferOffset. Original > implementation permits manipulation of InformationBufferOffset to > exploit OID_GEN_CURRENT_PACKET_FILTER to set arbitrary memory contents > within a 32byte offset as the devices packet filter. The packet filter > value may be next retrieved using gen_ndis_query_resp so it is possible > to extract specific memory regions two bytes a time. > > The rndis_query_response was not modified as neither the buffer offset > nor length passed to gen_ndis_query_resp is used. > > Signed-off-by: Szymon Heidrich <szymon.heidrich@gmail.com> > --- > V1 -> V2: Updated commit message > > drivers/usb/gadget/rndis.c | 9 ++++++--- > 1 file changed, 6 insertions(+), 3 deletions(-) > > 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); > - > #ifdef VERBOSE > debug("%s: Length: %d\n", __func__, BufLength); > debug("%s: Offset: %d\n", __func__, BufOffset); Applied to usb/master, thanks
On 09/12/2022 02:56, Marek Vasut wrote: > On 12/5/22 10:28, Szymon Heidrich wrote: >> Prevent access to arbitrary memory locations in gen_ndis_set_resp >> via manipulation of buf->InformationBufferOffset. Original >> implementation permits manipulation of InformationBufferOffset to >> exploit OID_GEN_CURRENT_PACKET_FILTER to set arbitrary memory contents >> within a 32byte offset as the devices packet filter. The packet filter >> value may be next retrieved using gen_ndis_query_resp so it is possible >> to extract specific memory regions two bytes a time. >> >> The rndis_query_response was not modified as neither the buffer offset >> nor length passed to gen_ndis_query_resp is used. >> >> Signed-off-by: Szymon Heidrich <szymon.heidrich@gmail.com> >> --- >> V1 -> V2: Updated commit message >> >> drivers/usb/gadget/rndis.c | 9 ++++++--- >> 1 file changed, 6 insertions(+), 3 deletions(-) >> >> 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); >> - >> #ifdef VERBOSE >> debug("%s: Length: %d\n", __func__, BufLength); >> debug("%s: Offset: %d\n", __func__, BufOffset); > > Applied to usb/master, thanks Thank you very much 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); - #ifdef VERBOSE debug("%s: Length: %d\n", __func__, BufLength); debug("%s: Offset: %d\n", __func__, BufOffset);
Prevent access to arbitrary memory locations in gen_ndis_set_resp via manipulation of buf->InformationBufferOffset. Original implementation permits manipulation of InformationBufferOffset to exploit OID_GEN_CURRENT_PACKET_FILTER to set arbitrary memory contents within a 32byte offset as the devices packet filter. The packet filter value may be next retrieved using gen_ndis_query_resp so it is possible to extract specific memory regions two bytes a time. The rndis_query_response was not modified as neither the buffer offset nor length passed to gen_ndis_query_resp is used. Signed-off-by: Szymon Heidrich <szymon.heidrich@gmail.com> --- V1 -> V2: Updated commit message drivers/usb/gadget/rndis.c | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-)