diff mbox series

[net-next] net: phy: mscc: macsec: do not copy encryption keys

Message ID 20221114092033.34405-1-atenart@kernel.org (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [net-next] net: phy: mscc: macsec: do not copy encryption keys | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/fixes_present success Fixes tag not required for -next series
netdev/subject_prefix success Link
netdev/cover_letter success Single patches do not need cover letters
netdev/patch_count success Link
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/cc_maintainers warning 3 maintainers not CCed: linux@armlinux.org.uk andrew@lunn.ch hkallweit1@gmail.com
netdev/build_clang success Errors and warnings before: 0 this patch: 0
netdev/module_param success Was 0 now: 0
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success No Fixes tag
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 136 lines checked
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/source_inline success Was 0 now: 0

Commit Message

Antoine Tenart Nov. 14, 2022, 9:20 a.m. UTC 
Instead of calling memzero_explicit on the key when freeing a flow,
let's simply not copy the key in the first place as it's only used when
a new flow is set up.

Signed-off-by: Antoine Tenart <atenart@kernel.org>
---

Following
https://lore.kernel.org/all/20221108153459.811293-1-atenart@kernel.org/T/
refactor the MSCC PHY driver not to make a copy the encryption keys.

 drivers/net/phy/mscc/mscc_macsec.c | 57 ++++++++++++++++--------------
 drivers/net/phy/mscc/mscc_macsec.h |  2 --
 2 files changed, 30 insertions(+), 29 deletions(-)

Comments

Paolo Abeni Nov. 15, 2022, 2:38 p.m. UTC | #1 
Hello,

On Mon, 2022-11-14 at 10:20 +0100, Antoine Tenart wrote:
> Instead of calling memzero_explicit on the key when freeing a flow,
> let's simply not copy the key in the first place as it's only used when
> a new flow is set up.
> 
> Signed-off-by: Antoine Tenart <atenart@kernel.org>
> ---
> 
> Following
> https://lore.kernel.org/all/20221108153459.811293-1-atenart@kernel.org/T/
> refactor the MSCC PHY driver not to make a copy the encryption keys.

The patch LGTM, but would you mind including into the commit message a
reference to the -net commit, so that the dependency is there to
simplify eventual backports?

Thanks!

Paolo
Antoine Tenart Nov. 15, 2022, 2:58 p.m. UTC | #2 
Hi,

Quoting Paolo Abeni (2022-11-15 15:38:06)
> On Mon, 2022-11-14 at 10:20 +0100, Antoine Tenart wrote:
> > Instead of calling memzero_explicit on the key when freeing a flow,
> > let's simply not copy the key in the first place as it's only used when
> > a new flow is set up.
> > 
> > Signed-off-by: Antoine Tenart <atenart@kernel.org>
> > ---
> > 
> > Following
> > https://lore.kernel.org/all/20221108153459.811293-1-atenart@kernel.org/T/
> > refactor the MSCC PHY driver not to make a copy the encryption keys.
> 
> The patch LGTM, but would you mind including into the commit message a
> reference to the -net commit, so that the dependency is there to
> simplify eventual backports?

Sure, will do in a v2.

Thanks!
Antoine
diff mbox series

Patch

diff --git a/drivers/net/phy/mscc/mscc_macsec.c b/drivers/net/phy/mscc/mscc_macsec.c
index f81b077618f4..018253a573b8 100644
--- a/drivers/net/phy/mscc/mscc_macsec.c
+++ b/drivers/net/phy/mscc/mscc_macsec.c
@@ -501,8 +501,7 @@  static u32 vsc8584_macsec_flow_context_id(struct macsec_flow *flow)
 }
 
 /* Derive the AES key to get a key for the hash autentication */
-static int vsc8584_macsec_derive_key(const u8 key[MACSEC_MAX_KEY_LEN],
-				     u16 key_len, u8 hkey[16])
+static int vsc8584_macsec_derive_key(const u8 *key, u16 key_len, u8 hkey[16])
 {
 	const u8 input[AES_BLOCK_SIZE] = {0};
 	struct crypto_aes_ctx ctx;
@@ -518,7 +517,8 @@  static int vsc8584_macsec_derive_key(const u8 key[MACSEC_MAX_KEY_LEN],
 }
 
 static int vsc8584_macsec_transformation(struct phy_device *phydev,
-					 struct macsec_flow *flow)
+					 struct macsec_flow *flow,
+					 const u8 *key)
 {
 	struct vsc8531_private *priv = phydev->priv;
 	enum macsec_bank bank = flow->bank;
@@ -527,7 +527,7 @@  static int vsc8584_macsec_transformation(struct phy_device *phydev,
 	u8 hkey[16];
 	u64 sci;
 
-	ret = vsc8584_macsec_derive_key(flow->key, priv->secy->key_len, hkey);
+	ret = vsc8584_macsec_derive_key(key, priv->secy->key_len, hkey);
 	if (ret)
 		return ret;
 
@@ -563,7 +563,7 @@  static int vsc8584_macsec_transformation(struct phy_device *phydev,
 	for (i = 0; i < priv->secy->key_len / sizeof(u32); i++)
 		vsc8584_macsec_phy_write(phydev, bank,
 					 MSCC_MS_XFORM_REC(index, rec++),
-					 ((u32 *)flow->key)[i]);
+					 ((u32 *)key)[i]);
 
 	/* Set the authentication key */
 	for (i = 0; i < 4; i++)
@@ -632,28 +632,14 @@  static void vsc8584_macsec_free_flow(struct vsc8531_private *priv,
 
 	list_del(&flow->list);
 	clear_bit(flow->index, bitmap);
-	memzero_explicit(flow->key, sizeof(flow->key));
 	kfree(flow);
 }
 
-static int vsc8584_macsec_add_flow(struct phy_device *phydev,
-				   struct macsec_flow *flow, bool update)
+static void vsc8584_macsec_add_flow(struct phy_device *phydev,
+				    struct macsec_flow *flow)
 {
-	int ret;
-
 	flow->port = MSCC_MS_PORT_CONTROLLED;
 	vsc8584_macsec_flow(phydev, flow);
-
-	if (update)
-		return 0;
-
-	ret = vsc8584_macsec_transformation(phydev, flow);
-	if (ret) {
-		vsc8584_macsec_free_flow(phydev->priv, flow);
-		return ret;
-	}
-
-	return 0;
 }
 
 static int vsc8584_macsec_default_flows(struct phy_device *phydev)
@@ -706,6 +692,7 @@  static int __vsc8584_macsec_add_rxsa(struct macsec_context *ctx,
 {
 	struct phy_device *phydev = ctx->phydev;
 	struct vsc8531_private *priv = phydev->priv;
+	int ret;
 
 	flow->assoc_num = ctx->sa.assoc_num;
 	flow->rx_sa = ctx->sa.rx_sa;
@@ -717,19 +704,39 @@  static int __vsc8584_macsec_add_rxsa(struct macsec_context *ctx,
 	if (priv->secy->validate_frames != MACSEC_VALIDATE_DISABLED)
 		flow->match.untagged = 1;
 
-	return vsc8584_macsec_add_flow(phydev, flow, update);
+	vsc8584_macsec_add_flow(phydev, flow);
+
+	if (update)
+		return 0;
+
+	ret = vsc8584_macsec_transformation(phydev, flow, ctx->sa.key);
+	if (ret)
+		vsc8584_macsec_free_flow(phydev->priv, flow);
+
+	return ret;
 }
 
 static int __vsc8584_macsec_add_txsa(struct macsec_context *ctx,
 				     struct macsec_flow *flow, bool update)
 {
+	int ret;
+
 	flow->assoc_num = ctx->sa.assoc_num;
 	flow->tx_sa = ctx->sa.tx_sa;
 
 	/* Always match untagged packets on egress */
 	flow->match.untagged = 1;
 
-	return vsc8584_macsec_add_flow(ctx->phydev, flow, update);
+	vsc8584_macsec_add_flow(ctx->phydev, flow);
+
+	if (update)
+		return 0;
+
+	ret = vsc8584_macsec_transformation(ctx->phydev, flow, ctx->sa.key);
+	if (ret)
+		vsc8584_macsec_free_flow(ctx->phydev->priv, flow);
+
+	return ret;
 }
 
 static int vsc8584_macsec_dev_open(struct macsec_context *ctx)
@@ -829,8 +836,6 @@  static int vsc8584_macsec_add_rxsa(struct macsec_context *ctx)
 	if (IS_ERR(flow))
 		return PTR_ERR(flow);
 
-	memcpy(flow->key, ctx->sa.key, priv->secy->key_len);
-
 	ret = __vsc8584_macsec_add_rxsa(ctx, flow, false);
 	if (ret)
 		return ret;
@@ -882,8 +887,6 @@  static int vsc8584_macsec_add_txsa(struct macsec_context *ctx)
 	if (IS_ERR(flow))
 		return PTR_ERR(flow);
 
-	memcpy(flow->key, ctx->sa.key, priv->secy->key_len);
-
 	ret = __vsc8584_macsec_add_txsa(ctx, flow, false);
 	if (ret)
 		return ret;
diff --git a/drivers/net/phy/mscc/mscc_macsec.h b/drivers/net/phy/mscc/mscc_macsec.h
index 453304bae778..21ce3b892f7f 100644
--- a/drivers/net/phy/mscc/mscc_macsec.h
+++ b/drivers/net/phy/mscc/mscc_macsec.h
@@ -81,8 +81,6 @@  struct macsec_flow {
 	/* Highest takes precedence [0..15] */
 	u8 priority;
 
-	u8 key[MACSEC_MAX_KEY_LEN];
-
 	union {
 		struct macsec_rx_sa *rx_sa;
 		struct macsec_tx_sa *tx_sa;