diff mbox series

[V2] net/mlx5e: fix potential null dereference in mlx5e_tc_nic_create_miss_table

Message ID 20250407072032.5232-1-hanchunchao@inspur.com (mailing list archive)
State Not Applicable
Headers show
Series [V2] net/mlx5e: fix potential null dereference in mlx5e_tc_nic_create_miss_table | expand

Commit Message

Charles Han April 7, 2025, 7:20 a.m. UTC 
mlx5_get_flow_namespace() may return a NULL pointer, dereferencing it
without NULL check may lead to NULL dereference.
Add a NULL check for ns.

Fixes: 66cb64e292d2 ("net/mlx5e: TC NIC mode, fix tc chains miss table")
Signed-off-by: Charles Han <hanchunchao@inspur.com>
---
 drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 4 ++++
 1 file changed, 4 insertions(+)

Comments

Tariq Toukan April 7, 2025, 9:29 a.m. UTC | #1 
On 07/04/2025 10:20, Charles Han wrote:
> mlx5_get_flow_namespace() may return a NULL pointer, dereferencing it
> without NULL check may lead to NULL dereference.
> Add a NULL check for ns.
> 
> Fixes: 66cb64e292d2 ("net/mlx5e: TC NIC mode, fix tc chains miss table")
> Signed-off-by: Charles Han <hanchunchao@inspur.com>
> ---
>   drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> index 9ba99609999f..c2f23ac95c3d 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> @@ -5216,6 +5216,10 @@ static int mlx5e_tc_nic_create_miss_table(struct mlx5e_priv *priv)
>   	ft_attr.level = MLX5E_TC_MISS_LEVEL;
>   	ft_attr.prio = 0;
>   	ns = mlx5_get_flow_namespace(priv->mdev, MLX5_FLOW_NAMESPACE_KERNEL);
> +	if (!ns) {
> +		netdev_err(priv->mdev, "Failed to get flow namespace\n");
> +		return -EOPNOTSUPP;
> +	}
>   
>   	*ft = mlx5_create_auto_grouped_flow_table(ns, &ft_attr);
>   	if (IS_ERR(*ft)) {

Same question here, did it fail for you, or just saw it while reading 
the code?
Simon Horman April 7, 2025, 4:18 p.m. UTC | #2 
On Mon, Apr 07, 2025 at 03:20:31PM +0800, Charles Han wrote:
> mlx5_get_flow_namespace() may return a NULL pointer, dereferencing it
> without NULL check may lead to NULL dereference.
> Add a NULL check for ns.
> 
> Fixes: 66cb64e292d2 ("net/mlx5e: TC NIC mode, fix tc chains miss table")
> Signed-off-by: Charles Han <hanchunchao@inspur.com>
> ---
>  drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> index 9ba99609999f..c2f23ac95c3d 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> @@ -5216,6 +5216,10 @@ static int mlx5e_tc_nic_create_miss_table(struct mlx5e_priv *priv)
>  	ft_attr.level = MLX5E_TC_MISS_LEVEL;
>  	ft_attr.prio = 0;
>  	ns = mlx5_get_flow_namespace(priv->mdev, MLX5_FLOW_NAMESPACE_KERNEL);
> +	if (!ns) {
> +		netdev_err(priv->mdev, "Failed to get flow namespace\n");

Hi Charles,

This does not seem to be correct. gcc-14.2.0 says:

drivers/net/ethernet/mellanox/mlx5/core/en_tc.c: In function 'mlx5e_tc_nic_create_miss_table':
drivers/net/ethernet/mellanox/mlx5/core/en_tc.c:5220:32: error: passing argument 1 of 'netdev_err' from incompatible pointer type [-Wincompatible-pointer-types]
 5220 |                 netdev_err(priv->mdev, "Failed to get flow namespace\n");
      |                            ~~~~^~~~~~
      |                                |
      |                                struct mlx5_core_dev *
In file included from ./include/linux/skbuff.h:39,
                 from ./include/linux/netlink.h:7,
                 from ./include/net/flow_offload.h:6,
                 from drivers/net/ethernet/mellanox/mlx5/core/en_tc.c:34:
./include/net/net_debug.h:20:42: note: expected 'const struct net_device *' but argument is of type 'struct mlx5_core_dev *'
   20 | void netdev_err(const struct net_device *dev, const char *format, ...);
      |                 ~~~~~~~~~~~~~~~~~~~~~~~~~^~~

...
Charles Han April 8, 2025, 7:06 a.m. UTC | #3 
On Mon, Apr 07, 2025 at 12:29:22PM +0300, Tariq Toukan wrote:
> 
> 
> On 07/04/2025 10:20, Charles Han wrote:
> > mlx5_get_flow_namespace() may return a NULL pointer, dereferencing it
> > without NULL check may lead to NULL dereference.
> > Add a NULL check for ns.
> > 
> > Fixes: 66cb64e292d2 ("net/mlx5e: TC NIC mode, fix tc chains miss table")
> > Signed-off-by: Charles Han <hanchunchao@inspur.com>
> > ---
> >   drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 4 ++++
> >   1 file changed, 4 insertions(+)
> > 
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> > index 9ba99609999f..c2f23ac95c3d 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> > @@ -5216,6 +5216,10 @@ static int mlx5e_tc_nic_create_miss_table(struct mlx5e_priv *priv)
> >   	ft_attr.level = MLX5E_TC_MISS_LEVEL;
> >   	ft_attr.prio = 0;
> >   	ns = mlx5_get_flow_namespace(priv->mdev, MLX5_FLOW_NAMESPACE_KERNEL);
> > +	if (!ns) {
> > +		netdev_err(priv->mdev, "Failed to get flow namespace\n");
> > +		return -EOPNOTSUPP;
> > +	}
> >   	*ft = mlx5_create_auto_grouped_flow_table(ns, &ft_attr);
> >   	if (IS_ERR(*ft)) {
> 
> Same question here, did it fail for you, or just saw it while reading the
> code?
I just saw it while reading the code.
I've been working on code vulnerability scanning recently.
Mark Bloch April 8, 2025, 3:16 p.m. UTC | #4 
On 08/04/2025 10:06, Charles Han wrote:
> On Mon, Apr 07, 2025 at 12:29:22PM +0300, Tariq Toukan wrote:
>>
>>
>> On 07/04/2025 10:20, Charles Han wrote:
>>> mlx5_get_flow_namespace() may return a NULL pointer, dereferencing it
>>> without NULL check may lead to NULL dereference.
>>> Add a NULL check for ns.
>>>
>>> Fixes: 66cb64e292d2 ("net/mlx5e: TC NIC mode, fix tc chains miss table")
>>> Signed-off-by: Charles Han <hanchunchao@inspur.com>
>>> ---
>>>   drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 4 ++++
>>>   1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
>>> index 9ba99609999f..c2f23ac95c3d 100644
>>> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
>>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
>>> @@ -5216,6 +5216,10 @@ static int mlx5e_tc_nic_create_miss_table(struct mlx5e_priv *priv)
>>>   	ft_attr.level = MLX5E_TC_MISS_LEVEL;
>>>   	ft_attr.prio = 0;
>>>   	ns = mlx5_get_flow_namespace(priv->mdev, MLX5_FLOW_NAMESPACE_KERNEL);
>>> +	if (!ns) {
>>> +		netdev_err(priv->mdev, "Failed to get flow namespace\n");
>>> +		return -EOPNOTSUPP;
>>> +	}
>>>   	*ft = mlx5_create_auto_grouped_flow_table(ns, &ft_attr);
>>>   	if (IS_ERR(*ft)) {
>>
>> Same question here, did it fail for you, or just saw it while reading the
>> code?
> I just saw it while reading the code.
> I've been working on code vulnerability scanning recently.
> 

I don't believe this scenario can actually occur.
The function mlx5e_tc_nic_init() is called from mlx5e_init_nic_rx(),
and before that, we invoke mlx5e_create_flow_steering().

In mlx5e_create_flow_steering(), the first operation is:

<snip>
int mlx5e_create_flow_steering(struct mlx5e_flow_steering *fs,
                               struct mlx5e_rx_res *rx_res,
                               const struct mlx5e_profile *profile,
                               struct net_device *netdev)
{
        struct mlx5_flow_namespace *ns = mlx5_get_flow_namespace(fs->mdev,
                                                                 MLX5_FLOW_NAMESPACE_KERNEL);
        int err;

        if (!ns)
                return -EOPNOTSUPP;
</snip>

Note that MLX5_FLOW_NAMESPACE_KERNEL is allocated and initialized at
driver startup (as most/all namespaces), and it does not
change dynamically.

If mlx5e_create_flow_steering() fails, it indicates that
something fundamental isn't functioning correctly, and we
never proceed to the more advanced functionality (like tc).

Mark
Tariq Toukan April 9, 2025, 2:16 p.m. UTC | #5 
On 07/04/2025 10:20, Charles Han wrote:
> mlx5_get_flow_namespace() may return a NULL pointer, dereferencing it
> without NULL check may lead to NULL dereference.
> Add a NULL check for ns.
> 
> Fixes: 66cb64e292d2 ("net/mlx5e: TC NIC mode, fix tc chains miss table")
> Signed-off-by: Charles Han <hanchunchao@inspur.com>
> ---
>   drivers/net/ethernet/mellanox/mlx5/core/en_tc.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> index 9ba99609999f..c2f23ac95c3d 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
> @@ -5216,6 +5216,10 @@ static int mlx5e_tc_nic_create_miss_table(struct mlx5e_priv *priv)
>   	ft_attr.level = MLX5E_TC_MISS_LEVEL;
>   	ft_attr.prio = 0;
>   	ns = mlx5_get_flow_namespace(priv->mdev, MLX5_FLOW_NAMESPACE_KERNEL);
> +	if (!ns) {
> +		netdev_err(priv->mdev, "Failed to get flow namespace\n");
> +		return -EOPNOTSUPP;
> +	}
>   
>   	*ft = mlx5_create_auto_grouped_flow_table(ns, &ft_attr);
>   	if (IS_ERR(*ft)) {


Too many similar patches submitted individually in parallel by multiple 
authors..

One can easily lose track.

Please gather similar patches in a series, provide cover letter and 
target branch.

Tariq.
diff mbox series

Patch

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
index 9ba99609999f..c2f23ac95c3d 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_tc.c
@@ -5216,6 +5216,10 @@  static int mlx5e_tc_nic_create_miss_table(struct mlx5e_priv *priv)
 	ft_attr.level = MLX5E_TC_MISS_LEVEL;
 	ft_attr.prio = 0;
 	ns = mlx5_get_flow_namespace(priv->mdev, MLX5_FLOW_NAMESPACE_KERNEL);
+	if (!ns) {
+		netdev_err(priv->mdev, "Failed to get flow namespace\n");
+		return -EOPNOTSUPP;
+	}
 
 	*ft = mlx5_create_auto_grouped_flow_table(ns, &ft_attr);
 	if (IS_ERR(*ft)) {