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

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]

Date:   Tue, 18 Oct 2022 23:27:10 -0700
From:   Saeed Mahameed <saeedm@...dia.com>
To:     Leon Romanovsky <leon@...nel.org>
Cc:     "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Leon Romanovsky <leonro@...dia.com>,
        Dan Carpenter <dan.carpenter@...cle.com>,
        Emeel Hakim <ehakim@...dia.com>,
        Eric Dumazet <edumazet@...gle.com>, netdev@...r.kernel.org,
        Paolo Abeni <pabeni@...hat.com>, Raed Salem <raeds@...dia.com>,
        Tariq Toukan <tariqt@...dia.com>
Subject: Re: [PATCH RESEND net] net/mlx5e: Cleanup MACsec uninitialization
 routine

On 19 Oct 09:06, Leon Romanovsky wrote:
>From: Leon Romanovsky <leonro@...dia.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@...cle.com>
>Signed-off-by: Leon Romanovsky <leonro@...dia.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.