In __qrtr_bind() and qrtr_port_assign(), the port number copied from the
unsigned `u32 addr->sq_port` field is stored in a signed `int port`
variable.

When a user supplies a port number with bit 31 set (`addr->sq_port >=
0x80000000`), `*port` becomes negative:

1. The privileged port check `*port < QRTR_MIN_EPH_SOCKET` (`0x4000`)
   evaluates to true for any negative `int`, requiring `CAP_NET_ADMIN`,
   or conversely in `idr_alloc_u32(&qrtr_ports, ipc, port, *port,
   GFP_ATOMIC)`, passing a `int *` cast to `u32 *` risks signed/unsigned
   mismatch.
2. Changing `port` in `__qrtr_bind()` and `qrtr_port_assign()` from
   `int` to `u32` matches `struct sockaddr_qrtr`'s `u32 sq_port` and
   `idr_alloc_u32()`'s `u32 *` parameter.

Fixes: bdabad3e363d ("net: Add Qualcomm IPC router")
Assisted-by: LLM
Signed-off-by: Hui Peng <benquike@gmail.com>

---
 net/qrtr/af_qrtr.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/qrtr/af_qrtr.c b/net/qrtr/af_qrtr.c
index 78347c937af7..cae7af5b9229 100644
--- a/net/qrtr/af_qrtr.c
+++ b/net/qrtr/af_qrtr.c
@@ -785,7 +785,7 @@ static void qrtr_port_remove(struct qrtr_sock *ipc)
  *   <QRTR_MIN_EPH_SOCKET: Specified; requires CAP_NET_ADMIN
  *   >QRTR_MIN_EPH_SOCKET: Specified; available to all
  */
-static int qrtr_port_assign(struct qrtr_sock *ipc, int *port)
+static int qrtr_port_assign(struct qrtr_sock *ipc, u32 *port)
 {
 	int rc;
 
@@ -835,7 +835,7 @@ static int __qrtr_bind(struct socket *sock,
 {
 	struct qrtr_sock *ipc = qrtr_sk(sock->sk);
 	struct sock *sk = sock->sk;
-	int port;
+	u32 port;
 	int rc;
 
 	/* rebinding ok */
-- 
2.55.0.1082.g2b9226bbc0-goog

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

Critical: 0 · High: 2 · Medium: 1 · 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] net/qrtr/af_qrtr.c:795 — changing `qrtr_port_assign()`'s
  parameter from `int *port` to `u32 *port` silently converts the…
- [High] The signedness conversion was applied only to the insertion
  side, breaking the xarray key convention on 64-bit kernels.
- [Medium] The commit message argues from code that does not exist in the
  tree being patched.

--

net: qrtr: use u32 for port in qrtr_port_assign() and __qrtr_bind()

This changes the port parameter of qrtr_port_assign() and the local port
variable in __qrtr_bind() from int to u32 so they match sq_port in struct
sockaddr_qrtr. A couple of questions below about the resulting behaviour
change and about the rationale in the changelog.

Starting with the changelog itself:

> 1. The privileged port check *port < QRTR_MIN_EPH_SOCKET (0x4000)
>    evaluates to true for any negative int, requiring CAP_NET_ADMIN,
>    or conversely in idr_alloc_u32(&qrtr_ports, ipc, port, *port,
>    GFP_ATOMIC), passing a int * cast to u32 * risks signed/unsigned
>    mismatch.
> 2. Changing port in __qrtr_bind() and qrtr_port_assign() from
>    int to u32 matches struct sockaddr_qrtr's u32 sq_port and
>    idr_alloc_u32()'s u32 * parameter.

[Severity: Medium]
Does this describe the code being patched?  net/qrtr/af_qrtr.c uses an
XArray, not an IDR:

	static DEFINE_XARRAY_ALLOC(qrtr_ports);

and qrtr_port_assign() allocates with:

		rc = xa_alloc(&qrtr_ports, port, ipc, QRTR_EPH_PORT_RANGE,
				GFP_KERNEL);

There is no idr_alloc_u32() and no GFP_ATOMIC in net/qrtr at all, so the
cited call does not exist in this tree.

The actual pre-existing type mismatch appears to be passing int *port to
the u32 *id parameter of xa_alloc().  Could the changelog name that
instead, and also describe the user-visible effect of the type change
(the CAP_NET_ADMIN comparison changing from signed to unsigned), which is
not mentioned at all?

> diff --git a/net/qrtr/af_qrtr.c b/net/qrtr/af_qrtr.c
> index 78347c937af7..cae7af5b9229 100644
> --- a/net/qrtr/af_qrtr.c
> +++ b/net/qrtr/af_qrtr.c
> @@ -785,7 +785,7 @@ static void qrtr_port_remove(struct qrtr_sock *ipc)
>   *   <QRTR_MIN_EPH_SOCKET: Specified; requires CAP_NET_ADMIN
>   *   >QRTR_MIN_EPH_SOCKET: Specified; available to all
>   */
> -static int qrtr_port_assign(struct qrtr_sock *ipc, int *port)
> +static int qrtr_port_assign(struct qrtr_sock *ipc, u32 *port)
>  {
>  	int rc;
>  

[Severity: High]
Does this drop the CAP_NET_ADMIN gate for the upper half of the port
space?  The only privilege check in the bind path is in the body of
qrtr_port_assign():

	} else if (*port < QRTR_MIN_EPH_SOCKET && !capable(CAP_NET_ADMIN)) {
		rc = -EACCES;
	} else if (*port == QRTR_PORT_CTRL) {
		rc = xa_insert(&qrtr_ports, 0, ipc, GFP_KERNEL);

QRTR_MIN_EPH_SOCKET is the int constant 0x4000, so with int *port every
sq_port >= 0x80000000 was negative, compared less than 0x4000, and
returned -EACCES without CAP_NET_ADMIN.  With u32 *port the comparison is
unsigned and 0x80000000..0xffffffff all compare greater than 0x4000, so
they skip the capable() branch entirely.

That includes QRTR_PORT_CTRL, which is 0xfffffffeu in
include/uapi/linux/qrtr.h, so the control-port branch is now reachable
with no capability check.  If the xarray index 0 is free, __qrtr_bind()
then does:

	/* Notify all open ports about the new controller */
	if (port == QRTR_PORT_CTRL)
		qrtr_reset_ports();

which sets sk_err = ENETRESET on every other QRTR socket, and
qrtr_sendmsg() subsequently treats that socket's payload as a QRTR
control command.

Index 0 is normally taken by the in-kernel name server, but qrtr_ns_init()
binds QRTR_PORT_CTRL only after sock_register() in qrtr_proto_init(), so
there is a window where an unprivileged bind can win it.  Independently of
that window, the bypass for 0x80000000..0xfffffffd looks unconditional.

Would it make sense to keep an explicit CAP_NET_ADMIN test for
QRTR_PORT_CTRL and for the high port range alongside the type change?

[Severity: High]
Does changing only the insertion side leave the xarray keys inconsistent
on 64-bit kernels?  qrtr_port_assign() now inserts with a u32:

		rc = xa_insert(&qrtr_ports, *port, ipc, GFP_KERNEL);

xa_insert(), xa_load() and xa_erase() all take an unsigned long index, so
port 0x80000000 is zero-extended and stored at index 0x0000000080000000.

The lookup and removal sides still use int, and were not changed:

	static struct qrtr_sock *qrtr_port_lookup(int port)
	{
		...
		ipc = xa_load(&qrtr_ports, port);

	static void qrtr_port_remove(struct qrtr_sock *ipc)
	{
		...
		int port = ipc->us.sq_port;
		...
		xa_erase(&qrtr_ports, port);

Those sign-extend to 0xffffffff80000000, which can never match the key
used at insert time.  For a socket bound in 0x80000000..0xfffffffd that
seems to give three effects:

- qrtr_port_lookup() never finds it, so qrtr_local_enqueue() and
  qrtr_endpoint_post() return -ENODEV for a port that bound
  successfully.

- At close, xa_erase() removes nothing, but qrtr_port_remove() still
  drops the reference taken by qrtr_port_assign():

	xa_erase(&qrtr_ports, port);
	...
	__sock_put(&ipc->sk);

  so the port stays -EADDRINUSE forever and the stale entry holds a
  dangling struct qrtr_sock * after qrtr_release() drops the last
  reference.

- qrtr_reset_ports() is the only iterator of qrtr_ports and would then
  touch that freed object:

	xa_for_each_start(&qrtr_ports, index, ipc, 1) {
		sock_hold(&ipc->sk);
		ipc->sk.sk_err = ENETRESET;
		sk_error_report(&ipc->sk);

Before the patch, insert, load and erase all sign-extended the same int,
so the keys agreed.  Should qrtr_port_lookup() and the local in
qrtr_port_remove() be widened to u32 in the same change?

> @@ -835,7 +835,7 @@ static int __qrtr_bind(struct socket *sock,
>  {
>  	struct qrtr_sock *ipc = qrtr_sk(sock->sk);
>  	struct sock *sk = sock->sk;
> -	int port;
> +	u32 port;
>  	int rc;
>  
>  	/* rebinding ok */

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