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: <aFMVCY2_TqjAp_Aj@x130>
Date: Wed, 18 Jun 2025 12:35:37 -0700
From: Saeed Mahameed <saeed@...nel.org>
To: Simon Horman <horms@...nel.org>
Cc: Saeed Mahameed <saeedm@...dia.com>, Leon Romanovsky <leon@...nel.org>,
	Tariq Toukan <tariqt@...dia.com>,
	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 Jun 19:27, Simon Horman wrote:
>Elsewhere in dest_is_valid it is assumed that dest may be NULL.
It can't be null, so we need to remove other checks elsewhere.
see below:

>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.
>---
> 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 can't be null is it is always taken from entries of an array of
mlx5_flow_destination.

2 solutions:
   1. remove other `if (dest && ...` checks in this function, 
      to not confuse smatch.
   2. pass dest as value. there's only one caller of this function. 
      it is a 40B struct, don't know what the perf impact on flow rule
      insertion rate.

I prefer solution 1.

>+		    dest->type == MLX5_FLOW_DESTINATION_TYPE_FLOW_TABLE &&
> 		    ft->type != dest->ft->type)
> 			return false;
> 	}
>
>