RE: [EXT] Re: [net PATCH] octeontx2-pf: mcs: Generate hash key using ecb(aes)
From: Subbaraya Sundeep Bhatta
Date: Mon Jul 17 2023 - 00:51:00 EST
Hi,
>From: Kalesh Anakkur Purayil <kalesh-anakkur.purayil@xxxxxxxxxxxx>
>Sent: Sunday, July 16, 2023 4:20 PM
>To: Subbaraya Sundeep Bhatta <sbhatta@xxxxxxxxxxx>
>Cc: netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; kuba@xxxxxxxxxx;
>davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; pabeni@xxxxxxxxxx; Sunil
>Kovvuri Goutham <sgoutham@xxxxxxxxxxx>; Geethasowjanya Akula
><gakula@xxxxxxxxxxx>; Hariprasad Kelam <hkelam@xxxxxxxxxxx>; Naveen
>Mamindlapalli <naveenm@xxxxxxxxxxx>
>Subject: Re: [net PATCH] octeontx2-pf: mcs: Generate hash key using ecb(aes)
>
>
>On Fri, Jul 14, 2023 at 9:53 PM Subbaraya Sundeep
><mailto:sbhatta@xxxxxxxxxxx> wrote:
>Hardware generated encryption and ICV tags are found to
>be wrong when tested with IEEE MACSEC test vectors.
>This is because as per the HRM, the hash key (derived by
>AES-ECB block encryption of an all 0s block with the SAK)
>has to be programmed by the software in
>MCSX_RS_MCS_CPM_TX_SLAVE_SA_PLCY_MEM_4X register.
>Hence fix this by generating hash key in software and
>configuring in hardware.
>
>Fixes: c54ffc73601c ("octeontx2-pf: mcs: Introduce MACSEC hardware
>offloading")
>Signed-off-by: Subbaraya Sundeep <mailto:sbhatta@xxxxxxxxxxx>
>---
> .../ethernet/marvell/octeontx2/nic/cn10k_macsec.c | 132 +++++++++++++++---
>---
> 1 file changed, 95 insertions(+), 37 deletions(-)
>
>diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/cn10k_macsec.c
>b/drivers/net/ethernet/marvell/octeontx2/nic/cn10k_macsec.c
>index 6e2fb24..9f23118 100644
>--- a/drivers/net/ethernet/marvell/octeontx2/nic/cn10k_macsec.c
>+++ b/drivers/net/ethernet/marvell/octeontx2/nic/cn10k_macsec.c
>@@ -4,6 +4,7 @@
> * Copyright (C) 2022 Marvell.
> */
>
>+#include <crypto/skcipher.h>
> #include <linux/rtnetlink.h>
> #include <linux/bitfield.h>
> #include "otx2_common.h"
>@@ -42,6 +43,51 @@
> #define MCS_TCI_E 0x08 /* encryption */
> #define MCS_TCI_C 0x04 /* changed text */
>
>+#define CN10K_MAX_HASH_LEN 16
>+#define CN10K_MAX_SAK_LEN 32
>+
>+static int cn10k_ecb_aes_encrypt(struct otx2_nic *pfvf, u8 *sak,
>+ u16 sak_len, u8 *hash)
>+{
>+ u8 data[CN10K_MAX_HASH_LEN] = { 0 };
>[Kalesh]: There is no need in 0 here, just use {}
>
Input has to be all zeroes. AES-ECB block encryption of an all 0s block with the SAK key.
Hence needed.
>+ struct skcipher_request *req = NULL;
>+ struct scatterlist sg_src, sg_dst;
>+ struct crypto_skcipher *tfm;
>+ DECLARE_CRYPTO_WAIT(wait);
>+ int err;
>+
>+ tfm = crypto_alloc_skcipher("ecb(aes)", 0, 0);
>+ if (IS_ERR(tfm)) {
>+ dev_err(pfvf->dev, "failed to allocate transform for ecb-aes\n");
>+ return PTR_ERR(tfm);
>+ }
>+
>+ req = skcipher_request_alloc(tfm, GFP_KERNEL);
>+ if (!req) {
>+ dev_err(pfvf->dev, "failed to allocate request for skcipher\n");
>+ err = -ENOMEM;
>+ goto out;
>+ }
>+
>+ err = crypto_skcipher_setkey(tfm, sak, sak_len);
>[Kalesh]: No need for a return value check here?
Missed it. Will add.
>+
>+ /* build sg list */
>+ sg_init_one(&sg_src, data, CN10K_MAX_HASH_LEN);
>+ sg_init_one(&sg_dst, hash, CN10K_MAX_HASH_LEN);
>+
>+ skcipher_request_set_callback(req, 0, crypto_req_done, &wait);
>+ skcipher_request_set_crypt(req, &sg_src, &sg_dst,
>+ CN10K_MAX_HASH_LEN, NULL);
>+
>+ err = crypto_skcipher_encrypt(req);
>+ err = crypto_wait_req(err, &wait);
>+
>+out:
>+ skcipher_request_free(req);
>[Kalesh]: I think you should move the label here.
No. After adding the new check, label must be above only.
>+ crypto_free_skcipher(tfm);
>+ return err;
>+}
>+
> static struct cn10k_mcs_txsc *cn10k_mcs_get_txsc(struct cn10k_mcs_cfg *cfg,
> struct macsec_secy *secy)
> {
>@@ -330,19 +376,53 @@ static int cn10k_mcs_write_sc_cam(struct otx2_nic
>*pfvf,
> return ret;
> }
>
>+static int cn10k_mcs_write_keys(struct otx2_nic *pfvf,
>+ struct macsec_secy *secy,
>+ struct mcs_sa_plcy_write_req *req,
>+ u8 *sak, u8 *salt, ssci_t ssci)
>+{
>+ u8 hash_rev[CN10K_MAX_HASH_LEN] = { 0 };
>[Kalesh]: There is no need in 0 here, just use {}
>
Okay
Thanks,
Sundeep
>+ u8 sak_rev[CN10K_MAX_SAK_LEN] = { 0 };
>+ u8 salt_rev[MACSEC_SALT_LEN] = { 0 };
>+ u8 hash[CN10K_MAX_HASH_LEN] = { 0 };
>+ u32 ssci_63_32;
>+ int err, i;
>+
>+ err = cn10k_ecb_aes_encrypt(pfvf, sak, secy->key_len, hash);
>+ if (err) {
>+ dev_err(pfvf->dev, "Generating hash using ECB(AES) failed\n");
>+ return err;
>+ }
>+
>+ for (i = 0; i < secy->key_len; i++)
>+ sak_rev[i] = sak[secy->key_len - 1 - i];
>+
>+ for (i = 0; i < CN10K_MAX_HASH_LEN; i++)
>+ hash_rev[i] = hash[CN10K_MAX_HASH_LEN - 1 - i];
>+
>+ for (i = 0; i < MACSEC_SALT_LEN; i++)
>+ salt_rev[i] = salt[MACSEC_SALT_LEN - 1 - i];
>+
>+ ssci_63_32 = (__force u32)cpu_to_be32((__force u32)ssci);
>+
>+ memcpy(&req->plcy[0][0], sak_rev, secy->key_len);
>+ memcpy(&req->plcy[0][4], hash_rev, CN10K_MAX_HASH_LEN);
>+ memcpy(&req->plcy[0][6], salt_rev, MACSEC_SALT_LEN);
>+ req->plcy[0][7] |= (u64)ssci_63_32 << 32;
>+
>+ return 0;
>+}
>+
> static int cn10k_mcs_write_rx_sa_plcy(struct otx2_nic *pfvf,
> struct macsec_secy *secy,
> struct cn10k_mcs_rxsc *rxsc,
> u8 assoc_num, bool sa_in_use)
> {
>- unsigned char *src = rxsc->sa_key[assoc_num];
> struct mcs_sa_plcy_write_req *plcy_req;
>- u8 *salt_p = rxsc->salt[assoc_num];
>+ u8 *sak = rxsc->sa_key[assoc_num];
>+ u8 *salt = rxsc->salt[assoc_num];
> struct mcs_rx_sc_sa_map *map_req;
> struct mbox *mbox = &pfvf->mbox;
>- u64 ssci_salt_95_64 = 0;
>- u8 reg, key_len;
>- u64 salt_63_0;
> int ret;
>
> mutex_lock(&mbox->lock);
>@@ -360,20 +440,10 @@ static int cn10k_mcs_write_rx_sa_plcy(struct otx2_nic
>*pfvf,
> goto fail;
> }
>
>- for (reg = 0, key_len = 0; key_len < secy->key_len; key_len += 8) {
>- memcpy((u8 *)&plcy_req->plcy[0][reg],
>- (src + reg * 8), 8);
>- reg++;
>- }
>-
>- if (secy->xpn) {
>- memcpy((u8 *)&salt_63_0, salt_p, 8);
>- memcpy((u8 *)&ssci_salt_95_64, salt_p + 8, 4);
>- ssci_salt_95_64 |= (__force u64)rxsc->ssci[assoc_num] << 32;
>-
>- plcy_req->plcy[0][6] = salt_63_0;
>- plcy_req->plcy[0][7] = ssci_salt_95_64;
>- }
>+ ret = cn10k_mcs_write_keys(pfvf, secy, plcy_req, sak,
>+ salt, rxsc->ssci[assoc_num]);
>+ if (ret)
>+ goto fail;
>
> plcy_req->sa_index[0] = rxsc->hw_sa_id[assoc_num];
> plcy_req->sa_cnt = 1;
>@@ -586,13 +656,10 @@ static int cn10k_mcs_write_tx_sa_plcy(struct otx2_nic
>*pfvf,
> struct cn10k_mcs_txsc *txsc,
> u8 assoc_num)
> {
>- unsigned char *src = txsc->sa_key[assoc_num];
> struct mcs_sa_plcy_write_req *plcy_req;
>- u8 *salt_p = txsc->salt[assoc_num];
>+ u8 *sak = txsc->sa_key[assoc_num];
>+ u8 *salt = txsc->salt[assoc_num];
> struct mbox *mbox = &pfvf->mbox;
>- u64 ssci_salt_95_64 = 0;
>- u8 reg, key_len;
>- u64 salt_63_0;
> int ret;
>
> mutex_lock(&mbox->lock);
>@@ -603,19 +670,10 @@ static int cn10k_mcs_write_tx_sa_plcy(struct otx2_nic
>*pfvf,
> goto fail;
> }
>
>- for (reg = 0, key_len = 0; key_len < secy->key_len; key_len += 8) {
>- memcpy((u8 *)&plcy_req->plcy[0][reg], (src + reg * 8), 8);
>- reg++;
>- }
>-
>- if (secy->xpn) {
>- memcpy((u8 *)&salt_63_0, salt_p, 8);
>- memcpy((u8 *)&ssci_salt_95_64, salt_p + 8, 4);
>- ssci_salt_95_64 |= (__force u64)txsc->ssci[assoc_num] << 32;
>-
>- plcy_req->plcy[0][6] = salt_63_0;
>- plcy_req->plcy[0][7] = ssci_salt_95_64;
>- }
>+ ret = cn10k_mcs_write_keys(pfvf, secy, plcy_req, sak,
>+ salt, txsc->ssci[assoc_num]);
>+ if (ret)
>+ goto fail;
>
> plcy_req->plcy[0][8] = assoc_num;
> plcy_req->sa_index[0] = txsc->hw_sa_id[assoc_num];
>--
>2.7.4
>
>
>
>
>--
>Regards,
>Kalesh A P