Two validation checks are missing in the QRTR in-kernel nameservice
(`net/qrtr/ns.c`):

1. `qrtr_ns_worker()` receives control packets into a single reused
   4096-byte `recv_buf` via `kernel_recvmsg()` without checking that
   `msglen >= sizeof(*pkt)` (20 bytes). If a short packet arrives on
   `qrtr_ns.sock`, `qrtr_ns_worker()` reads stale bytes from previous
   control messages (or uninitialized heap memory on the first packet)
   when parsing `pkt->cmd` and its union fields.
2. Unlike `ctrl_cmd_del_client()`, `ctrl_cmd_bye()` does not verify that
   `from->sq_port == QRTR_PORT_CTRL`, allowing any unprivileged local
   `AF_QIPCRTR` socket to send a `QRTR_TYPE_BYE` packet and purge all
   registered services for the local node.

Check `msglen < sizeof(*pkt)` in `qrtr_ns_worker()` and require
`from->sq_port == QRTR_PORT_CTRL` in `ctrl_cmd_bye()`.

Fixes: 0c2204a4ad71 ("net: qrtr: Migrate nameservice to kernel from userspace")
Assisted-by: LLM
Signed-off-by: Hui Peng <benquike@gmail.com>

---
 net/qrtr/ns.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/net/qrtr/ns.c b/net/qrtr/ns.c
index bcb090ee79d4..d7e1a37f0717 100644
--- a/net/qrtr/ns.c
+++ b/net/qrtr/ns.c
@@ -347,6 +347,9 @@ static int ctrl_cmd_bye(struct sockaddr_qrtr *from)
 	iv.iov_base = &pkt;
 	iv.iov_len = sizeof(pkt);
 
+	if (from->sq_port != QRTR_PORT_CTRL)
+		return -EINVAL;
+
 	node = node_get(from->sq_node);
 	if (!node)
 		return 0;
@@ -639,6 +642,9 @@ static void qrtr_ns_worker(struct work_struct *work)
 			break;
 		}
 
+		if (msglen < sizeof(*pkt))
+			continue;
+
 		pkt = recv_buf;
 		cmd = le32_to_cpu(pkt->cmd);
 		if (cmd < ARRAY_SIZE(qrtr_ctrl_pkt_strings) &&
-- 
2.55.0.1082.g2b9226bbc0-goog

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 2 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] Over-broad minimum-length gate in net/qrtr/ns.c:qrtr_ns_worker()
  (lines 645-646, `if (msglen < sizeof(*pkt)) continue;`) silently…
- [Medium] The commit message contains two factually incorrect supporting
  claims.
- [Medium] The new unconditional `if (from->sq_port != QRTR_PORT_CTRL)
  return -EINVAL;` in net/qrtr/ns.c:ctrl_cmd_bye() (lines 350-351) also…

--

net: qrtr: ns: validate msglen and sq_port in qrtr_ns_worker() and ctrl_cmd_bye()

This adds two checks to the in-kernel QRTR nameservice: a minimum length
test of sizeof(struct qrtr_ctrl_pkt) on every control datagram received by
qrtr_ns_worker(), and a requirement that BYE messages arrive with
from->sq_port == QRTR_PORT_CTRL in ctrl_cmd_bye().

A few questions about the commit message and the two new checks below.

> 1. `qrtr_ns_worker()` receives control packets into a single reused
>    4096-byte `recv_buf` via `kernel_recvmsg()` without checking that
>    `msglen >= sizeof(*pkt)` (20 bytes). If a short packet arrives on
>    `qrtr_ns.sock`, `qrtr_ns_worker()` reads stale bytes from previous
>    control messages (or uninitialized heap memory on the first packet)
>    when parsing `pkt->cmd` and its union fields.

[Severity: Medium]
Is the "uninitialized heap memory on the first packet" part accurate?
qrtr_ns_worker() allocates the buffer with kzalloc():

net/qrtr/ns.c:qrtr_ns_worker() {
	recv_buf = kzalloc(recv_buf_size, GFP_KERNEL);
	if (!recv_buf)
		return;
}

so every byte not written by the first kernel_recvmsg() reads as zero.  The
remaining exposure looks like stale data from an earlier, longer datagram
handled in the same worker invocation, since the buffer is reused across
loop iterations without being re-zeroed.  Could the changelog be narrowed
to that, so the bug is not triaged as an uninitialized-memory/infoleak
class issue?

> 2. Unlike `ctrl_cmd_del_client()`, `ctrl_cmd_bye()` does not verify that
>    `from->sq_port == QRTR_PORT_CTRL`, allowing any unprivileged local
>    `AF_QIPCRTR` socket to send a `QRTR_TYPE_BYE` packet and purge all
>    registered services for the local node.

Does ctrl_cmd_del_client() actually contain that check?  The sender
validation there is:

net/qrtr/ns.c:ctrl_cmd_del_client() {
	/* Don't accept spoofed messages */
	if (from->sq_node != node_id)
		return -EINVAL;

	/* Local DEL_CLIENT messages comes from the port being closed */
	if (from->sq_node == qrtr_ns.local_node && from->sq_port != port)
		return -EINVAL;
}

QRTR_PORT_CTRL does not appear, and the second test deliberately requires
the client port being closed rather than the control port.  The new BYE
rule is an unconditional authorization requirement instead of a match to an
existing sibling check, so could the comparison be reworded?

> diff --git a/net/qrtr/ns.c b/net/qrtr/ns.c
> index bcb090ee79d4..d7e1a37f0717 100644
> --- a/net/qrtr/ns.c
> +++ b/net/qrtr/ns.c
> @@ -347,6 +347,9 @@ static int ctrl_cmd_bye(struct sockaddr_qrtr *from)
>  	iv.iov_base = &pkt;
>  	iv.iov_len = sizeof(pkt);
>  
> +	if (from->sq_port != QRTR_PORT_CTRL)
> +		return -EINVAL;
> +

[Severity: Medium]
Can this reject BYE messages from remote endpoints that encode their
control source port as the legacy value?  net/qrtr/af_qrtr.c defines:

#define QRTR_PORT_CTRL_LEGACY 0xffff

and qrtr_endpoint_post() only normalizes the destination:

net/qrtr/af_qrtr.c:qrtr_endpoint_post() {
	...
	if (cb->dst_port == QRTR_PORT_CTRL_LEGACY)
		cb->dst_port = QRTR_PORT_CTRL;
}

For QRTR_PROTO_VER_1 the source port is taken raw:

	cb->src_port = le32_to_cpu(v1->src_port_id);

and the v2 path only maps (u16)QRTR_PORT_CTRL, i.e. 0xfffe, not 0xffff.
qrtr_recvmsg() then copies it verbatim:

	addr->sq_port = cb->src_port;

So a BYE arriving with src_port 0xffff would now return -EINVAL before any
teardown runs.  Since ctrl_cmd_bye() is the only place that does:

net/qrtr/ns.c:ctrl_cmd_bye() {
	xa_for_each(&node->servers, index, srv)
		server_del(node, srv->port, true);
	...
delete_node:
	xa_erase(&nodes, from->sq_node);
	kfree(node);
	node_count--;
}

does an early return here leak the struct qrtr_node and all of its struct
qrtr_server objects, keep one of the QRTR_NS_MAX_NODES slots occupied for
the module lifetime, and leave the dead node's services advertised, with
only a rate-limited pr_err as a symptom?

The neighbouring handlers restrict the source port only for local-node
messages:

net/qrtr/ns.c:ctrl_cmd_del_client() {
	/* Local DEL_CLIENT messages comes from the port being closed */
	if (from->sq_node == qrtr_ns.local_node && from->sq_port != port)
		return -EINVAL;
}

Would gating the BYE check on from->sq_node == qrtr_ns.local_node, or
normalizing legacy source ports in qrtr_endpoint_post(), block the local
abuse described in the changelog while keeping remote node cleanup working?

>  	node = node_get(from->sq_node);
>  	if (!node)
>  		return 0;

[ ... ]

> @@ -639,6 +642,9 @@ static void qrtr_ns_worker(struct work_struct *work)
>  			break;
>  		}
>  
> +		if (msglen < sizeof(*pkt))
> +			continue;
> +

[Severity: High]
Does this blanket 20-byte floor drop control packets that the router itself
treats as valid?  qrtr_endpoint_post() only requires the full
struct qrtr_ctrl_pkt for two types:

net/qrtr/af_qrtr.c:qrtr_endpoint_post() {
	if (!size || size > len || len != ALIGN(size, 4) + hdrlen)
		goto err;

	if ((cb->type == QRTR_TYPE_NEW_SERVER ||
	     cb->type == QRTR_TYPE_RESUME_TX) &&
	    size < sizeof(struct qrtr_ctrl_pkt))
		goto err;

	if (cb->dst_port != QRTR_PORT_CTRL && cb->type != QRTR_TYPE_DATA &&
	    cb->type != QRTR_TYPE_RESUME_TX)
		goto err;
}

Everything else with a 4-byte payload is accepted and queued to
QRTR_PORT_CTRL, and qrtr_sendmsg() states the same lower bound for control
port senders:

net/qrtr/af_qrtr.c:qrtr_sendmsg() {
	if (ipc->us.sq_port == QRTR_PORT_CTRL) {
		if (len < 4) {
			rc = -EINVAL;
}

The per-command needs in ns.c look like 4 bytes for HELLO and BYE (neither
handler reads the union), 12 bytes for DEL_CLIENT, NEW_LOOKUP and
DEL_LOOKUP, and 20 bytes only for NEW_SERVER and DEL_SERVER.

With this check, does a 4-byte HELLO get discarded by the bare continue
with no diagnostic, so ctrl_cmd_hello() and announce_servers() never run
and local services are never announced to that node?

And does a 4-byte BYE now skip ctrl_cmd_bye() entirely, leaking the
struct qrtr_node plus its struct qrtr_server objects and consuming a node
slot permanently, since that function is the only node teardown path?

Would per-command length checks mirroring af_qrtr.c (4 /
offsetofend(struct qrtr_ctrl_pkt, client.port) / sizeof(*pkt)), or simply
memset()ing recv_buf at the top of each loop iteration, remove the stale
byte read without dropping traffic the router considers legal?

>  		pkt = recv_buf;
>  		cmd = le32_to_cpu(pkt->cmd);
>  		if (cmd < ARRAY_SIZE(qrtr_ctrl_pkt_strings) &&

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260919212850.3240353-1-benquike%40gmail.com