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

Message-ID: <20250618194957.GB1699@horms.kernel.org>
Date: Wed, 18 Jun 2025 20:49:57 +0100
From: Simon Horman <horms@...nel.org>
To: Mark Bloch <mbloch@...dia.com>, Saeed Mahameed <saeed@...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 Wed, Jun 18, 2025 at 10:35:25PM +0300, Mark Bloch wrote:
> 
> 
> 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!

...

On Wed, Jun 18, 2025 at 12:35:37PM -0700, Saeed Mahameed wrote:
> On 18 Jun 19:27, Simon Horman wrote:

...

> > 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.


Thanks Saeed and Mark for your quick response,
and sorry for the false positive.

I agree that removing the other dest checks makes sense.
And of course there is no urgency for that on my side.