Hi,
>From: Kalesh Anakkur Purayil <mailto:[email protected]>
>Sent: Monday, July 17, 2023 10:50 AM
>To: Subbaraya Sundeep Bhatta <mailto:[email protected]>
>Cc: mailto:[email protected]; mailto:[email protected];
>mailto:[email protected]; mailto:[email protected];
>mailto:[email protected]; mailto:[email protected]; Sunil Kovvuri
>Goutham <mailto:[email protected]>; Geethasowjanya Akula
><mailto:[email protected]>; Hariprasad Kelam
><mailto:[email protected]>; Naveen Mamindlapalli
><mailto:[email protected]>
>Subject: Re: [net PATCH] octeontx2-pf: mcs: Generate hash key using ecb(aes)
>
>
>
>On Mon, Jul 17, 2023 at 10:20 AM Subbaraya Sundeep Bhatta
><mailto:[email protected]> wrote:
>Hi,
>
>>From: Kalesh Anakkur Purayil <mailto:[email protected]>
>>Sent: Sunday, July 16, 2023 4:20 PM
>>To: Subbaraya Sundeep Bhatta <mailto:[email protected]>
>>Cc: mailto:[email protected]; mailto:[email protected];
>mailto:[email protected];
>>mailto:[email protected]; mailto:[email protected];
>mailto:[email protected]; Sunil
>>Kovvuri Goutham <mailto:[email protected]>; Geethasowjanya Akula
>><mailto:[email protected]>; Hariprasad Kelam
><mailto:[email protected]>; Naveen
>>Mamindlapalli <mailto:[email protected]>
>>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:mailto:[email protected]> 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:mailto:[email protected]>
>>---
>> .../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.
>[Kalesh] Sorry, I did not get that.
>Why to invoke skcipher_request_free() when skcipher_request_alloc() fails? Am I
>missing something here?
>
I avoided another label since we can call skcipher_request_free even input req is NULL.
Anyway will add new label so that it is easier to understand and send v2. Thanks for review.
Sundeep
>>+ 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
>
>
>
>--
>Regards,
>Kalesh A P