netdev - Re: [PATCH v4] net: mlx5: Add a missing check on idr

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Wed, 20 Mar 2019 09:49:25 +0000
From:   Boris Pismenny <borisp@...lanox.com>
To:     Saeed Mahameed <saeedm@...lanox.com>,
        "eric.dumazet@...il.com" <eric.dumazet@...il.com>,
        "pakki001@....edu" <pakki001@....edu>
CC:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
        "weiyongjun1@...wei.com" <weiyongjun1@...wei.com>,
        "kjlu@....edu" <kjlu@....edu>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Ilya Lesokhin <ilyal@...lanox.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "leon@...nel.org" <leon@...nel.org>
Subject: Re: [PATCH v4] net: mlx5: Add a missing check on idr_find, free buf



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@....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;
>>>   }
>>>