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(-)
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)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
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
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
© 2016 - 2026 Red Hat, Inc.