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?

Best regards, Szymon

...

---

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); -- 2.38.1

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.

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);

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.