From 5f32de8fb10bd50a7b4b597076a86eb0da7a61da Mon Sep 17 00:00:00 2001 From: Chris Lew Date: Wed, 17 Jun 2020 17:32:42 -0700 Subject: [PATCH 1/2] 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 | 14 ++++++-------- 1 file changed, 6 insertions(+), 8 deletions(-) diff --git a/net/qrtr/ns.c b/net/qrtr/ns.c index e933450bcd5f..bd687ad3cb01 100644 --- a/net/qrtr/ns.c +++ b/net/qrtr/ns.c @@ -11,6 +11,7 @@ #include #include #include +#include #include #include "qrtr.h" @@ -22,7 +23,7 @@ static void *ns_ilc; #define NS_INFO(x, ...) ipc_log_string(ns_ilc, x, ##__VA_ARGS__) -static RADIX_TREE(nodes, GFP_KERNEL); +static DEFINE_XARRAY(nodes); static struct { struct socket *sock; @@ -79,7 +80,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; @@ -90,7 +91,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; } @@ -545,12 +546,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 */ @@ -570,9 +570,7 @@ static int ctrl_cmd_new_lookup(struct sockaddr_qrtr *from, filter.service = service; filter.instance = instance; - radix_tree_for_each_slot(node_slot, &nodes, &node_iter, 0) { - node = radix_tree_deref_slot(node_slot); - + xa_for_each(&nodes, node_idx, node) { radix_tree_for_each_slot(srv_slot, &node->servers, &srv_iter, 0) { struct qrtr_server *srv; From 5dc5204c4988d58af5c505b03118ffa61649814c Mon Sep 17 00:00:00 2001 From: Chris Lew Date: Thu, 18 Jun 2020 13:35:08 -0700 Subject: [PATCH 2/2] 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 | 54 +++++++++++++++++++++------------------------------ 1 file changed, 22 insertions(+), 32 deletions(-) diff --git a/net/qrtr/ns.c b/net/qrtr/ns.c index bd687ad3cb01..e109a6773bfa 100644 --- a/net/qrtr/ns.c +++ b/net/qrtr/ns.c @@ -73,7 +73,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) @@ -90,6 +90,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); @@ -202,10 +203,9 @@ 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); @@ -213,9 +213,7 @@ static int announce_servers(struct sockaddr_qrtr *sq) return 0; /* 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); - + xa_for_each(&node->servers, index, srv) { ret = service_announce_new(sq, srv); if (ret < 0) { if (ret == -ENODEV) @@ -255,14 +253,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); @@ -282,11 +283,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) @@ -346,13 +347,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; @@ -364,8 +364,7 @@ static int ctrl_cmd_bye(struct sockaddr_qrtr *from) return 0; /* 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); + xa_for_each(&node->servers, index, srv) { server_del(node, srv->port); } @@ -378,9 +377,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); - radix_tree_for_each_slot(slot, &local_node->servers, &iter, 0) { - srv = radix_tree_deref_slot(slot); - + xa_for_each(&local_node->servers, index, srv) { sq.sq_family = AF_QIPCRTR; sq.sq_node = srv->node; sq.sq_port = srv->port; @@ -401,7 +398,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 = { }; @@ -410,7 +406,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; @@ -452,9 +448,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); - radix_tree_for_each_slot(slot, &local_node->servers, &iter, 0) { - srv = radix_tree_deref_slot(slot); - + xa_for_each(&local_node->servers, index, srv) { sq.sq_family = AF_QIPCRTR; sq.sq_node = srv->node; sq.sq_port = srv->port; @@ -547,11 +541,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) @@ -571,11 +565,7 @@ 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); + xa_for_each(&node->servers, srv_idx, srv) { if (!server_match(srv, &filter)) continue;