[PATCH RESEND net] net/mlx5e: Cleanup MACsec uninitialization routine

[PATCH RESEND net] net/mlx5e: Cleanup MACsec uninitialization routine

[Leon Romanovsky]

From: Leon Romanovsky <leonro@nvidia.com>

The mlx5e_macsec_cleanup() routine has pointer dereferencing if mlx5 device
doesn't support MACsec (priv->macsec will be NULL) together with useless
comment line, assignment and extra blank lines.

Fix everything in one patch.

Fixes: 1f53da676439 ("net/mlx5e: Create advanced steering operation (ASO) object for MACsec")
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
---
Resend: https://lore.kernel.org/all/b43b1c5aadd5cfdcd2e385ce32693220331700ba.1665645548.git.leonro@nvidia.com
---
 .../net/ethernet/mellanox/mlx5/core/en_accel/macsec.c | 11 +----------
 1 file changed, 1 insertion(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
index 41970067917b..4331235b21ee 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
@@ -1846,25 +1846,16 @@ int mlx5e_macsec_init(struct mlx5e_priv *priv)
 void mlx5e_macsec_cleanup(struct mlx5e_priv *priv)
 {
 	struct mlx5e_macsec *macsec = priv->macsec;
-	struct mlx5_core_dev *mdev = macsec->mdev;
+	struct mlx5_core_dev *mdev = priv->mdev;
 
 	if (!macsec)
 		return;
 
 	mlx5_notifier_unregister(mdev, &macsec->nb);
-
 	mlx5e_macsec_fs_cleanup(macsec->macsec_fs);
-
-	/* Cleanup workqueue */
 	destroy_workqueue(macsec->wq);
-
 	mlx5e_macsec_aso_cleanup(&macsec->aso, mdev);
-
-	priv->macsec = NULL;
-
 	rhashtable_destroy(&macsec->sci_hash);
-
 	mutex_destroy(&macsec->lock);
-
 	kfree(macsec);
 }
-- 
2.37.3

Re: [PATCH RESEND net] net/mlx5e: Cleanup MACsec uninitialization routine

[Saeed Mahameed]

On 19 Oct 09:06, Leon Romanovsky wrote:
>From: Leon Romanovsky <leonro@nvidia.com>
>
>The mlx5e_macsec_cleanup() routine has pointer dereferencing if mlx5 device
>doesn't support MACsec (priv->macsec will be NULL) together with useless
>comment line, assignment and extra blank lines.
>
>Fix everything in one patch.
>
>Fixes: 1f53da676439 ("net/mlx5e: Create advanced steering operation (ASO) object for MACsec")
>Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
>---
>Resend: https://lore.kernel.org/all/b43b1c5aadd5cfdcd2e385ce32693220331700ba.1665645548.git.leonro@nvidia.com
>---
> .../net/ethernet/mellanox/mlx5/core/en_accel/macsec.c | 11 +----------
> 1 file changed, 1 insertion(+), 10 deletions(-)
>
>diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
>index 41970067917b..4331235b21ee 100644
>--- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
>+++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
>@@ -1846,25 +1846,16 @@ int mlx5e_macsec_init(struct mlx5e_priv *priv)
> void mlx5e_macsec_cleanup(struct mlx5e_priv *priv)
> {
> 	struct mlx5e_macsec *macsec = priv->macsec;
>-	struct mlx5_core_dev *mdev = macsec->mdev;
>+	struct mlx5_core_dev *mdev = priv->mdev;
>
> 	if (!macsec)
> 		return;
>
> 	mlx5_notifier_unregister(mdev, &macsec->nb);
>-
> 	mlx5e_macsec_fs_cleanup(macsec->macsec_fs);
>-
>-	/* Cleanup workqueue */
> 	destroy_workqueue(macsec->wq);
>-
> 	mlx5e_macsec_aso_cleanup(&macsec->aso, mdev);
>-
>-	priv->macsec = NULL;
>-

Tariq was right, we need this check, the same priv can be resurrected
after cleanup to be used in switchdev representor profile, where
capabilities are not guaranteed to be the same as NIC netdev, so you will
end up using a garbage macsec.

Also we don't submit cleanups to net to avoid porting unnecessary changes
and new bugs to -rc.

Re: [PATCH RESEND net] net/mlx5e: Cleanup MACsec uninitialization routine

[Leon Romanovsky]

On Tue, Oct 18, 2022 at 11:27:10PM -0700, Saeed Mahameed wrote:
> On 19 Oct 09:06, Leon Romanovsky wrote:
> > From: Leon Romanovsky <leonro@nvidia.com>
> > 
> > The mlx5e_macsec_cleanup() routine has pointer dereferencing if mlx5 device
> > doesn't support MACsec (priv->macsec will be NULL) together with useless
> > comment line, assignment and extra blank lines.
> > 
> > Fix everything in one patch.
> > 
> > Fixes: 1f53da676439 ("net/mlx5e: Create advanced steering operation (ASO) object for MACsec")
> > Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> > Signed-off-by: Leon Romanovsky <leonro@nvidia.com>
> > ---
> > Resend: https://lore.kernel.org/all/b43b1c5aadd5cfdcd2e385ce32693220331700ba.1665645548.git.leonro@nvidia.com
> > ---
> > .../net/ethernet/mellanox/mlx5/core/en_accel/macsec.c | 11 +----------
> > 1 file changed, 1 insertion(+), 10 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
> > index 41970067917b..4331235b21ee 100644
> > --- a/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
> > +++ b/drivers/net/ethernet/mellanox/mlx5/core/en_accel/macsec.c
> > @@ -1846,25 +1846,16 @@ int mlx5e_macsec_init(struct mlx5e_priv *priv)
> > void mlx5e_macsec_cleanup(struct mlx5e_priv *priv)
> > {
> > 	struct mlx5e_macsec *macsec = priv->macsec;
> > -	struct mlx5_core_dev *mdev = macsec->mdev;
> > +	struct mlx5_core_dev *mdev = priv->mdev;
> > 
> > 	if (!macsec)
> > 		return;
> > 
> > 	mlx5_notifier_unregister(mdev, &macsec->nb);
> > -
> > 	mlx5e_macsec_fs_cleanup(macsec->macsec_fs);
> > -
> > -	/* Cleanup workqueue */
> > 	destroy_workqueue(macsec->wq);
> > -
> > 	mlx5e_macsec_aso_cleanup(&macsec->aso, mdev);
> > -
> > -	priv->macsec = NULL;
> > -
> 
> Tariq was right, we need this check, the same priv can be resurrected
> after cleanup to be used in switchdev representor profile, where
> capabilities are not guaranteed to be the same as NIC netdev, so you will
> end up using a garbage macsec.

Not really, we are checking that device supports MACsec with
mlx5e_is_macsec_device() in every entry call where is a chance do
not have priv->macsec. If device doesn't support, the relevant ops
won't be installed (mlx5e_macsec_build_netdev) and nothing will call
uninitialized MACsec.

The NULL assignment is purely anti-pattern where you hide use-after-free
access.

> 
> Also we don't submit cleanups to net to avoid porting unnecessary changes
> and new bugs to -rc.

It is something that was added in previous cycle. There is no
backporting yet.

Thanks

Re: [PATCH RESEND net] net/mlx5e: Cleanup MACsec uninitialization routine

[Jakub Kicinski]

On Wed, 19 Oct 2022 09:06:43 +0300 Leon Romanovsky wrote:
> The mlx5e_macsec_cleanup() routine has pointer dereferencing if mlx5 device
                                        ^
                                NULL? _/

> doesn't support MACsec (priv->macsec will be NULL) together with useless

s/together with/. While at it delete/

> comment line, assignment and extra blank lines.