Review Request

Review Request

[Divya Indi]

[PATCH v2] IB/sa: Resolving use-after-free in ib_nl_send_msg.

Hi,

This is the v2 of the patch that addresses the comments received for v1 -

- Using atomic bit ops for setting and testing IB_SA_NL_QUERY_SENT.
- Rewording and adding comments.


Thanks,
Divya

[PATCH] IB/sa: Resolving use-after-free in ib_nl_send_msg.

[Divya Indi]

This patch fixes commit -
commit 3ebd2fd0d011 ("IB/sa: Put netlink request into the request list before sending")'

Above commit adds the query to the request list before ib_nl_snd_msg.

However, if there is a delay in sending out the request (For
eg: Delay due to low memory situation) the timer to handle request timeout
might kick in before the request is sent out to ibacm via netlink.
ib_nl_request_timeout may release the query if it fails to send it to SA
as well causing a use after free situation.

Call Trace for the above race:

[<ffffffffa02f43cb>] ? ib_pack+0x17b/0x240 [ib_core]
[<ffffffffa032aef1>] ib_sa_path_rec_get+0x181/0x200 [ib_sa]
[<ffffffffa0379db0>] rdma_resolve_route+0x3c0/0x8d0 [rdma_cm]
[<ffffffffa0374450>] ? cma_bind_port+0xa0/0xa0 [rdma_cm]
[<ffffffffa040f850>] ? rds_rdma_cm_event_handler_cmn+0x850/0x850
[rds_rdma]
[<ffffffffa040f22c>] rds_rdma_cm_event_handler_cmn+0x22c/0x850
[rds_rdma]
[<ffffffffa040f860>] rds_rdma_cm_event_handler+0x10/0x20 [rds_rdma]
[<ffffffffa037778e>] addr_handler+0x9e/0x140 [rdma_cm]
[<ffffffffa026cdb4>] process_req+0x134/0x190 [ib_addr]
[<ffffffff810a02f9>] process_one_work+0x169/0x4a0
[<ffffffff810a0b2b>] worker_thread+0x5b/0x560
[<ffffffff810a0ad0>] ? flush_delayed_work+0x50/0x50
[<ffffffff810a68fb>] kthread+0xcb/0xf0
[<ffffffff816ec49a>] ? __schedule+0x24a/0x810
[<ffffffff816ec49a>] ? __schedule+0x24a/0x810
[<ffffffff810a6830>] ? kthread_create_on_node+0x180/0x180
[<ffffffff816f25a7>] ret_from_fork+0x47/0x90
[<ffffffff810a6830>] ? kthread_create_on_node+0x180/0x180
....
RIP  [<ffffffffa03296cd>] send_mad+0x33d/0x5d0 [ib_sa]

To resolve this issue, we introduce a new flag IB_SA_NL_QUERY_SENT.
This flag Indicates if the request has been sent out to ibacm yet.

If this flag is not set for a query and the query times out, we add it
back to the list with the original delay.

To handle the case where a response is received before we could set this
flag, the response handler waits for the flag to be
set before proceeding with the query.

Signed-off-by: Divya Indi <divya.indi@oracle.com>
---
 drivers/infiniband/core/sa_query.c | 53 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/drivers/infiniband/core/sa_query.c b/drivers/infiniband/core/sa_query.c
index 30d4c12..91486e7 100644
--- a/drivers/infiniband/core/sa_query.c
+++ b/drivers/infiniband/core/sa_query.c
@@ -42,6 +42,7 @@
 #include <linux/kref.h>
 #include <linux/xarray.h>
 #include <linux/workqueue.h>
+#include <linux/bitops.h>
 #include <uapi/linux/if_ether.h>
 #include <rdma/ib_pack.h>
 #include <rdma/ib_cache.h>
@@ -59,6 +60,9 @@
 #define IB_SA_LOCAL_SVC_TIMEOUT_MAX		200000
 #define IB_SA_CPI_MAX_RETRY_CNT			3
 #define IB_SA_CPI_RETRY_WAIT			1000 /*msecs */
+
+DECLARE_WAIT_QUEUE_HEAD(wait_queue);
+
 static int sa_local_svc_timeout_ms = IB_SA_LOCAL_SVC_TIMEOUT_DEFAULT;
 
 struct ib_sa_sm_ah {
@@ -123,6 +127,12 @@ struct ib_sa_query {
 #define IB_SA_CANCEL			0x00000002
 #define IB_SA_QUERY_OPA			0x00000004
 
+/*
+ * This bit in the SA query flags indicates whether the query
+ * has been sent out to ibacm via netlink
+ */
+#define IB_SA_NL_QUERY_SENT		3
+
 struct ib_sa_service_query {
 	void (*callback)(int, struct ib_sa_service_rec *, void *);
 	void *context;
@@ -746,6 +756,11 @@ static inline int ib_sa_query_cancelled(struct ib_sa_query *query)
 	return (query->flags & IB_SA_CANCEL);
 }
 
+static inline int ib_sa_nl_query_sent(struct ib_sa_query *query)
+{
+	return test_bit(IB_SA_NL_QUERY_SENT, (unsigned long *)&query->flags);
+}
+
 static void ib_nl_set_path_rec_attrs(struct sk_buff *skb,
 				     struct ib_sa_query *query)
 {
@@ -889,6 +904,15 @@ static int ib_nl_make_request(struct ib_sa_query *query, gfp_t gfp_mask)
 		spin_lock_irqsave(&ib_nl_request_lock, flags);
 		list_del(&query->list);
 		spin_unlock_irqrestore(&ib_nl_request_lock, flags);
+	} else {
+		set_bit(IB_SA_NL_QUERY_SENT, (unsigned long *)&query->flags);
+
+		/*
+		 * If response is received before this flag was set
+		 * someone is waiting to process the response and release the
+		 * query.
+		 */
+		wake_up(&wait_queue);
 	}
 
 	return ret;
@@ -994,6 +1018,21 @@ static void ib_nl_request_timeout(struct work_struct *work)
 		}
 
 		list_del(&query->list);
+
+		/*
+		 * If IB_SA_NL_QUERY_SENT is not set, this query has not been
+		 * sent to ibacm yet. Reset the timer.
+		 */
+		if (!ib_sa_nl_query_sent(query)) {
+			delay = msecs_to_jiffies(sa_local_svc_timeout_ms);
+			query->timeout = delay + jiffies;
+			list_add_tail(&query->list, &ib_nl_request_list);
+			/* Start the timeout if this is the only request */
+			if (ib_nl_request_list.next == &query->list)
+				queue_delayed_work(ib_nl_wq, &ib_nl_timed_work,
+						delay);
+			break;
+		}
 		ib_sa_disable_local_svc(query);
 		/* Hold the lock to protect against query cancellation */
 		if (ib_sa_query_cancelled(query))
@@ -1122,6 +1161,20 @@ int ib_nl_handle_resolve_resp(struct sk_buff *skb,
 	}
 
 	send_buf = query->mad_buf;
+	/*
+	 * In case of a quick response ie when a response comes in before
+	 * setting IB_SA_NL_QUERY_SENT, we can have an unlikely race where the
+	 * response handler will release the query, but we can still access the
+	 * freed query while setting the flag.
+	 * Hence, before proceeding and eventually releasing the query -
+	 * wait till the flag is set. The flag should be set soon since getting
+	 * a response is indicative of having successfully sent the query.
+	 */
+	if (unlikely(!ib_sa_nl_query_sent(query))) {
+		spin_unlock_irqrestore(&ib_nl_request_lock, flags);
+		wait_event(wait_queue, ib_sa_nl_query_sent(query));
+		spin_lock_irqsave(&ib_nl_request_lock, flags);
+	}
 
 	if (!ib_nl_is_good_resolve_resp(nlh)) {
 		/* if the result is a failure, send out the packet via IB */
-- 
1.8.3.1

Re: [PATCH] IB/sa: Resolving use-after-free in ib_nl_send_msg.

[Jason Gunthorpe]

On Thu, May 14, 2020 at 08:11:24AM -0700, Divya Indi wrote:
>  static void ib_nl_set_path_rec_attrs(struct sk_buff *skb,
>  				     struct ib_sa_query *query)
>  {
> @@ -889,6 +904,15 @@ static int ib_nl_make_request(struct ib_sa_query *query, gfp_t gfp_mask)
>  		spin_lock_irqsave(&ib_nl_request_lock, flags);
>  		list_del(&query->list);
>  		spin_unlock_irqrestore(&ib_nl_request_lock, flags);
> +	} else {
> +		set_bit(IB_SA_NL_QUERY_SENT, (unsigned long *)&query->flags);
> +
> +		/*
> +		 * If response is received before this flag was set
> +		 * someone is waiting to process the response and release the
> +		 * query.
> +		 */
> +		wake_up(&wait_queue);
>  	}

As far as I can see the issue here is that the request is put into the
ib_nl_request_list before it is really ready to be in that list, eg
ib_nl_send_msg() has actually completed and ownership of the memory
has been transfered.

It appears to me the reason for this is simply because a spinlock is
used for the ib_nl_request_lock and it cannot be held across
ib_nl_send_msg().

Convert that lock to a mutex and move the list_add to after the
success of ib_nl_send_msg() and this bug should be fixed without
adding jaunty atomics or a wait queue.

This is a 'racy error unwind' bug class...

Jason

Re: Review Request

[Divya Indi]

Thanks Leon, Noted!

On 6/9/20 12:03 AM, Leon Romanovsky wrote:
> On Mon, Jun 08, 2020 at 07:46:15AM -0700, Divya Indi wrote:
>> [PATCH v3] IB/sa: Resolving use-after-free in ib_nl_send_msg
>>
>> Hi,
>>
>> Please review the patch that follows.
> Please read Documentation/process/submitting-patches.rst
> 14) The canonical patch format
>
> You don't need an extra email "Review request" and Changelog should be
> put inside the patch itself under "---" marker.
>
> Thanks

Re: Review Request

[Leon Romanovsky]

On Mon, Jun 08, 2020 at 07:46:15AM -0700, Divya Indi wrote:
> [PATCH v3] IB/sa: Resolving use-after-free in ib_nl_send_msg
>
> Hi,
>
> Please review the patch that follows.

Please read Documentation/process/submitting-patches.rst
14) The canonical patch format

You don't need an extra email "Review request" and Changelog should be
put inside the patch itself under "---" marker.

Thanks

Review Request

[Divya Indi]

[PATCH v3] IB/sa: Resolving use-after-free in ib_nl_send_msg

Hi,

Please review the patch that follows. 

v3 addresses the previously raised concerns. Changes include -

1. To resolve the race where the timer can kick in before request
has been sent out, we now add the request to the list after sending out
the request.

2. To handle the race where the response can come in before we got a 
chance to add the req to the list, sending and adding the request to
request list is done under spinlock - request_lock.

3. To make sure there is no blocking op/delay while holding the spinlock,
using GFP_NOWAIT for memory allocation.

Thanks Jason for providing your valuable feedback. 

Let me know if you have any suggestions or concerns.

Thanks,
Divya