Re: [PATCH v4] net: mlx5: Add a missing check on idr_find, free buf

[Boris Pismenny <borisp@mellanox.com>]

On 3/20/2019 7:02 AM, Saeed Mahameed wrote:
> On Tue, 2019-03-19 at 21:54 -0700, Eric Dumazet wrote:
>>
>> On 03/19/2019 02:42 PM, Aditya Pakki wrote:
>>> idr_find() can return a NULL value to 'flow' which is used without
>>> a
>>> check. The patch adds a check to avoid potential NULL pointer
>>> dereference.
>>>
>>> In case of mlx5_fpga_sbu_conn_sendmsg() failure, free buf allocated
>>> using kzalloc.
>>>
>>> Fixes: ab412e1dd7db ("net/mlx5: Accel, add TLS rx offload
>>> routines")
>>> ---
>>> v3: Reorder buf allocations and flow check.
>>> v2: failure to return in case of flow failure.
>>> v1: Failed to free buf in case of flow failure.
>>>
>>> Signed-off-by: Aditya Pakki <pakki001@umn.edu>
>>> ---
>>>   drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c | 14
>>> +++++++++++---
>>>   1 file changed, 11 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c
>>> b/drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c
>>> index 5cf5f2a9d51f..8de64e88c670 100644
>>> --- a/drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c
>>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c
>>> @@ -217,15 +217,21 @@ int mlx5_fpga_tls_resync_rx(struct
>>> mlx5_core_dev *mdev, u32 handle, u32 seq,
>>>   	void *cmd;
>>>   	int ret;
>>>   
>>> +	rcu_read_lock();
>>> +	flow = idr_find(&mdev->fpga->tls->rx_idr, ntohl(handle));
>>> +	rcu_read_unlock();
>>
>> This looks suspect (even before your patch)
>>
>> What prevents flow from disappearing after this rcu_read_lock() ?
>>
>> IMO your patch might prevent a NULL deref, but not use-after-free.
> 
> That crossed my mind when i reviewed this patch but since this is an
> old issue i just put a todo aside to handle it later
> 
> i think the author didn't want to deal with use-after-free here, but
> only wanted to keep the data structure safe against add/remove
> operations.
> 
> Anyway all we need here is
>   
> rcu_read_lock()
> flow = idr_find(..)
> mlx5_fpga_tls_flow_to_cmd(flow, cmd);
> rcu_read_unlock()
> 
> Will fix it up in a follow up patch,
> 
> Thank you Eric !!
> 

This flow won't be removed unless the tcp connection call sk_destruct or 
the netdev is removed.

Thus, this lookup should *always* succeed.

This, is also why I wanted to ask Aditya if this flow was actually 
encountered in practice. I'm sure that if our assumption about the 
safety of this flow breaks, then other parts of the TLS driver will fail 
as well.

> 
>>
>>> +
>>> +	if (!flow) {
>>> +		WARN_ONCE(1, "Received NULL pointer for handle\n");
>>> +		return -EINVAL;
>>> +	}
>>> +
>>>   	buf = kzalloc(size, GFP_ATOMIC);
>>>   	if (!buf)
>>>   		return -ENOMEM;
>>>   
>>>   	cmd = (buf + 1);
>>>   
>>> -	rcu_read_lock();
>>> -	flow = idr_find(&mdev->fpga->tls->rx_idr, ntohl(handle));
>>> -	rcu_read_unlock();
>>>   	mlx5_fpga_tls_flow_to_cmd(flow, cmd);
>>>   
>>>   	MLX5_SET(tls_cmd, cmd, swid, ntohl(handle));
>>> @@ -238,6 +244,8 @@ int mlx5_fpga_tls_resync_rx(struct
>>> mlx5_core_dev *mdev, u32 handle, u32 seq,
>>>   	buf->complete = mlx_tls_kfree_complete;
>>>   
>>>   	ret = mlx5_fpga_sbu_conn_sendmsg(mdev->fpga->tls->conn, buf);
>>> +	if (ret < 0)
>>> +		kfree(buf);
>>>   
>>>   	return ret;
>>>   }
>>>