* [PATCH] net: mlx5: Add a missing check on idr_find
@ 2019-03-18 22:18 Aditya Pakki
2019-03-19 6:14 ` Leon Romanovsky
2019-03-19 13:41 ` Boris Pismenny
0 siblings, 2 replies; 5+ messages in thread
From: Aditya Pakki @ 2019-03-18 22:18 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.
Signed-off-by: Aditya Pakki <pakki001@umn.edu>
---
drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c b/drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c
index 5cf5f2a9d51f..3df468acdffc 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c
@@ -226,6 +226,8 @@ int mlx5_fpga_tls_resync_rx(struct mlx5_core_dev *mdev, u32 handle, u32 seq,
rcu_read_lock();
flow = idr_find(&mdev->fpga->tls->rx_idr, ntohl(handle));
rcu_read_unlock();
+ if (!flow)
+ return -EINVAL;
mlx5_fpga_tls_flow_to_cmd(flow, cmd);
MLX5_SET(tls_cmd, cmd, swid, ntohl(handle));
--
2.17.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] net: mlx5: Add a missing check on idr_find
2019-03-18 22:18 [PATCH] net: mlx5: Add a missing check on idr_find Aditya Pakki
@ 2019-03-19 6:14 ` Leon Romanovsky
2019-03-19 13:41 ` Boris Pismenny
1 sibling, 0 replies; 5+ messages in thread
From: Leon Romanovsky @ 2019-03-19 6:14 UTC (permalink / raw)
To: Aditya Pakki, Boris Pismeny, Saeed Mahameed
Cc: kjlu, David S. Miller, Wei Yongjun, netdev, linux-rdma, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1210 bytes --]
On Mon, Mar 18, 2019 at 05:18:51PM -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.
>
> Signed-off-by: Aditya Pakki <pakki001@umn.edu>
> ---
> drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c b/drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c
> index 5cf5f2a9d51f..3df468acdffc 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/fpga/tls.c
> @@ -226,6 +226,8 @@ int mlx5_fpga_tls_resync_rx(struct mlx5_core_dev *mdev, u32 handle, u32 seq,
> rcu_read_lock();
> flow = idr_find(&mdev->fpga->tls->rx_idr, ntohl(handle));
> rcu_read_unlock();
> + if (!flow)
> + return -EINVAL;
It is wrong and whole function is wrong too.
In such case, you will leak "buf" allocated above.
The function mlx5_fpga_sbu_conn_sendmsg() which is used below can fail
and it will leave "buf" unfreed too.
Thanks
> mlx5_fpga_tls_flow_to_cmd(flow, cmd);
>
> MLX5_SET(tls_cmd, cmd, swid, ntohl(handle));
> --
> 2.17.1
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] net: mlx5: Add a missing check on idr_find
2019-03-18 22:18 [PATCH] net: mlx5: Add a missing check on idr_find Aditya Pakki
2019-03-19 6:14 ` Leon Romanovsky
@ 2019-03-19 13:41 ` Boris Pismenny
2019-03-19 14:35 ` Leon Romanovsky
1 sibling, 1 reply; 5+ messages in thread
From: Boris Pismenny @ 2019-03-19 13:41 UTC (permalink / raw)
To: Aditya Pakki
Cc: kjlu, Saeed Mahameed, Leon Romanovsky, David S. Miller,
Ilya Lesokhin, Wei Yongjun, netdev, linux-rdma, linux-kernel
On 3/19/2019 12:18 AM, 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.
Did you encounter this in practice?
This flow you are suggesting shouldn't be possible, because the handle
is always there until the socket is destroyed in sk_destruct.
But, I wouldn't mind some defensive coding here.
Maybe also a WARN_ONCE :)
Could you also release buf in case of an error returned from
mlx5_fpga_sbu_conn_sendmsg below?
Otherwise, I could submit a patch for this.
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] net: mlx5: Add a missing check on idr_find
2019-03-19 13:41 ` Boris Pismenny
@ 2019-03-19 14:35 ` Leon Romanovsky
2019-03-19 15:00 ` Boris Pismenny
0 siblings, 1 reply; 5+ messages in thread
From: Leon Romanovsky @ 2019-03-19 14:35 UTC (permalink / raw)
To: Boris Pismenny
Cc: Aditya Pakki, kjlu, Saeed Mahameed, David S. Miller,
Ilya Lesokhin, Wei Yongjun, netdev, linux-rdma, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 800 bytes --]
On Tue, Mar 19, 2019 at 01:41:49PM +0000, Boris Pismenny wrote:
>
>
> On 3/19/2019 12:18 AM, 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.
>
> Did you encounter this in practice?
> This flow you are suggesting shouldn't be possible, because the handle
> is always there until the socket is destroyed in sk_destruct.
>
> But, I wouldn't mind some defensive coding here.
> Maybe also a WARN_ONCE :)
>
> Could you also release buf in case of an error returned from
> mlx5_fpga_sbu_conn_sendmsg below?
> Otherwise, I could submit a patch for this.
Boris,
Can you please invest ten seconds to read previous emails prior to answering?
https://lkml.org/lkml/2019/3/19/36
Thanks
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] net: mlx5: Add a missing check on idr_find
2019-03-19 14:35 ` Leon Romanovsky
@ 2019-03-19 15:00 ` Boris Pismenny
0 siblings, 0 replies; 5+ messages in thread
From: Boris Pismenny @ 2019-03-19 15:00 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Aditya Pakki, kjlu, Saeed Mahameed, David S. Miller,
Ilya Lesokhin, Wei Yongjun, netdev, linux-rdma, linux-kernel
Hi Leon,
On 3/19/2019 4:35 PM, Leon Romanovsky wrote:
> On Tue, Mar 19, 2019 at 01:41:49PM +0000, Boris Pismenny wrote:
>>
>>
>> On 3/19/2019 12:18 AM, 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.
>>
>> Did you encounter this in practice?
>> This flow you are suggesting shouldn't be possible, because the handle
>> is always there until the socket is destroyed in sk_destruct.
>>
>> But, I wouldn't mind some defensive coding here.
>> Maybe also a WARN_ONCE :)
>>
>> Could you also release buf in case of an error returned from
>> mlx5_fpga_sbu_conn_sendmsg below?
>> Otherwise, I could submit a patch for this.
>
> Boris,
>
> Can you please invest ten seconds to read previous emails prior to answering?
> https://lkml.org/lkml/2019/3/19/36
>
The fix you suggested is valid and should be addressed as well.
I didn't comment on your reply, but it doesn't mean that I disagree.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-03-19 15:00 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-18 22:18 [PATCH] net: mlx5: Add a missing check on idr_find Aditya Pakki
2019-03-19 6:14 ` Leon Romanovsky
2019-03-19 13:41 ` Boris Pismenny
2019-03-19 14:35 ` Leon Romanovsky
2019-03-19 15:00 ` 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).