netdev - Re: [PATCH RFC net] net/mlx5: Avoid NULL dereference in dest_is

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <4f115511-5739-416d-a687-6b25a21fdd56@nvidia.com>
Date: Wed, 18 Jun 2025 22:35:25 +0300
From: Mark Bloch <mbloch@...dia.com>
To: Simon Horman <horms@...nel.org>, Saeed Mahameed <saeedm@...dia.com>,
 Leon Romanovsky <leon@...nel.org>, Tariq Toukan <tariqt@...dia.com>
Cc: Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller"
 <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>,
 Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
 Mark Bloch <markb@...lanox.com>, Paul Blakey <paulb@...lanox.com>,
 netdev@...r.kernel.org, linux-rdma@...r.kernel.org
Subject: Re: [PATCH RFC net] net/mlx5: Avoid NULL dereference in dest_is_valid

On 18/06/2025 21:27, Simon Horman wrote:
> Elsewhere in dest_is_valid it is assumed that dest may be NULL.
> But the line updated by this patch dereferences dest unconditionally.
> This seems to be inconsistent.
> 
> Flagged by Smatch.
> Compile tested only.
> 
> Fixes: ff189b435682 ("net/mlx5: Add ignore level support fwd to table rules")
> Signed-off-by: Simon Horman <horms@...nel.org>
> ---
> I am posting this as an RFC as I am not completely sure this change is
> necessary. F.e. an invariant that I'm unaware of may preclude dest
> from being NULL in this case.

_mlx5_add_flow_rules() does:
        for (i = 0; i < dest_num; i++) {
                if (!dest_is_valid(&dest[i], flow_act, ft))
                        return ERR_PTR(-EINVAL);
        }

so indeed dest must be valid inside dest_is_valid().\
No defensive programming for this as all the callers are part of mlx5
driver and they shouldn't pass dest_num that doesn't match how many
dests are passed.

If anything I would drop the other checks for dest inside that function.
I'll add that to my TODO list.

Thanks!

> ---
>  drivers/net/ethernet/mellanox/mlx5/core/fs_core.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/fs_core.c b/drivers/net/ethernet/mellanox/mlx5/core/fs_core.c
> index a8046200d376..7eeab93a1aa9 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/fs_core.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/fs_core.c
> @@ -2041,7 +2041,8 @@ static bool dest_is_valid(struct mlx5_flow_destination *dest,
>  		    ft->type != FS_FT_NIC_TX)
>  			return false;
>  
> -		if (dest->type == MLX5_FLOW_DESTINATION_TYPE_FLOW_TABLE &&
> +		if (dest &&
> +		    dest->type == MLX5_FLOW_DESTINATION_TYPE_FLOW_TABLE &&
>  		    ft->type != dest->ft->type)
>  			return false;
>  	}
>