* [PATCH v4] net: mlx5: Add a missing check on idr_find, free buf
@ 2019-03-19 21:42 Aditya Pakki
2019-03-19 22:37 ` Saeed Mahameed
2019-03-20 4:54 ` Eric Dumazet
0 siblings, 2 replies; 5+ messages in thread
From: Aditya Pakki @ 2019-03-19 21:42 UTC (permalink / raw)
To: pakki001
Cc: kjlu, Boris Pismenny, Saeed Mahameed, Leon Romanovsky,
David S. Miller, Ilya Lesokhin, Wei Yongjun, netdev, linux-rdma,
linux-kernel
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();
+
+ 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;
}
--
2.17.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v4] net: mlx5: Add a missing check on idr_find, free buf
2019-03-19 21:42 [PATCH v4] net: mlx5: Add a missing check on idr_find, free buf Aditya Pakki
@ 2019-03-19 22:37 ` Saeed Mahameed
2019-03-20 4:54 ` Eric Dumazet
1 sibling, 0 replies; 5+ messages in thread
From: Saeed Mahameed @ 2019-03-19 22:37 UTC (permalink / raw)
To: pakki001
Cc: linux-kernel, linux-rdma, Boris Pismenny, weiyongjun1, kjlu,
netdev, Ilya Lesokhin, davem, leon
On Tue, 2019-03-19 at 16:42 -0500, 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(-)
>
Applied to net-mlx5, will be sent upstream shortly,
You messed up the Signed-off tag, it also should have been part of the
commit message, but don't worry, i fixed it up and added Yuval's
Reviewed-by.
Thanks Adeitya for handling all of the comments.
Saeed.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v4] net: mlx5: Add a missing check on idr_find, free buf
2019-03-19 21:42 [PATCH v4] net: mlx5: Add a missing check on idr_find, free buf Aditya Pakki
2019-03-19 22:37 ` Saeed Mahameed
@ 2019-03-20 4:54 ` Eric Dumazet
2019-03-20 5:02 ` Saeed Mahameed
1 sibling, 1 reply; 5+ messages in thread
From: Eric Dumazet @ 2019-03-20 4:54 UTC (permalink / raw)
To: Aditya Pakki
Cc: kjlu, Boris Pismenny, Saeed Mahameed, Leon Romanovsky,
David S. Miller, Ilya Lesokhin, Wei Yongjun, netdev, linux-rdma,
linux-kernel
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.
> +
> + 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;
> }
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v4] net: mlx5: Add a missing check on idr_find, free buf
2019-03-20 4:54 ` Eric Dumazet
@ 2019-03-20 5:02 ` Saeed Mahameed
2019-03-20 9:49 ` Boris Pismenny
0 siblings, 1 reply; 5+ messages in thread
From: Saeed Mahameed @ 2019-03-20 5:02 UTC (permalink / raw)
To: eric.dumazet, pakki001
Cc: linux-kernel, linux-rdma, Boris Pismenny, weiyongjun1, kjlu,
netdev, Ilya Lesokhin, davem, leon
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 !!
>
> > +
> > + 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;
> > }
> >
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v4] net: mlx5: Add a missing check on idr_find, free buf
2019-03-20 5:02 ` Saeed Mahameed
@ 2019-03-20 9:49 ` Boris Pismenny
0 siblings, 0 replies; 5+ messages in thread
From: Boris Pismenny @ 2019-03-20 9:49 UTC (permalink / raw)
To: Saeed Mahameed, eric.dumazet, pakki001
Cc: linux-kernel, linux-rdma, weiyongjun1, kjlu, netdev,
Ilya Lesokhin, davem, leon
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;
>>> }
>>>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-03-20 9:49 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-19 21:42 [PATCH v4] net: mlx5: Add a missing check on idr_find, free buf Aditya Pakki
2019-03-19 22:37 ` Saeed Mahameed
2019-03-20 4:54 ` Eric Dumazet
2019-03-20 5:02 ` Saeed Mahameed
2019-03-20 9:49 ` Boris Pismenny
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).