[PATCH v2] usb: gadget: rndis: validate query and set message buffers

[PATCH v2] usb: gadget: rndis: validate query and set message buffers

[Pengpeng Hou]

rndis_set_response() already checks the host-controlled
InformationBufferOffset/InformationBufferLength pair before using it,
but the QUERY path still passes the same fields straight into
gen_ndis_query_resp(). The parser also does not verify that MsgLength
fits the actual EP0 request buffer before dispatching the message.

Pass the actual request size into rndis_msg_parser(), reject messages
whose MsgLength exceeds the received buffer, and apply the same offset
and length validation to QUERY and SET requests before dereferencing the
embedded information buffer.

Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
---
v2:
- add commit message context and fix rationale
- no code changes

 drivers/usb/gadget/function/f_rndis.c |  2 +-
 drivers/usb/gadget/function/rndis.c   | 49 +++++++++++++++++++--------
 drivers/usb/gadget/function/rndis.h   |  2 +-
 3 files changed, 37 insertions(+), 16 deletions(-)

diff --git a/drivers/usb/gadget/function/f_rndis.c b/drivers/usb/gadget/function/f_rndis.c
index 8b11d8d6d89c..4878b1133582 100644
--- a/drivers/usb/gadget/function/f_rndis.c
+++ b/drivers/usb/gadget/function/f_rndis.c
@@ -443,7 +443,7 @@ static void rndis_command_complete(struct usb_ep *ep, struct usb_request *req)
 
 	/* received RNDIS command from USB_CDC_SEND_ENCAPSULATED_COMMAND */
 //	spin_lock(&dev->lock);
-	status = rndis_msg_parser(rndis->params, (u8 *) req->buf);
+	status = rndis_msg_parser(rndis->params, (u8 *)req->buf, req->actual);
 	if (status < 0)
 		pr_err("RNDIS command error %d, %d/%d\n",
 			status, req->actual, req->length);
diff --git a/drivers/usb/gadget/function/rndis.c b/drivers/usb/gadget/function/rndis.c
index 3da54a7d7aba..3f3201934c12 100644
--- a/drivers/usb/gadget/function/rndis.c
+++ b/drivers/usb/gadget/function/rndis.c
@@ -588,9 +588,22 @@ static int rndis_init_response(struct rndis_params *params,
 	return 0;
 }
 
+static bool rndis_check_query_set_msg_len(u32 msg_len, u32 buf_offset,
+					  u32 buf_length, size_t min_len)
+{
+	if (msg_len < min_len || msg_len > RNDIS_MAX_TOTAL_SIZE)
+		return false;
+
+	if (buf_offset > msg_len - 8)
+		return false;
+
+	return buf_length <= msg_len - buf_offset - 8;
+}
+
 static int rndis_query_response(struct rndis_params *params,
-				rndis_query_msg_type *buf)
+				rndis_query_msg_type *buf, u32 msg_len)
 {
+	u32 buf_length, buf_offset;
 	rndis_query_cmplt_type *resp;
 	rndis_resp_t *r;
 
@@ -598,6 +611,12 @@ static int rndis_query_response(struct rndis_params *params,
 	if (!params->dev)
 		return -ENOTSUPP;
 
+	buf_length = le32_to_cpu(buf->InformationBufferLength);
+	buf_offset = le32_to_cpu(buf->InformationBufferOffset);
+	if (!rndis_check_query_set_msg_len(msg_len, buf_offset, buf_length,
+					   sizeof(*buf)))
+		return -EINVAL;
+
 	/*
 	 * we need more memory:
 	 * gen_ndis_query_resp expects enough space for
@@ -614,9 +633,7 @@ static int rndis_query_response(struct rndis_params *params,
 	resp->RequestID = buf->RequestID; /* Still LE in msg buffer */
 
 	if (gen_ndis_query_resp(params, le32_to_cpu(buf->OID),
-			le32_to_cpu(buf->InformationBufferOffset)
-					+ 8 + (u8 *)buf,
-			le32_to_cpu(buf->InformationBufferLength),
+			buf_offset + 8 + (u8 *)buf, buf_length,
 			r)) {
 		/* OID not supported */
 		resp->Status = cpu_to_le32(RNDIS_STATUS_NOT_SUPPORTED);
@@ -631,7 +648,7 @@ static int rndis_query_response(struct rndis_params *params,
 }
 
 static int rndis_set_response(struct rndis_params *params,
-			      rndis_set_msg_type *buf)
+			      rndis_set_msg_type *buf, u32 msg_len)
 {
 	u32 BufLength, BufOffset;
 	rndis_set_cmplt_type *resp;
@@ -639,10 +656,9 @@ static int rndis_set_response(struct rndis_params *params,
 
 	BufLength = le32_to_cpu(buf->InformationBufferLength);
 	BufOffset = le32_to_cpu(buf->InformationBufferOffset);
-	if ((BufLength > RNDIS_MAX_TOTAL_SIZE) ||
-	    (BufOffset > RNDIS_MAX_TOTAL_SIZE) ||
-	    (BufOffset + 8 >= RNDIS_MAX_TOTAL_SIZE))
-		    return -EINVAL;
+	if (!rndis_check_query_set_msg_len(msg_len, BufOffset, BufLength,
+					   sizeof(*buf)))
+		return -EINVAL;
 
 	r = rndis_add_response(params, sizeof(rndis_set_cmplt_type));
 	if (!r)
@@ -788,13 +804,13 @@ EXPORT_SYMBOL_GPL(rndis_set_host_mac);
 /*
  * Message Parser
  */
-int rndis_msg_parser(struct rndis_params *params, u8 *buf)
+int rndis_msg_parser(struct rndis_params *params, u8 *buf, u32 buflen)
 {
 	u32 MsgType, MsgLength;
 	__le32 *tmp;
 
-	if (!buf)
-		return -ENOMEM;
+	if (!buf || buflen < 2 * sizeof(*tmp))
+		return -EINVAL;
 
 	tmp = (__le32 *)buf;
 	MsgType   = get_unaligned_le32(tmp++);
@@ -803,6 +819,9 @@ int rndis_msg_parser(struct rndis_params *params, u8 *buf)
 	if (!params)
 		return -ENOTSUPP;
 
+	if (MsgLength > buflen || MsgLength > RNDIS_MAX_TOTAL_SIZE)
+		return -EINVAL;
+
 	/* NOTE: RNDIS is *EXTREMELY* chatty ... Windows constantly polls for
 	 * rx/tx statistics and link status, in addition to KEEPALIVE traffic
 	 * and normal HC level polling to see if there's any IN traffic.
@@ -828,10 +847,12 @@ int rndis_msg_parser(struct rndis_params *params, u8 *buf)
 
 	case RNDIS_MSG_QUERY:
 		return rndis_query_response(params,
-					(rndis_query_msg_type *)buf);
+					(rndis_query_msg_type *)buf,
+					MsgLength);
 
 	case RNDIS_MSG_SET:
-		return rndis_set_response(params, (rndis_set_msg_type *)buf);
+		return rndis_set_response(params, (rndis_set_msg_type *)buf,
+					  MsgLength);
 
 	case RNDIS_MSG_RESET:
 		pr_debug("%s: RNDIS_MSG_RESET\n",
diff --git a/drivers/usb/gadget/function/rndis.h b/drivers/usb/gadget/function/rndis.h
index 6206b8b7490f..cbb016fdad97 100644
--- a/drivers/usb/gadget/function/rndis.h
+++ b/drivers/usb/gadget/function/rndis.h
@@ -178,7 +178,7 @@ typedef struct rndis_params {
 } rndis_params;
 
 /* RNDIS Message parser and other useless functions */
-int  rndis_msg_parser(struct rndis_params *params, u8 *buf);
+int  rndis_msg_parser(struct rndis_params *params, u8 *buf, u32 buflen);
 struct rndis_params *rndis_register(void (*resp_avail)(void *v), void *v);
 void rndis_deregister(struct rndis_params *params);
 int  rndis_set_param_dev(struct rndis_params *params, struct net_device *dev,
-- 
2.50.1 (Apple Git-155)

Re: [PATCH v2] usb: gadget: rndis: validate query and set message buffers

[Greg Kroah-Hartman]

On Mon, Mar 23, 2026 at 04:08:45PM +0800, Pengpeng Hou wrote:
> rndis_set_response() already checks the host-controlled
> InformationBufferOffset/InformationBufferLength pair before using it,
> but the QUERY path still passes the same fields straight into
> gen_ndis_query_resp(). The parser also does not verify that MsgLength
> fits the actual EP0 request buffer before dispatching the message.
> 
> Pass the actual request size into rndis_msg_parser(), reject messages
> whose MsgLength exceeds the received buffer, and apply the same offset
> and length validation to QUERY and SET requests before dereferencing the
> embedded information buffer.
> 
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> ---
> v2:
> - add commit message context and fix rationale
> - no code changes

Have you tested this?  I remember lots of issues like this in the
protocol, so this might not be the only one in here.  I really just want
to delete this code entirely, but some people really like to talk to old
obsolete Windows systems :(

thanks,

greg k-h

Re: [PATCH v2] usb: gadget: rndis: validate query and set message buffers

[Pengpeng Hou]

Hi Greg,

I have not tested this against an actual RNDIS host/device setup yet.

For clarity, v2 did not change the code from v1; it only expanded the
commit message.

What I was trying to fix here is limited to two current-tree checks that
are missing today:

1. rndis_msg_parser() reads MsgLength from the request body but does not
   verify that it fits within the actual EP0 request buffer length.

2. rndis_set_response() validates the host-controlled
   InformationBufferOffset/InformationBufferLength pair before using it,
   but rndis_query_response() still passes the same fields directly into
   gen_ndis_query_resp() without corresponding bounds validation.

I do not mean this patch to claim that these are the only issues in the
RNDIS parser.

If you want runtime testing before considering this further, I can stop
here until I can test it properly.

Thanks,
Pengpeng

Re: [PATCH v2] usb: gadget: rndis: validate query and set message buffers

[Greg KH]

On Wed, Apr 01, 2026 at 07:34:10PM +0800, Pengpeng Hou wrote:
> Hi Greg,
> 
> I have not tested this against an actual RNDIS host/device setup yet.
> 
> For clarity, v2 did not change the code from v1; it only expanded the
> commit message.
> 
> What I was trying to fix here is limited to two current-tree checks that
> are missing today:
> 
> 1. rndis_msg_parser() reads MsgLength from the request body but does not
>    verify that it fits within the actual EP0 request buffer length.

Yes, that's a good thing to check :)

> 2. rndis_set_response() validates the host-controlled
>    InformationBufferOffset/InformationBufferLength pair before using it,
>    but rndis_query_response() still passes the same fields directly into
>    gen_ndis_query_resp() without corresponding bounds validation.

Ah, so that should be fixed up.

> I do not mean this patch to claim that these are the only issues in the
> RNDIS parser.

Oh, I did not think that, there are loads of issues with RNDIS that are
not covered by this patch :)

> If you want runtime testing before considering this further, I can stop
> here until I can test it properly.

Yes, please test this so that we know it doesn't break existing systems.

thanks,

greg k-h