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

[Divya Indi]

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