Add fixes in QRTR ns to change server and nodes radix tree to xarray to
avoid a use-after-free while iterating through the server or nodes
radix tree.
Also fix the destination port value for IPCR control buffer on older
targets.
Vignesh Viswanathan (3):
net: qrtr: ns: Change servers radix tree to xarray
net: qrtr: ns: Change nodes radix tree to xarray
net: qrtr: Handle IPCR control port format of older targets
net/qrtr/af_qrtr.c | 5 ++
net/qrtr/ns.c | 139 +++++++++------------------------------------
2 files changed, 32 insertions(+), 112 deletions(-)
--
2.41.0
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.
Signed-off-by: Chris Lew <[email protected]>
Signed-off-by: Vignesh Viswanathan <[email protected]>
---
net/qrtr/ns.c | 133 +++++++++-----------------------------------------
1 file changed, 24 insertions(+), 109 deletions(-)
diff --git a/net/qrtr/ns.c b/net/qrtr/ns.c
index 0f7a729f1a1f..039313c3e044 100644
--- a/net/qrtr/ns.c
+++ b/net/qrtr/ns.c
@@ -66,7 +66,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 +83,7 @@ static struct qrtr_node *node_get(unsigned int node_id)
return NULL;
node->id = node_id;
+ xa_init(&node->servers);
if (radix_tree_insert(&nodes, node_id, node)) {
kfree(node);
@@ -193,40 +194,23 @@ 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;
}
@@ -256,14 +240,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);
@@ -280,11 +267,11 @@ static int server_del(struct qrtr_node *node, unsigned int port, bool bcast)
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 && bcast)
@@ -344,13 +331,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;
@@ -361,22 +347,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, true);
- rcu_read_lock();
- }
- rcu_read_unlock();
/* Advertise the removal of this client to all local servers */
local_node = node_get(qrtr_ns.local_node);
@@ -387,18 +360,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;
@@ -411,11 +373,7 @@ 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;
}
@@ -423,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 = { };
@@ -432,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;
@@ -477,18 +434,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;
@@ -501,11 +447,7 @@ 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;
}
@@ -576,13 +518,12 @@ 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;
- void __rcu **srv_slot;
+ struct qrtr_server *srv;
+ unsigned long node_idx;
+ unsigned long srv_idx;
/* Accept only local observers */
if (from->sq_node != qrtr_ns.local_node)
@@ -601,40 +542,14 @@ 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);
-
- 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(&nodes, node_idx, node) {
+ xa_for_each(&node->servers, srv_idx, srv) {
if (!server_match(srv, &filter))
continue;
- 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);
--
2.41.0
On Wed, Jul 12, 2023 at 04:56:29PM +0530, Vignesh Viswanathan wrote:
> 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.
>
> Signed-off-by: Chris Lew <[email protected]>
> Signed-off-by: Vignesh Viswanathan <[email protected]>
Reviewed-by: Simon Horman <[email protected]>
A few very minor nits below.
...
> @@ -256,14 +240,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));
The indentation of the line above is not correct.
It should be:
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);
>
...
> @@ -576,13 +518,12 @@ 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;
> - void __rcu **srv_slot;
> + struct qrtr_server *srv;
This breaks reverse xmas tree ordering of local variables.
The srv line should be directly above rather than below the node line.
> + unsigned long node_idx;
> + unsigned long srv_idx;
>
> /* Accept only local observers */
> if (from->sq_node != qrtr_ns.local_node)
...
On 7/13/2023 8:24 PM, Simon Horman wrote:
> On Wed, Jul 12, 2023 at 04:56:29PM +0530, Vignesh Viswanathan wrote:
>> 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.
>>
>> Signed-off-by: Chris Lew <[email protected]>
>> Signed-off-by: Vignesh Viswanathan <[email protected]>
>
> Reviewed-by: Simon Horman <[email protected]>
>
> A few very minor nits below.
Thanks Simon for the review. Will fix both the comments in V2.
Thanks,
Vignesh
>
> ...
>
>> @@ -256,14 +240,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));
>
> The indentation of the line above is not correct.
> It should be:
>
> 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);
>>
>
> ...
>
>> @@ -576,13 +518,12 @@ 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;
>> - void __rcu **srv_slot;
>> + struct qrtr_server *srv;
>
> This breaks reverse xmas tree ordering of local variables.
> The srv line should be directly above rather than below the node line.
>
>> + unsigned long node_idx;
>> + unsigned long srv_idx;
>>
>> /* Accept only local observers */
>> if (from->sq_node != qrtr_ns.local_node)
>
> ...