Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH 1/2] IB/sa: Resolving use-after-free in ib_nl_send_msg.
From:   Divya Indi <divya.indi@oracle.com>
To:     Jason Gunthorpe <jgg@ziepe.ca>
Cc:     linux-kernel@vger.kernel.org, linux-rdma@vger.kernel.org,
        Kaike Wan <kaike.wan@intel.com>,
        Gerd Rausch <gerd.rausch@oracle.com>,
        =?UTF-8?Q?H=c3=a5kon_Bugge?= <haakon.bugge@oracle.com>,
        Srinivas Eeda <srinivas.eeda@oracle.com>,
        Rama Nichanamatlu <rama.nichanamatlu@oracle.com>,
        Doug Ledford <dledford@redhat.com>
References: <1588876487-5781-1-git-send-email-divya.indi@oracle.com>
 <1588876487-5781-2-git-send-email-divya.indi@oracle.com>
 <20200508000809.GM26002@ziepe.ca>
 <33fc99e2-e9fc-3c8c-e47f-41535f514c2d@oracle.com>
 <20200513150021.GD29989@ziepe.ca>
 <c761da30-3663-4932-dd72-3501f15c0197@oracle.com>
Message-ID: <272dbe83-a72b-d38b-6993-d3bbda50a7d1@oracle.com>
Date:   Tue, 19 May 2020 16:30:52 -0700
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:68.0)
 Gecko/20100101 Thunderbird/68.8.0
MIME-Version: 1.0
In-Reply-To: <c761da30-3663-4932-dd72-3501f15c0197@oracle.com>
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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