Re: [PATCH 1/2] net: qrtr: ns: Limit the maximum server registration per node

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: qrtr: ns: Limit the maximum server registration per node

This commit introduces a limit of 256 server registrations per node to
prevent malicious clients from exhausting memory by flooding NEW_SERVER
messages.

Cc: stable@vger.kernel.org
Fixes: 0c2204a4ad71 ("net: qrtr: Migrate nameservice to kernel from userspace")
Reported-by: Yiming Qian <yimingqian591@gmail.com>

> diff --git a/net/qrtr/ns.c b/net/qrtr/ns.c
> index 3203b2220860..fb4e8a2d370d 100644
> --- a/net/qrtr/ns.c
> +++ b/net/qrtr/ns.c

[ ... ]

> @@ -229,6 +235,17 @@ static struct qrtr_server *server_add(unsigned int service,
>  	if (!service || !port)
>  		return NULL;
>  
> +	node = node_get(node_id);

Does this per-node limit fully protect against memory exhaustion?

If a remote client spoofs the node ID in the NEW_SERVER message, the
attacker-controlled node_id is passed to server_add(). Because node_get()
unconditionally allocates a new qrtr_node structure when one is not found,
an attacker could continuously send messages with unique node IDs.

Could this result in unbounded node allocations that still exhaust system
memory despite the per-node server limit?

> +	if (!node)
> +		return NULL;
> +
> +	/* Make sure the new servers per port are capped at the maximum value */
> +	old = xa_load(&node->servers, port);
> +	if (!old && node->server_count >= QRTR_NS_MAX_SERVERS) {
> +		pr_err_ratelimited("QRTR client node %u exceeds max server limit!\n", node_id);
> +		return NULL;

While this error message is rate-limited, returning NULL here causes
ctrl_cmd_new_server() to return -EINVAL.

In the main packet loop qrtr_ns_worker(), a negative return value triggers
an un-ratelimited error:

	if (ret < 0)
		pr_err("failed while handling packet from %d:%d",
		       sq.sq_node, sq.sq_port);

Could a flood of excess server registrations trigger this un-ratelimited
message and cause a console logging storm?

> +	}
> +
>  	srv = kzalloc_obj(*srv);
>  	if (!srv)
>  		return NULL;
-- 
pw-bot: cr