netdev - Re: [PATCH v3 04/11] net: qrtr: support identical node ids

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250724130836.GL1150792@horms.kernel.org>
Date: Thu, 24 Jul 2025 14:08:36 +0100
From: Simon Horman <horms@...nel.org>
To: Mihai Moldovan <ionic@...ic.de>
Cc: linux-arm-msm@...r.kernel.org, Manivannan Sadhasivam <mani@...nel.org>,
	Denis Kenzior <denkenz@...il.com>,
	Eric Dumazet <edumazet@...gle.com>,
	Kuniyuki Iwashima <kuniyu@...gle.com>,
	Paolo Abeni <pabeni@...hat.com>,
	Willem de Bruijn <willemb@...gle.com>,
	"David S . Miller" <davem@...emloft.net>,
	Jakub Kicinski <kuba@...nel.org>, linux-kernel@...r.kernel.org,
	netdev@...r.kernel.org
Subject: Re: [PATCH v3 04/11] net: qrtr: support identical node ids

On Thu, Jul 24, 2025 at 01:24:01AM +0200, Mihai Moldovan wrote:
> From: Denis Kenzior <denkenz@...il.com>
> 
> Add support for tracking multiple endpoints that may have conflicting
> node identifiers. This is achieved by using both the node and endpoint
> identifiers as the key inside the radix_tree data structure.
> 
> For backward compatibility with existing clients, the previous key
> schema (node identifier only) is preserved. However, this schema will
> only support the first endpoint/node combination.  This is acceptable
> for legacy clients as support for multiple endpoints with conflicting
> node identifiers was not previously possible.
> 
> Signed-off-by: Denis Kenzior <denkenz@...il.com>
> Reviewed-by: Marcel Holtmann <marcel@...tmann.org>
> Reviewed-by: Andy Gross <agross@...nel.org>
> Signed-off-by: Mihai Moldovan <ionic@...ic.de>

...

> diff --git a/net/qrtr/af_qrtr.c b/net/qrtr/af_qrtr.c

...

> @@ -465,19 +466,36 @@ static struct qrtr_node *qrtr_node_lookup(unsigned int nid)
>   *
>   * This is mostly useful for automatic node id assignment, based on
>   * the source id in the incoming packet.
> + *
> + * Return: 0 on success; negative error code on failure
>   */
> -static void qrtr_node_assign(struct qrtr_node *node, unsigned int nid)
> +static int qrtr_node_assign(struct qrtr_node *node, unsigned int nid)
>  {
>  	unsigned long flags;
> +	unsigned long key;
>  
>  	if (nid == QRTR_EP_NID_AUTO)
> -		return;
> +		return 0;
>  
>  	spin_lock_irqsave(&qrtr_nodes_lock, flags);
> -	radix_tree_insert(&qrtr_nodes, nid, node);
> +
> +	if (node->ep->id > QRTR_INDEX_HALF_UNSIGNED_MAX ||
> +	    nid > QRTR_INDEX_HALF_UNSIGNED_MAX)
> +		return -EINVAL;

Hi Mihai, Denis, all,

This will leak holding qrtr_nodes_lock.

Flagged by Smatch.

> +
> +	/* Always insert with the endpoint_id + node_id */
> +	key = ((unsigned long)(node->ep->id) << QRTR_INDEX_HALF_BITS) |
> +	      ((unsigned long)(nid) & QRTR_INDEX_HALF_UNSIGNED_MAX);
> +	radix_tree_insert(&qrtr_nodes, key, node);
> +
> +	if (!radix_tree_lookup(&qrtr_nodes, nid))
> +		radix_tree_insert(&qrtr_nodes, nid, node);
> +
>  	if (node->nid == QRTR_EP_NID_AUTO)
>  		node->nid = nid;
>  	spin_unlock_irqrestore(&qrtr_nodes_lock, flags);
> +
> +	return 0;
>  }
>  
>  /**
> @@ -571,14 +589,18 @@ int qrtr_endpoint_post(struct qrtr_endpoint *ep, const void *data, size_t len)
>  
>  	skb_put_data(skb, data + hdrlen, size);

When declared, ret is assigned the value -EINVAL.
And that is still the value of ret if this line is reached.

>  
> -	qrtr_node_assign(node, cb->src_node);
> +	ret = qrtr_node_assign(node, cb->src_node);
> +	if (ret)
> +		goto err;

With this patch, if we get to this line, ret is 0.
Whereas before this patch it was -EINVAL.

>  
>  	if (cb->type == QRTR_TYPE_NEW_SERVER) {
>  		/* Remote node endpoint can bridge other distant nodes */
>  		const struct qrtr_ctrl_pkt *pkt;
>  
>  		pkt = data + hdrlen;
> -		qrtr_node_assign(node, le32_to_cpu(pkt->server.node));
> +		ret = qrtr_node_assign(node, le32_to_cpu(pkt->server.node));
> +		if (ret)
> +			goto err;
>  	}
>  
>  	if (cb->type == QRTR_TYPE_RESUME_TX) {

The next portion of this function looks like this:

		ret = qrtr_tx_resume(node, skb);
		if (ret)
			goto err;
	} else {
		ipc = qrtr_port_lookup(cb->dst_port);
		if (!ipc)
			goto err;

If we get to the line above, then the function will jump to err,
free skb, and return ret.

But ret is now 0, whereas before this patch it was -EINVAL.
This seems both to be an unintentional side effect of this patch,
and incorrect.

Also flagged by Smatch.

...