From f9991ae46e31e8aa9c5c9bafdf1b5ab911041e15 Mon Sep 17 00:00:00 2001 From: Chris Lew Date: Mon, 3 Oct 2022 09:58:55 -0700 Subject: [PATCH 1/5] net: qrtr: Handle IPCR control port format of older targets The destination port value in the IPCR control buffer on older targets is 0xFFFF. Handle the same by updating the dst_port to QRTR_PORT_CTRL. Change-Id: Ia70ce1c078ea84f0de47240f6fc3e764f4ae7a6f Signed-off-by: Ajay Agarwal Signed-off-by: Chris Lew --- net/qrtr/af_qrtr.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/net/qrtr/af_qrtr.c b/net/qrtr/af_qrtr.c index 099a36887a5d..cd16f774bbc7 100644 --- a/net/qrtr/af_qrtr.c +++ b/net/qrtr/af_qrtr.c @@ -25,6 +25,8 @@ #define QRTR_EPH_PORT_RANGE \ XA_LIMIT(QRTR_MIN_EPH_SOCKET, QRTR_MAX_EPH_SOCKET) +#define QRTR_PORT_CTRL_LEGACY 0xffff + /* qrtr socket states */ #define QRTR_STATE_MULTI -2 #define QRTR_STATE_INIT -1 @@ -544,6 +546,9 @@ int qrtr_endpoint_post(struct qrtr_endpoint *ep, const void *data, size_t len) goto err; } + if (cb->dst_port == QRTR_PORT_CTRL_LEGACY) + cb->dst_port = QRTR_PORT_CTRL; + if (!size || len != ALIGN(size, 4) + hdrlen) goto err; From 6ec2df4fa3ccac0352a8f838c5496bf1a98a0b4c Mon Sep 17 00:00:00 2001 From: Chris Lew Date: Mon, 3 Oct 2022 09:59:03 -0700 Subject: [PATCH 2/5] net: qrtr: ns: Change nodes radix tree to xarray There is a use after free scenario while iterating through the servers radix tree despite the ns being a single threaded process. This can happen when the radix tree APIs are not synchronized with the rcu_read_lock() APIs. Convert the radix tree for nodes to xarray to take advantage of the built in rcu lock usage provided by xarray. Change-Id: If2f735abf911b7b47cd7cb07224751114a2bf943 Signed-off-by: Chris Lew --- net/qrtr/ns.c | 25 ++++++------------------- 1 file changed, 6 insertions(+), 19 deletions(-) diff --git a/net/qrtr/ns.c b/net/qrtr/ns.c index 1990d496fcfc..18ccf4199ad2 100644 --- a/net/qrtr/ns.c +++ b/net/qrtr/ns.c @@ -8,6 +8,7 @@ #include #include #include +#include #include #include "qrtr.h" @@ -15,7 +16,7 @@ #define CREATE_TRACE_POINTS #include -static RADIX_TREE(nodes, GFP_KERNEL); +static DEFINE_XARRAY(nodes); static struct { struct socket *sock; @@ -72,7 +73,7 @@ static struct qrtr_node *node_get(unsigned int node_id) { struct qrtr_node *node; - node = radix_tree_lookup(&nodes, node_id); + node = xa_load(&nodes, node_id); if (node) return node; @@ -83,7 +84,7 @@ static struct qrtr_node *node_get(unsigned int node_id) node->id = node_id; - radix_tree_insert(&nodes, node_id, node); + xa_store(&nodes, node_id, node, GFP_KERNEL); return node; } @@ -569,12 +570,11 @@ static int ctrl_cmd_del_server(struct sockaddr_qrtr *from, static int ctrl_cmd_new_lookup(struct sockaddr_qrtr *from, unsigned int service, unsigned int instance) { - struct radix_tree_iter node_iter; struct qrtr_server_filter filter; struct radix_tree_iter srv_iter; struct qrtr_lookup *lookup; struct qrtr_node *node; - void __rcu **node_slot; + unsigned long node_idx; void __rcu **srv_slot; /* Accept only local observers */ @@ -594,17 +594,7 @@ static int ctrl_cmd_new_lookup(struct sockaddr_qrtr *from, filter.service = service; filter.instance = instance; - rcu_read_lock(); - radix_tree_for_each_slot(node_slot, &nodes, &node_iter, 0) { - node = radix_tree_deref_slot(node_slot); - if (!node) - continue; - if (radix_tree_deref_retry(node)) { - node_slot = radix_tree_iter_retry(&node_iter); - continue; - } - node_slot = radix_tree_iter_resume(node_slot, &node_iter); - + xa_for_each(&nodes, node_idx, node) { radix_tree_for_each_slot(srv_slot, &node->servers, &srv_iter, 0) { struct qrtr_server *srv; @@ -622,12 +612,9 @@ static int ctrl_cmd_new_lookup(struct sockaddr_qrtr *from, srv_slot = radix_tree_iter_resume(srv_slot, &srv_iter); - rcu_read_unlock(); lookup_notify(from, srv, true); - rcu_read_lock(); } } - rcu_read_unlock(); /* Empty notification, to indicate end of listing */ lookup_notify(from, NULL, true); From 06424dff8c578d1f87c811712f9660e84a995ae9 Mon Sep 17 00:00:00 2001 From: Chris Lew Date: Mon, 3 Oct 2022 10:00:13 -0700 Subject: [PATCH 3/5] net: qrtr: ns: Change servers radix tree to xarray There is a use after free scenario while iterating through the servers radix tree despite the ns being a single threaded process. This can happen when the radix tree APIs are not synchronized with the rcu_read_lock() APIs. Convert the radix tree for servers to xarray to take advantage of the built in rcu lock usage provided by xarray. Change-Id: I1d9b017da4efba9d8fc72e4666253060cc7b87e3 Signed-off-by: Chris Lew --- net/qrtr/ns.c | 113 ++++++++++---------------------------------------- 1 file changed, 23 insertions(+), 90 deletions(-) diff --git a/net/qrtr/ns.c b/net/qrtr/ns.c index 18ccf4199ad2..3713fcc03c40 100644 --- a/net/qrtr/ns.c +++ b/net/qrtr/ns.c @@ -3,6 +3,7 @@ * Copyright (c) 2015, Sony Mobile Communications Inc. * Copyright (c) 2013, The Linux Foundation. All rights reserved. * Copyright (c) 2020, Linaro Ltd. + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved. */ #include @@ -66,7 +67,7 @@ struct qrtr_server { struct qrtr_node { unsigned int id; - struct radix_tree_root servers; + struct xarray servers; }; static struct qrtr_node *node_get(unsigned int node_id) @@ -83,6 +84,7 @@ static struct qrtr_node *node_get(unsigned int node_id) return NULL; node->id = node_id; + xa_init(&node->servers); xa_store(&nodes, node_id, node, GFP_KERNEL); @@ -190,40 +192,24 @@ static void lookup_notify(struct sockaddr_qrtr *to, struct qrtr_server *srv, static int announce_servers(struct sockaddr_qrtr *sq) { - struct radix_tree_iter iter; struct qrtr_server *srv; struct qrtr_node *node; - void __rcu **slot; + unsigned long index; int ret; node = node_get(qrtr_ns.local_node); if (!node) return 0; - rcu_read_lock(); /* Announce the list of servers registered in this node */ - radix_tree_for_each_slot(slot, &node->servers, &iter, 0) { - srv = radix_tree_deref_slot(slot); - if (!srv) - continue; - if (radix_tree_deref_retry(srv)) { - slot = radix_tree_iter_retry(&iter); - continue; - } - slot = radix_tree_iter_resume(slot, &iter); - rcu_read_unlock(); - + xa_for_each(&node->servers, index, srv) { ret = service_announce_new(sq, srv); if (ret < 0) { pr_err("failed to announce new service\n"); return ret; } - - rcu_read_lock(); } - rcu_read_unlock(); - return 0; } @@ -253,14 +239,17 @@ static struct qrtr_server *server_add(unsigned int service, goto err; /* Delete the old server on the same port */ - old = radix_tree_lookup(&node->servers, port); + old = xa_store(&node->servers, port, srv, GFP_KERNEL); if (old) { - radix_tree_delete(&node->servers, port); - kfree(old); + if (xa_is_err(old)) { + pr_err("failed to add server [0x%x:0x%x] ret:%d\n", + srv->service, srv->instance, xa_err(old)); + goto err; + } else { + kfree(old); + } } - radix_tree_insert(&node->servers, port, srv); - trace_qrtr_ns_server_add(srv->service, srv->instance, srv->node, srv->port); @@ -277,11 +266,11 @@ static int server_del(struct qrtr_node *node, unsigned int port) struct qrtr_server *srv; struct list_head *li; - srv = radix_tree_lookup(&node->servers, port); + srv = xa_load(&node->servers, port); if (!srv) return -ENOENT; - radix_tree_delete(&node->servers, port); + xa_erase(&node->servers, port); /* Broadcast the removal of local servers */ if (srv->node == qrtr_ns.local_node) @@ -341,13 +330,12 @@ static int ctrl_cmd_hello(struct sockaddr_qrtr *sq) static int ctrl_cmd_bye(struct sockaddr_qrtr *from) { struct qrtr_node *local_node; - struct radix_tree_iter iter; struct qrtr_ctrl_pkt pkt; struct qrtr_server *srv; struct sockaddr_qrtr sq; struct msghdr msg = { }; struct qrtr_node *node; - void __rcu **slot; + unsigned long index; struct kvec iv; int ret; @@ -358,22 +346,9 @@ static int ctrl_cmd_bye(struct sockaddr_qrtr *from) if (!node) return 0; - rcu_read_lock(); /* Advertise removal of this client to all servers of remote node */ - radix_tree_for_each_slot(slot, &node->servers, &iter, 0) { - srv = radix_tree_deref_slot(slot); - if (!srv) - continue; - if (radix_tree_deref_retry(srv)) { - slot = radix_tree_iter_retry(&iter); - continue; - } - slot = radix_tree_iter_resume(slot, &iter); - rcu_read_unlock(); + xa_for_each(&node->servers, index, srv) server_del(node, srv->port); - rcu_read_lock(); - } - rcu_read_unlock(); /* Advertise the removal of this client to all local servers */ local_node = node_get(qrtr_ns.local_node); @@ -384,18 +359,7 @@ static int ctrl_cmd_bye(struct sockaddr_qrtr *from) pkt.cmd = cpu_to_le32(QRTR_TYPE_BYE); pkt.client.node = cpu_to_le32(from->sq_node); - rcu_read_lock(); - radix_tree_for_each_slot(slot, &local_node->servers, &iter, 0) { - srv = radix_tree_deref_slot(slot); - if (!srv) - continue; - if (radix_tree_deref_retry(srv)) { - slot = radix_tree_iter_retry(&iter); - continue; - } - slot = radix_tree_iter_resume(slot, &iter); - rcu_read_unlock(); - + xa_for_each(&local_node->servers, index, srv) { sq.sq_family = AF_QIPCRTR; sq.sq_node = srv->node; sq.sq_port = srv->port; @@ -408,11 +372,8 @@ static int ctrl_cmd_bye(struct sockaddr_qrtr *from) pr_err("failed to send bye cmd\n"); return ret; } - rcu_read_lock(); } - rcu_read_unlock(); - return 0; } @@ -420,7 +381,6 @@ static int ctrl_cmd_del_client(struct sockaddr_qrtr *from, unsigned int node_id, unsigned int port) { struct qrtr_node *local_node; - struct radix_tree_iter iter; struct qrtr_lookup *lookup; struct qrtr_ctrl_pkt pkt; struct msghdr msg = { }; @@ -429,7 +389,7 @@ static int ctrl_cmd_del_client(struct sockaddr_qrtr *from, struct qrtr_node *node; struct list_head *tmp; struct list_head *li; - void __rcu **slot; + unsigned long index; struct kvec iv; int ret; @@ -471,18 +431,7 @@ static int ctrl_cmd_del_client(struct sockaddr_qrtr *from, pkt.client.node = cpu_to_le32(node_id); pkt.client.port = cpu_to_le32(port); - rcu_read_lock(); - radix_tree_for_each_slot(slot, &local_node->servers, &iter, 0) { - srv = radix_tree_deref_slot(slot); - if (!srv) - continue; - if (radix_tree_deref_retry(srv)) { - slot = radix_tree_iter_retry(&iter); - continue; - } - slot = radix_tree_iter_resume(slot, &iter); - rcu_read_unlock(); - + xa_for_each(&local_node->servers, index, srv) { sq.sq_family = AF_QIPCRTR; sq.sq_node = srv->node; sq.sq_port = srv->port; @@ -495,11 +444,8 @@ static int ctrl_cmd_del_client(struct sockaddr_qrtr *from, pr_err("failed to send del client cmd\n"); return ret; } - rcu_read_lock(); } - rcu_read_unlock(); - return 0; } @@ -571,11 +517,11 @@ static int ctrl_cmd_new_lookup(struct sockaddr_qrtr *from, unsigned int service, unsigned int instance) { struct qrtr_server_filter filter; - struct radix_tree_iter srv_iter; struct qrtr_lookup *lookup; + struct qrtr_server *srv; struct qrtr_node *node; unsigned long node_idx; - void __rcu **srv_slot; + unsigned long srv_idx; /* Accept only local observers */ if (from->sq_node != qrtr_ns.local_node) @@ -595,23 +541,10 @@ static int ctrl_cmd_new_lookup(struct sockaddr_qrtr *from, filter.instance = instance; xa_for_each(&nodes, node_idx, node) { - radix_tree_for_each_slot(srv_slot, &node->servers, - &srv_iter, 0) { - struct qrtr_server *srv; - - srv = radix_tree_deref_slot(srv_slot); - if (!srv) - continue; - if (radix_tree_deref_retry(srv)) { - srv_slot = radix_tree_iter_retry(&srv_iter); - continue; - } - + xa_for_each(&node->servers, srv_idx, srv) { if (!server_match(srv, &filter)) continue; - srv_slot = radix_tree_iter_resume(srv_slot, &srv_iter); - lookup_notify(from, srv, true); } } From 82b36ba3cb54c194cdf1bbe79b2a2d9e556e780c Mon Sep 17 00:00:00 2001 From: Chris Lew Date: Mon, 3 Oct 2022 10:00:20 -0700 Subject: [PATCH 4/5] net: qrtr: ns: Remove check for spoofed messages In external soc usecases, it is possible to receive control messages for a node that is not the src node. The APPS on the external soc will forward control messages from the modem. Change-Id: Ibbf8f27fa64b18e387e40e655a13b5251190a00b Signed-off-by: Tony Truong Signed-off-by: Chris Lew --- net/qrtr/ns.c | 4 ---- 1 file changed, 4 deletions(-) diff --git a/net/qrtr/ns.c b/net/qrtr/ns.c index 3713fcc03c40..1384e31827a6 100644 --- a/net/qrtr/ns.c +++ b/net/qrtr/ns.c @@ -396,10 +396,6 @@ static int ctrl_cmd_del_client(struct sockaddr_qrtr *from, iv.iov_base = &pkt; iv.iov_len = sizeof(pkt); - /* 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; From d8e12b91fa89e932cbbdf78db1f56343ace124e3 Mon Sep 17 00:00:00 2001 From: Chris Lew Date: Mon, 3 Oct 2022 10:00:36 -0700 Subject: [PATCH 5/5] net: qrtr: ns: Return 0 if server port is not present When a del client message is received from remote, the corresponding server port is deleted. And then, a del server message is announced for this server. As part of handling this message, ns will try to delete this server port again and returns a -ENOENT error. Return value from server_del is returned back to qrtr_ns_worker and any non-zero value is printed to the serial port. This is flooding the serial port display. Returinng 0 in this case as this is not an error. Change-Id: Icd2594875f55313baa6e3313b6aa0c62facc5b2d Signed-off-by: Jay Jayanna Signed-off-by: Chris Lew --- net/qrtr/ns.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/net/qrtr/ns.c b/net/qrtr/ns.c index 1384e31827a6..b092de1678dc 100644 --- a/net/qrtr/ns.c +++ b/net/qrtr/ns.c @@ -268,7 +268,7 @@ static int server_del(struct qrtr_node *node, unsigned int port) srv = xa_load(&node->servers, port); if (!srv) - return -ENOENT; + return 0; xa_erase(&node->servers, port);