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 <[email protected]>
---
drivers/infiniband/core/sa_query.c | 45 ++++++++++++++++++++++++++++++++++++++
1 file changed, 45 insertions(+)
diff --git a/drivers/infiniband/core/sa_query.c b/drivers/infiniband/core/sa_query.c
index 30d4c12..ffbae2f 100644
--- a/drivers/infiniband/core/sa_query.c
+++ b/drivers/infiniband/core/sa_query.c
@@ -59,6 +59,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 {
@@ -122,6 +125,7 @@ struct ib_sa_query {
#define IB_SA_ENABLE_LOCAL_SERVICE 0x00000001
#define IB_SA_CANCEL 0x00000002
#define IB_SA_QUERY_OPA 0x00000004
+#define IB_SA_NL_QUERY_SENT 0x00000008
struct ib_sa_service_query {
void (*callback)(int, struct ib_sa_service_rec *, void *);
@@ -746,6 +750,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 (query->flags & IB_SA_NL_QUERY_SENT);
+}
+
static void ib_nl_set_path_rec_attrs(struct sk_buff *skb,
struct ib_sa_query *query)
{
@@ -889,6 +898,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 {
+ query->flags |= IB_SA_NL_QUERY_SENT;
+
+ /*
+ * 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 +1012,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))
@@ -1123,6 +1156,18 @@ int ib_nl_handle_resolve_resp(struct sk_buff *skb,
send_buf = query->mad_buf;
+ /*
+ * Make sure the IB_SA_NL_QUERY_SENT flag is set before
+ * processing this query. If flag is not set, query can be accessed in
+ * another context while setting the flag and processing the query will
+ * eventually release it causing a possible use-after-free.
+ */
+ 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 */
ib_sa_disable_local_svc(query);
--
1.8.3.1
> 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 <[email protected]>
> ---
> drivers/infiniband/core/sa_query.c | 45
> ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 45 insertions(+)
>
> diff --git a/drivers/infiniband/core/sa_query.c
> b/drivers/infiniband/core/sa_query.c
> index 30d4c12..ffbae2f 100644
> --- a/drivers/infiniband/core/sa_query.c
> +++ b/drivers/infiniband/core/sa_query.c
> @@ -59,6 +59,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 {
> @@ -122,6 +125,7 @@ struct ib_sa_query {
> #define IB_SA_ENABLE_LOCAL_SERVICE 0x00000001
> #define IB_SA_CANCEL 0x00000002
> #define IB_SA_QUERY_OPA 0x00000004
> +#define IB_SA_NL_QUERY_SENT 0x00000008
>
> struct ib_sa_service_query {
> void (*callback)(int, struct ib_sa_service_rec *, void *); @@ -746,6
> +750,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 (query->flags & IB_SA_NL_QUERY_SENT); }
> +
> static void ib_nl_set_path_rec_attrs(struct sk_buff *skb,
> struct ib_sa_query *query)
> {
> @@ -889,6 +898,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 {
> + query->flags |= IB_SA_NL_QUERY_SENT;
> +
> + /*
> + * 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 +1012,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))
> @@ -1123,6 +1156,18 @@ int ib_nl_handle_resolve_resp(struct sk_buff
> *skb,
>
> send_buf = query->mad_buf;
>
> + /*
> + * Make sure the IB_SA_NL_QUERY_SENT flag is set before
> + * processing this query. If flag is not set, query can be accessed in
> + * another context while setting the flag and processing the query
> will
> + * eventually release it causing a possible use-after-free.
> + */
> + 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 */
> ib_sa_disable_local_svc(query);
> --
> 1.8.3.1
Reviewd-by: Kaike Wan <[email protected]>
On 5/7/2020 11:34, Divya Indi wrote:
> 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 <[email protected]>
> ---
> drivers/infiniband/core/sa_query.c | 45 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 45 insertions(+)
>
> diff --git a/drivers/infiniband/core/sa_query.c b/drivers/infiniband/core/sa_query.c
> index 30d4c12..ffbae2f 100644
> --- a/drivers/infiniband/core/sa_query.c
> +++ b/drivers/infiniband/core/sa_query.c
> @@ -59,6 +59,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 {
> @@ -122,6 +125,7 @@ struct ib_sa_query {
> #define IB_SA_ENABLE_LOCAL_SERVICE 0x00000001
> #define IB_SA_CANCEL 0x00000002
> #define IB_SA_QUERY_OPA 0x00000004
> +#define IB_SA_NL_QUERY_SENT 0x00000008
>
> struct ib_sa_service_query {
> void (*callback)(int, struct ib_sa_service_rec *, void *);
> @@ -746,6 +750,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 (query->flags & IB_SA_NL_QUERY_SENT);
> +}
> +
> static void ib_nl_set_path_rec_attrs(struct sk_buff *skb,
> struct ib_sa_query *query)
> {
> @@ -889,6 +898,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 {
> + query->flags |= IB_SA_NL_QUERY_SENT;
> +
> + /*
> + * 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 +1012,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))
> @@ -1123,6 +1156,18 @@ int ib_nl_handle_resolve_resp(struct sk_buff *skb,
>
> send_buf = query->mad_buf;
>
> + /*
> + * Make sure the IB_SA_NL_QUERY_SENT flag is set before
> + * processing this query. If flag is not set, query can be accessed in
> + * another context while setting the flag and processing the query will
> + * eventually release it causing a possible use-after-free.
> + */
> + if (unlikely(!ib_sa_nl_query_sent(query))) {
Can't there be a race here where you check the flag (it isn't set)
and before you call wait_event() the flag is set and wake_up() is called
which means you will wait here forever? or worse, a timeout will happen
the query will be freed and them some other query will call wake_up()
and we have again a use-after-free.
> + spin_unlock_irqrestore(&ib_nl_request_lock, flags);
> + wait_event(wait_queue, ib_sa_nl_query_sent(query));
What if there are two queries sent to userspace, shouldn't you check and make sure
you got woken up by the right one setting the flag?
Other than that, the entire solution makes it very complicated to reason with (flags set/checked without locking etc)
maybe we should just revert and fix it the other way?
Mark
> + 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 */
> ib_sa_disable_local_svc(query);
>
> -----Original Message-----
> From: Mark Bloch <[email protected]>
> Sent: Thursday, May 07, 2020 3:36 PM
> To: Divya Indi <[email protected]>; [email protected]; linux-
> [email protected]; Jason Gunthorpe <[email protected]>; Wan, Kaike
> <[email protected]>
> Cc: Gerd Rausch <[email protected]>; Håkon Bugge
> <[email protected]>; Srinivas Eeda <[email protected]>;
> Rama Nichanamatlu <[email protected]>; Doug Ledford
> <[email protected]>
> Subject: Re: [PATCH 1/2] IB/sa: Resolving use-after-free in ib_nl_send_msg.
>
>
> > @@ -1123,6 +1156,18 @@ int ib_nl_handle_resolve_resp(struct sk_buff
> > *skb,
> >
> > send_buf = query->mad_buf;
> >
> > + /*
> > + * Make sure the IB_SA_NL_QUERY_SENT flag is set before
> > + * processing this query. If flag is not set, query can be accessed in
> > + * another context while setting the flag and processing the query
> will
> > + * eventually release it causing a possible use-after-free.
> > + */
> > + if (unlikely(!ib_sa_nl_query_sent(query))) {
>
> Can't there be a race here where you check the flag (it isn't set) and before
> you call wait_event() the flag is set and wake_up() is called which means you
> will wait here forever?
Should wait_event() catch that? That is, if the flag is not set, wait_event() will sleep until the flag is set.
or worse, a timeout will happen the query will be
> freed and them some other query will call wake_up() and we have again a
> use-after-free.
The request has been deleted from the request list by this time and therefore the timeout should have no impact here.
>
> > + spin_unlock_irqrestore(&ib_nl_request_lock, flags);
> > + wait_event(wait_queue, ib_sa_nl_query_sent(query));
>
> What if there are two queries sent to userspace, shouldn't you check and
> make sure you got woken up by the right one setting the flag?
The wait_event() is conditioned on the specific query (ib_sa_nl_query_sent(query)), not on the wait_queue itself.
>
> Other than that, the entire solution makes it very complicated to reason with
> (flags set/checked without locking etc) maybe we should just revert and fix it
> the other way?
The flag could certainly be set under the lock, which may reduce complications.
Kaike
On 5/7/2020 13:16, Wan, Kaike wrote:
>
>
>> -----Original Message-----
>> From: Mark Bloch <[email protected]>
>> Sent: Thursday, May 07, 2020 3:36 PM
>> To: Divya Indi <[email protected]>; [email protected]; linux-
>> [email protected]; Jason Gunthorpe <[email protected]>; Wan, Kaike
>> <[email protected]>
>> Cc: Gerd Rausch <[email protected]>; Håkon Bugge
>> <[email protected]>; Srinivas Eeda <[email protected]>;
>> Rama Nichanamatlu <[email protected]>; Doug Ledford
>> <[email protected]>
>> Subject: Re: [PATCH 1/2] IB/sa: Resolving use-after-free in ib_nl_send_msg.
>>
>>
>>> @@ -1123,6 +1156,18 @@ int ib_nl_handle_resolve_resp(struct sk_buff
>>> *skb,
>>>
>>> send_buf = query->mad_buf;
>>>
>>> + /*
>>> + * Make sure the IB_SA_NL_QUERY_SENT flag is set before
>>> + * processing this query. If flag is not set, query can be accessed in
>>> + * another context while setting the flag and processing the query
>> will
>>> + * eventually release it causing a possible use-after-free.
>>> + */
>>> + if (unlikely(!ib_sa_nl_query_sent(query))) {
>>
>> Can't there be a race here where you check the flag (it isn't set) and before
>> you call wait_event() the flag is set and wake_up() is called which means you
>> will wait here forever?
>
> Should wait_event() catch that? That is, if the flag is not set, wait_event() will sleep until the flag is set.
>
> or worse, a timeout will happen the query will be
>> freed and them some other query will call wake_up() and we have again a
>> use-after-free.
>
> The request has been deleted from the request list by this time and therefore the timeout should have no impact here.
>
>
>>
>>> + spin_unlock_irqrestore(&ib_nl_request_lock, flags);
>>> + wait_event(wait_queue, ib_sa_nl_query_sent(query));
>>
>> What if there are two queries sent to userspace, shouldn't you check and
>> make sure you got woken up by the right one setting the flag?
>
> The wait_event() is conditioned on the specific query (ib_sa_nl_query_sent(query)), not on the wait_queue itself.
Right, missed that this macro is expends into some inline code.
Looking at the code a little more, I think this also fixes another issue.
Lets say ib_nl_send_msg() returns an error but before we do the list_del() in
ib_nl_make_request() there is also a timeout, so in ib_nl_request_timeout()
we will do list_del() and then another one list_del() will be done in ib_nl_make_request().
>
>>
>> Other than that, the entire solution makes it very complicated to reason with
>> (flags set/checked without locking etc) maybe we should just revert and fix it
>> the other way?
>
> The flag could certainly be set under the lock, which may reduce complications.
Anything that can help here with this.
For me in ib_nl_make_request() the comment should also explain that not only ib_nl_handle_resolve_resp()
is waiting for the flag to be set but also ib_nl_request_timeout() and that a timeout can't happen
before the flag is set.
Mark
>
> Kaike
>
On Thu, May 07, 2020 at 11:34:47AM -0700, Divya Indi wrote:
> 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 <[email protected]>
> drivers/infiniband/core/sa_query.c | 45 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 45 insertions(+)
>
> diff --git a/drivers/infiniband/core/sa_query.c b/drivers/infiniband/core/sa_query.c
> index 30d4c12..ffbae2f 100644
> +++ b/drivers/infiniband/core/sa_query.c
> @@ -59,6 +59,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 {
> @@ -122,6 +125,7 @@ struct ib_sa_query {
> #define IB_SA_ENABLE_LOCAL_SERVICE 0x00000001
> #define IB_SA_CANCEL 0x00000002
> #define IB_SA_QUERY_OPA 0x00000004
> +#define IB_SA_NL_QUERY_SENT 0x00000008
>
> struct ib_sa_service_query {
> void (*callback)(int, struct ib_sa_service_rec *, void *);
> @@ -746,6 +750,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 (query->flags & IB_SA_NL_QUERY_SENT);
> +}
> +
> static void ib_nl_set_path_rec_attrs(struct sk_buff *skb,
> struct ib_sa_query *query)
> {
> @@ -889,6 +898,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 {
> + query->flags |= IB_SA_NL_QUERY_SENT;
> +
> + /*
> + * 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 +1012,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))
> @@ -1123,6 +1156,18 @@ int ib_nl_handle_resolve_resp(struct sk_buff *skb,
>
> send_buf = query->mad_buf;
>
> + /*
> + * Make sure the IB_SA_NL_QUERY_SENT flag is set before
> + * processing this query. If flag is not set, query can be accessed in
> + * another context while setting the flag and processing the query will
> + * eventually release it causing a possible use-after-free.
> + */
This comment doesn't really make sense, flags insige the memory being
freed inherently can't prevent use after free.
Jason
Hi,
Thanks for taking the time to review. Please find my comments inline -
On 5/7/20 1:16 PM, Wan, Kaike wrote:
>
>> -----Original Message-----
>> From: Mark Bloch <[email protected]>
>> Sent: Thursday, May 07, 2020 3:36 PM
>> To: Divya Indi <[email protected]>; [email protected]; linux-
>> [email protected]; Jason Gunthorpe <[email protected]>; Wan, Kaike
>> <[email protected]>
>> Cc: Gerd Rausch <[email protected]>; Håkon Bugge
>> <[email protected]>; Srinivas Eeda <[email protected]>;
>> Rama Nichanamatlu <[email protected]>; Doug Ledford
>> <[email protected]>
>> Subject: Re: [PATCH 1/2] IB/sa: Resolving use-after-free in ib_nl_send_msg.
>>
>>
>>> @@ -1123,6 +1156,18 @@ int ib_nl_handle_resolve_resp(struct sk_buff
>>> *skb,
>>>
>>> send_buf = query->mad_buf;
>>>
>>> + /*
>>> + * Make sure the IB_SA_NL_QUERY_SENT flag is set before
>>> + * processing this query. If flag is not set, query can be accessed in
>>> + * another context while setting the flag and processing the query
>> will
>>> + * eventually release it causing a possible use-after-free.
>>> + */
>>> + if (unlikely(!ib_sa_nl_query_sent(query))) {
>> Can't there be a race here where you check the flag (it isn't set) and before
>> you call wait_event() the flag is set and wake_up() is called which means you
>> will wait here forever?
> Should wait_event() catch that? That is, if the flag is not set, wait_event() will sleep until the flag is set.
>
> or worse, a timeout will happen the query will be
>> freed and them some other query will call wake_up() and we have again a
>> use-after-free.
> The request has been deleted from the request list by this time and therefore the timeout should have no impact here.
>
>
>>> + spin_unlock_irqrestore(&ib_nl_request_lock, flags);
>>> + wait_event(wait_queue, ib_sa_nl_query_sent(query));
>> What if there are two queries sent to userspace, shouldn't you check and
>> make sure you got woken up by the right one setting the flag?
> The wait_event() is conditioned on the specific query (ib_sa_nl_query_sent(query)), not on the wait_queue itself.
>
>> Other than that, the entire solution makes it very complicated to reason with
>> (flags set/checked without locking etc) maybe we should just revert and fix it
>> the other way?
> The flag could certainly be set under the lock, which may reduce complications.
We could use a lock or use atomic operations. However, the reason for not doing so was that
we have 1 writer and multiple readers of the IB_SA_NL_QUERY_SENT flag and the readers
wouldnt mind reading a stale value.
Would it still be better to have a lock for this flag?
Thanks,
Divya
>
> Kaike
>
Hi Mark,
Please find my comments inline -
On 5/7/20 2:40 PM, Mark Bloch wrote:
>
> On 5/7/2020 13:16, Wan, Kaike wrote:
>>
>>> -----Original Message-----
>>> From: Mark Bloch <[email protected]>
>>> Sent: Thursday, May 07, 2020 3:36 PM
>>> To: Divya Indi <[email protected]>; [email protected]; linux-
>>> [email protected]; Jason Gunthorpe <[email protected]>; Wan, Kaike
>>> <[email protected]>
>>> Cc: Gerd Rausch <[email protected]>; Håkon Bugge
>>> <[email protected]>; Srinivas Eeda <[email protected]>;
>>> Rama Nichanamatlu <[email protected]>; Doug Ledford
>>> <[email protected]>
>>> Subject: Re: [PATCH 1/2] IB/sa: Resolving use-after-free in ib_nl_send_msg.
>>>
>>>
>>>> @@ -1123,6 +1156,18 @@ int ib_nl_handle_resolve_resp(struct sk_buff
>>>> *skb,
>>>>
>>>> send_buf = query->mad_buf;
>>>>
>>>> + /*
>>>> + * Make sure the IB_SA_NL_QUERY_SENT flag is set before
>>>> + * processing this query. If flag is not set, query can be accessed in
>>>> + * another context while setting the flag and processing the query
>>> will
>>>> + * eventually release it causing a possible use-after-free.
>>>> + */
>>>> + if (unlikely(!ib_sa_nl_query_sent(query))) {
>>> Can't there be a race here where you check the flag (it isn't set) and before
>>> you call wait_event() the flag is set and wake_up() is called which means you
>>> will wait here forever?
>> Should wait_event() catch that? That is, if the flag is not set, wait_event() will sleep until the flag is set.
>>
>> or worse, a timeout will happen the query will be
>>> freed and them some other query will call wake_up() and we have again a
>>> use-after-free.
>> The request has been deleted from the request list by this time and therefore the timeout should have no impact here.
>>
>>
>>>> + spin_unlock_irqrestore(&ib_nl_request_lock, flags);
>>>> + wait_event(wait_queue, ib_sa_nl_query_sent(query));
>>> What if there are two queries sent to userspace, shouldn't you check and
>>> make sure you got woken up by the right one setting the flag?
>> The wait_event() is conditioned on the specific query (ib_sa_nl_query_sent(query)), not on the wait_queue itself.
> Right, missed that this macro is expends into some inline code.
>
> Looking at the code a little more, I think this also fixes another issue.
> Lets say ib_nl_send_msg() returns an error but before we do the list_del() in
> ib_nl_make_request() there is also a timeout, so in ib_nl_request_timeout()
> we will do list_del() and then another one list_del() will be done in ib_nl_make_request().
>
>>> Other than that, the entire solution makes it very complicated to reason with
>>> (flags set/checked without locking etc) maybe we should just revert and fix it
>>> the other way?
>> The flag could certainly be set under the lock, which may reduce complications.
> Anything that can help here with this.
> For me in ib_nl_make_request() the comment should also explain that not only ib_nl_handle_resolve_resp()
> is waiting for the flag to be set but also ib_nl_request_timeout() and that a timeout can't happen
> before the flag is set.
ib_nl_request_timeout() would re-queue the query to the request list if the flag is not set.
However, makes sense! Noted, il add the comment in ib_nl_make_request to make things more clear.
Thanks,
Divya
> Mark
>
>> Kaike
>> i
Hi Jason,
On 5/7/20 5:08 PM, Jason Gunthorpe wrote:
> On Thu, May 07, 2020 at 11:34:47AM -0700, Divya Indi wrote:
>> 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 <[email protected]>
>> drivers/infiniband/core/sa_query.c | 45 ++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 45 insertions(+)
>>
>> diff --git a/drivers/infiniband/core/sa_query.c b/drivers/infiniband/core/sa_query.c
>> index 30d4c12..ffbae2f 100644
>> +++ b/drivers/infiniband/core/sa_query.c
>> @@ -59,6 +59,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 {
>> @@ -122,6 +125,7 @@ struct ib_sa_query {
>> #define IB_SA_ENABLE_LOCAL_SERVICE 0x00000001
>> #define IB_SA_CANCEL 0x00000002
>> #define IB_SA_QUERY_OPA 0x00000004
>> +#define IB_SA_NL_QUERY_SENT 0x00000008
>>
>> struct ib_sa_service_query {
>> void (*callback)(int, struct ib_sa_service_rec *, void *);
>> @@ -746,6 +750,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 (query->flags & IB_SA_NL_QUERY_SENT);
>> +}
>> +
>> static void ib_nl_set_path_rec_attrs(struct sk_buff *skb,
>> struct ib_sa_query *query)
>> {
>> @@ -889,6 +898,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 {
>> + query->flags |= IB_SA_NL_QUERY_SENT;
>> +
>> + /*
>> + * 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 +1012,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))
>> @@ -1123,6 +1156,18 @@ int ib_nl_handle_resolve_resp(struct sk_buff *skb,
>>
>> send_buf = query->mad_buf;
>>
>> + /*
>> + * Make sure the IB_SA_NL_QUERY_SENT flag is set before
>> + * processing this query. If flag is not set, query can be accessed in
>> + * another context while setting the flag and processing the query will
>> + * eventually release it causing a possible use-after-free.
>> + */
> This comment doesn't really make sense, flags insige the memory being
> freed inherently can't prevent use after free.
I can definitely re-phrase here to make things clearer. But, the idea here is
in the unlikely/rare case where a response for a query comes in before the flag has been
set in ib_nl_make_request, we want to wait for the flag to be sent before proceeding.
The response handler will eventually release the query so this wait avoids that if the flag has not been set
else
"query->flags |= IB_SA_NL_QUERY_SENT;"
will be accessing a query which was freed due to the above mentioned race.
It is unlikely since getting a response => We have actually sent out the query to ibacm.
How about this -
"Getting a response is indicative of having sent out the query, but in an unlikely race when
the response comes in before setting IB_SA_NL_QUERY_SENT, we need to wait till the flag is set to
avoid accessing a query that has been released."
Thoughts?
Thanks,
Divya
Jason
Hi Hillf,
Please find my comments inline -
On 5/8/20 4:03 AM, Hillf Danton wrote:
> On Thu, 7 May 2020 12:36:29 Mark Bloch wrote:
>> On 5/7/2020 11:34, Divya Indi wrote:
>>> 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 <[email protected]>
>>> ---
>>> drivers/infiniband/core/sa_query.c | 45 ++++++++++++++++++++++++++++++++++++++
>>> 1 file changed, 45 insertions(+)
>>>
>>> diff --git a/drivers/infiniband/core/sa_query.c b/drivers/infiniband/core/sa_query.c
>>> index 30d4c12..ffbae2f 100644
>>> --- a/drivers/infiniband/core/sa_query.c
>>> +++ b/drivers/infiniband/core/sa_query.c
>>> @@ -59,6 +59,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 {
>>> @@ -122,6 +125,7 @@ struct ib_sa_query {
>>> #define IB_SA_ENABLE_LOCAL_SERVICE 0x00000001
>>> #define IB_SA_CANCEL 0x00000002
>>> #define IB_SA_QUERY_OPA 0x00000004
>>> +#define IB_SA_NL_QUERY_SENT 0x00000008
>>>
>>> struct ib_sa_service_query {
>>> void (*callback)(int, struct ib_sa_service_rec *, void *);
>>> @@ -746,6 +750,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 (query->flags & IB_SA_NL_QUERY_SENT);
>>> +}
>>> +
>>> static void ib_nl_set_path_rec_attrs(struct sk_buff *skb,
>>> struct ib_sa_query *query)
>>> {
>>> @@ -889,6 +898,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);
> Here it also needs to do wakeup as sleeper might come while sending, no
> matter the failure to send.
We only sleep in the response handler. If we could not send a query, then
response handler is not triggered and hence no one would be waiting on this query.
Thanks,
Divya
>>> + } else {
>>> + query->flags |= IB_SA_NL_QUERY_SENT;
>>> +
>>> + /*
>>> + * 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;
> With minor changes added on top of your work it now looks like the diff
> below (only including the wakeup part).
>
> --- a/drivers/infiniband/core/sa_query.c
> +++ b/drivers/infiniband/core/sa_query.c
> @@ -863,6 +863,11 @@ static int ib_nl_send_msg(struct ib_sa_q
> return rdma_nl_multicast(&init_net, skb, RDMA_NL_GROUP_LS, gfp_mask);
> }
>
> +static inline bool ib_sa_nl_query_sending(struct ib_sa_query *query)
> +{
> + return query->flags & IB_SA_NL_QUERY_SENDING;
> +}
> +
> static int ib_nl_make_request(struct ib_sa_query *query, gfp_t gfp_mask)
> {
> unsigned long flags;
> @@ -874,6 +879,8 @@ static int ib_nl_make_request(struct ib_
>
> /* Put the request on the list first.*/
> spin_lock_irqsave(&ib_nl_request_lock, flags);
> + /* info others we're having work to do */
> + query->flags |= IB_SA_NL_QUERY_SENDING;
> delay = msecs_to_jiffies(sa_local_svc_timeout_ms);
> query->timeout = delay + jiffies;
> list_add_tail(&query->list, &ib_nl_request_list);
> @@ -883,13 +890,15 @@ static int ib_nl_make_request(struct ib_
> spin_unlock_irqrestore(&ib_nl_request_lock, flags);
>
> ret = ib_nl_send_msg(query, gfp_mask);
> + /* safe to delete it with IB_SA_NL_QUERY_SENDING set */
> + spin_lock_irqsave(&ib_nl_request_lock, flags);
> if (ret) {
> ret = -EIO;
> - /* Remove the request */
> - spin_lock_irqsave(&ib_nl_request_lock, flags);
> list_del(&query->list);
> - spin_unlock_irqrestore(&ib_nl_request_lock, flags);
> }
> + query->flags &= ~IB_SA_NL_QUERY_SENDING;
> + wake_up(&wait_queue);
> + spin_unlock_irqrestore(&ib_nl_request_lock, flags);
>
> return ret;
> }
>
>
>>> @@ -994,6 +1012,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))
>>> @@ -1123,6 +1156,18 @@ int ib_nl_handle_resolve_resp(struct sk_buff *skb,
>>>
>>> send_buf = query->mad_buf;
>>>
>>> + /*
>>> + * Make sure the IB_SA_NL_QUERY_SENT flag is set before
>>> + * processing this query. If flag is not set, query can be accessed in
>>> + * another context while setting the flag and processing the query will
>>> + * eventually release it causing a possible use-after-free.
>>> + */
>>> + if (unlikely(!ib_sa_nl_query_sent(query))) {
>> Can't there be a race here where you check the flag (it isn't set)
>> and before you call wait_event() the flag is set and wake_up() is called
>> which means you will wait here forever? or worse, a timeout will happen
>> the query will be freed and them some other query will call wake_up()
>> and we have again a use-after-free.
>>
>>> + spin_unlock_irqrestore(&ib_nl_request_lock, flags);
>>> + wait_event(wait_queue, ib_sa_nl_query_sent(query));
>> What if there are two queries sent to userspace, shouldn't you check and make sure
>> you got woken up by the right one setting the flag?
>>
>> Other than that, the entire solution makes it very complicated to reason with (flags set/checked without locking etc)
>> maybe we should just revert and fix it the other way?
>>
>> Mark
>>
>>> + 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 */
>>> ib_sa_disable_local_svc(query);
>>>
> -----Original Message-----
> From: Divya Indi <[email protected]>
> Sent: Monday, May 11, 2020 5:06 PM
> To: Wan, Kaike <[email protected]>; Mark Bloch
> <[email protected]>; [email protected]; linux-
> [email protected]; Jason Gunthorpe <[email protected]>
> Cc: Gerd Rausch <[email protected]>; Håkon Bugge
> <[email protected]>; Srinivas Eeda <[email protected]>;
> Rama Nichanamatlu <[email protected]>; Doug Ledford
> <[email protected]>
> Subject: Re: [PATCH 1/2] IB/sa: Resolving use-after-free in ib_nl_send_msg.
>
> Hi,
>
> Thanks for taking the time to review. Please find my comments inline -
>
> On 5/7/20 1:16 PM, Wan, Kaike wrote:
> >
> >> -----Original Message-----
> >> From: Mark Bloch <[email protected]>
> >> Sent: Thursday, May 07, 2020 3:36 PM
> >> To: Divya Indi <[email protected]>; [email protected];
> >> linux- [email protected]; Jason Gunthorpe <[email protected]>; Wan,
> >> Kaike <[email protected]>
> >> Cc: Gerd Rausch <[email protected]>; Håkon Bugge
> >> <[email protected]>; Srinivas Eeda <[email protected]>;
> >> Rama Nichanamatlu <[email protected]>; Doug Ledford
> >> <[email protected]>
> >> Subject: Re: [PATCH 1/2] IB/sa: Resolving use-after-free in
> ib_nl_send_msg.
> >>
> >>
> >>> @@ -1123,6 +1156,18 @@ int ib_nl_handle_resolve_resp(struct sk_buff
> >>> *skb,
> >>>
> >>> send_buf = query->mad_buf;
> >>>
> >>> + /*
> >>> + * Make sure the IB_SA_NL_QUERY_SENT flag is set before
> >>> + * processing this query. If flag is not set, query can be accessed in
> >>> + * another context while setting the flag and processing the query
> >> will
> >>> + * eventually release it causing a possible use-after-free.
> >>> + */
> >>> + if (unlikely(!ib_sa_nl_query_sent(query))) {
> >> Can't there be a race here where you check the flag (it isn't set)
> >> and before you call wait_event() the flag is set and wake_up() is
> >> called which means you will wait here forever?
> > Should wait_event() catch that? That is, if the flag is not set, wait_event()
> will sleep until the flag is set.
> >
> > or worse, a timeout will happen the query will be
> >> freed and them some other query will call wake_up() and we have again
> >> a use-after-free.
> > The request has been deleted from the request list by this time and
> therefore the timeout should have no impact here.
> >
> >
> >>> + spin_unlock_irqrestore(&ib_nl_request_lock, flags);
> >>> + wait_event(wait_queue, ib_sa_nl_query_sent(query));
> >> What if there are two queries sent to userspace, shouldn't you check
> >> and make sure you got woken up by the right one setting the flag?
> > The wait_event() is conditioned on the specific query
> (ib_sa_nl_query_sent(query)), not on the wait_queue itself.
> >
> >> Other than that, the entire solution makes it very complicated to
> >> reason with (flags set/checked without locking etc) maybe we should
> >> just revert and fix it the other way?
> > The flag could certainly be set under the lock, which may reduce
> complications.
>
> We could use a lock or use atomic operations. However, the reason for not
> doing so was that we have 1 writer and multiple readers of the
> IB_SA_NL_QUERY_SENT flag and the readers wouldnt mind reading a stale
> value.
>
> Would it still be better to have a lock for this flag?
>
Yes. It will make the code less complicated and easier to maintain.
Kaike
On Mon, May 11, 2020 at 02:26:30PM -0700, Divya Indi wrote:
> >> @@ -1123,6 +1156,18 @@ int ib_nl_handle_resolve_resp(struct sk_buff *skb,
> >>
> >> send_buf = query->mad_buf;
> >>
> >> + /*
> >> + * Make sure the IB_SA_NL_QUERY_SENT flag is set before
> >> + * processing this query. If flag is not set, query can be accessed in
> >> + * another context while setting the flag and processing the query will
> >> + * eventually release it causing a possible use-after-free.
> >> + */
> > This comment doesn't really make sense, flags insige the memory being
> > freed inherently can't prevent use after free.
>
> I can definitely re-phrase here to make things clearer. But, the idea here is
> in the unlikely/rare case where a response for a query comes in before the flag has been
> set in ib_nl_make_request, we want to wait for the flag to be sent before proceeding.
> The response handler will eventually release the query so this wait avoids that if the flag has not been set
> else
> "query->flags |= IB_SA_NL_QUERY_SENT;"
> will be accessing a query which was freed due to the above mentioned race.
>
> It is unlikely since getting a response => We have actually sent out the query to ibacm.
>
> How about this -
>
> "Getting a response is indicative of having sent out the query, but in an unlikely race when
> the response comes in before setting IB_SA_NL_QUERY_SENT, we need to wait till the flag is set to
> avoid accessing a query that has been released."
It still makes no sense, a flag that is set before freeing the memory
is fundamentally useless to prevent races.
Jason
Hi Jason,
Please find my comments inline -
On 5/13/20 8:00 AM, Jason Gunthorpe wrote:
> On Mon, May 11, 2020 at 02:26:30PM -0700, Divya Indi wrote:
>>>> @@ -1123,6 +1156,18 @@ int ib_nl_handle_resolve_resp(struct sk_buff *skb,
>>>>
>>>> send_buf = query->mad_buf;
>>>>
>>>> + /*
>>>> + * Make sure the IB_SA_NL_QUERY_SENT flag is set before
>>>> + * processing this query. If flag is not set, query can be accessed in
>>>> + * another context while setting the flag and processing the query will
>>>> + * eventually release it causing a possible use-after-free.
>>>> + */
>>> This comment doesn't really make sense, flags insige the memory being
>>> freed inherently can't prevent use after free.
>> I can definitely re-phrase here to make things clearer. But, the idea here is
>> in the unlikely/rare case where a response for a query comes in before the flag has been
>> set in ib_nl_make_request, we want to wait for the flag to be sent before proceeding.
>> The response handler will eventually release the query so this wait avoids that if the flag has not been set
>> else
>> "query->flags |= IB_SA_NL_QUERY_SENT;"
>> will be accessing a query which was freed due to the above mentioned race.
>>
>> It is unlikely since getting a response => We have actually sent out the query to ibacm.
>>
>> How about this -
>>
>> "Getting a response is indicative of having sent out the query, but in an unlikely race when
>> the response comes in before setting IB_SA_NL_QUERY_SENT, we need to wait till the flag is set to
>> avoid accessing a query that has been released."
> It still makes no sense, a flag that is set before freeing the memory
> is fundamentally useless to prevent races.
Here the race is -
1. ib_nl_send_msg()
2. query->flags |= IB_SA_NL_QUERY_SENT
3. return;
-------------
response_handler() {
wait till flag is set.
....
kfree(query);
}
Here, if the response handler was called => Query was sent
and flag should have been set. However if response handler kicks in
before line 2, we want to wait and make sure the flag is set and
then free the query.
Ideally after ib_nl_send_msg, we should not be accessing the query
because we can be getting a response/timeout asynchronously and cant be
sure when.
The only places we access a query after it is successfully sent [response handler getting called
=> sending was successful] -
1. ib_nl_request_timeout
2. While setting the flag.
1. is taken care of because the request list access is protected by a lock
and whoever gets the lock first deletes it from the request list and
hence we can only have the response handler OR the timeout handler operate on the
query.
2. To handle this is why we wait in the response handler. Once the flag is
set we are no longer accessing the query and hence safe to release it.
I have modified the comment for v2 as follows -
/*
+ * 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.
+ */
Thanks,
Divya
>
> Jason
Hi Jason,
I wanted to follow up to see if you got a chance to review the following reply?
Let me know if it addresses your concern and if you have any questions!
Thanks,
Divya
On 5/13/20 2:02 PM, Divya Indi wrote:
> Hi Jason,
>
> Please find my comments inline -
>
> On 5/13/20 8:00 AM, Jason Gunthorpe wrote:
>> On Mon, May 11, 2020 at 02:26:30PM -0700, Divya Indi wrote:
>>>>> @@ -1123,6 +1156,18 @@ int ib_nl_handle_resolve_resp(struct sk_buff *skb,
>>>>>
>>>>> send_buf = query->mad_buf;
>>>>>
>>>>> + /*
>>>>> + * Make sure the IB_SA_NL_QUERY_SENT flag is set before
>>>>> + * processing this query. If flag is not set, query can be accessed in
>>>>> + * another context while setting the flag and processing the query will
>>>>> + * eventually release it causing a possible use-after-free.
>>>>> + */
>>>> This comment doesn't really make sense, flags insige the memory being
>>>> freed inherently can't prevent use after free.
>>> I can definitely re-phrase here to make things clearer. But, the idea here is
>>> in the unlikely/rare case where a response for a query comes in before the flag has been
>>> set in ib_nl_make_request, we want to wait for the flag to be sent before proceeding.
>>> The response handler will eventually release the query so this wait avoids that if the flag has not been set
>>> else
>>> "query->flags |= IB_SA_NL_QUERY_SENT;"
>>> will be accessing a query which was freed due to the above mentioned race.
>>>
>>> It is unlikely since getting a response => We have actually sent out the query to ibacm.
>>>
>>> How about this -
>>>
>>> "Getting a response is indicative of having sent out the query, but in an unlikely race when
>>> the response comes in before setting IB_SA_NL_QUERY_SENT, we need to wait till the flag is set to
>>> avoid accessing a query that has been released."
>> It still makes no sense, a flag that is set before freeing the memory
>> is fundamentally useless to prevent races.
> Here the race is -
>
> 1. ib_nl_send_msg()
> 2. query->flags |= IB_SA_NL_QUERY_SENT
> 3. return;
>
> -------------
>
> response_handler() {
> wait till flag is set.
> ....
> kfree(query);
>
> }
>
> Here, if the response handler was called => Query was sent
> and flag should have been set. However if response handler kicks in
> before line 2, we want to wait and make sure the flag is set and
> then free the query.
>
> Ideally after ib_nl_send_msg, we should not be accessing the query
> because we can be getting a response/timeout asynchronously and cant be
> sure when.
>
> The only places we access a query after it is successfully sent [response handler getting called
> => sending was successful] -
> 1. ib_nl_request_timeout
> 2. While setting the flag.
>
> 1. is taken care of because the request list access is protected by a lock
> and whoever gets the lock first deletes it from the request list and
> hence we can only have the response handler OR the timeout handler operate on the
> query.
>
> 2. To handle this is why we wait in the response handler. Once the flag is
> set we are no longer accessing the query and hence safe to release it.
>
> I have modified the comment for v2 as follows -
>
> /*
> + * 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.
> + */
>
>
> Thanks,
> Divya
>
>> Jason
On Tue, May 19, 2020 at 04:30:52PM -0700, Divya Indi wrote:
> Hi Jason,
>
> I wanted to follow up to see if you got a chance to review the following reply?
Not yet, it still seems bad to be doing code like this.
If two threads are sharing memory they really need to use a
refcount/kref not rely on some sketchy thing with flags. It is very
hard to tell if the sketchy thing with flags is correct or not.
Jason