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

[Divya Indi <divya.indi@oracle.com>]

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