[PATCH RFC net] net/mlx5: Avoid NULL dereference in dest_is_valid

[PATCH RFC net] net/mlx5: Avoid NULL dereference in dest_is_valid

[Simon Horman]

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@kernel.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->type == MLX5_FLOW_DESTINATION_TYPE_FLOW_TABLE &&
 		    ft->type != dest->ft->type)
 			return false;
 	}

Re: [PATCH RFC net] net/mlx5: Avoid NULL dereference in dest_is_valid

[Mark Bloch]

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@kernel.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;
>  	}
> 

Re: [PATCH RFC net] net/mlx5: Avoid NULL dereference in dest_is_valid

[Saeed Mahameed]

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@kernel.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;
> 	}
>
>

Re: [PATCH RFC net] net/mlx5: Avoid NULL dereference in dest_is_valid

[Simon Horman]

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