[PATCH] net: mlx5: Add a missing check on idr_find

[PATCH] net: mlx5: Add a missing check on idr_find

[Aditya Pakki]

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

Re: [PATCH] net: mlx5: Add a missing check on idr_find

[Leon Romanovsky]

[-- 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 --]

Re: [PATCH] net: mlx5: Add a missing check on idr_find

[Boris Pismenny]

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.

Re: [PATCH] net: mlx5: Add a missing check on idr_find

[Leon Romanovsky]

[-- 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 --]

Re: [PATCH] net: mlx5: Add a missing check on idr_find

[Boris Pismenny]

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.