| 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
| ||
|
Message-ID: <20240513165133.GS2787@kernel.org>
Date: Mon, 13 May 2024 17:51:33 +0100
From: Simon Horman <horms@...nel.org>
To: Bharat Bhushan <bbhushan2@...vell.com>
Cc: netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
sgoutham@...vell.com, gakula@...vell.com, sbhatta@...vell.com,
hkelam@...vell.com, davem@...emloft.net, edumazet@...gle.com,
kuba@...nel.org, pabeni@...hat.com, jerinj@...vell.com,
lcherian@...vell.com, richardcochran@...il.com
Subject: Re: [net-next,v2 5/8] cn10k-ipsec: Add SA add/delete support for
outb inline ipsec
On Mon, May 13, 2024 at 04:24:43PM +0530, Bharat Bhushan wrote:
> This patch adds support to add and delete Security Association
> (SA) xfrm ops. Hardware maintains SA context in memory allocated
> by software. Each SA context is 128 byte aligned and size of
> each context is multiple of 128-byte. Add support for transport
> and tunnel ipsec mode, ESP protocol, aead aes-gcm-icv16, key size
> 128/192/256-bits with 32bit salt.
>
> Signed-off-by: Bharat Bhushan <bbhushan2@...vell.com>
> ---
> v1->v2:
> - Use dma_wmb() instead of architecture specific barrier
>
> .../marvell/octeontx2/nic/cn10k_ipsec.c | 433 +++++++++++++++++-
> .../marvell/octeontx2/nic/cn10k_ipsec.h | 114 +++++
> 2 files changed, 546 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/cn10k_ipsec.c b/drivers/net/ethernet/marvell/octeontx2/nic/cn10k_ipsec.c
...
> @@ -356,6 +362,414 @@ static int cn10k_outb_cpt_clean(struct otx2_nic *pf)
> return err;
> }
>
> +static int cn10k_outb_get_sa_index(struct otx2_nic *pf,
> + struct cn10k_tx_sa_s *sa_entry)
> +{
> + u32 sa_size = pf->ipsec.sa_size;
> + u32 sa_index;
> +
> + if (!sa_entry || ((void *)sa_entry < pf->ipsec.outb_sa->base))
> + return -EINVAL;
> +
> + sa_index = ((void *)sa_entry - pf->ipsec.outb_sa->base) / sa_size;
> + if (sa_index >= CN10K_IPSEC_OUTB_MAX_SA)
> + return -EINVAL;
> +
> + return sa_index;
> +}
> +
> +static dma_addr_t cn10k_outb_get_sa_iova(struct otx2_nic *pf,
> + struct cn10k_tx_sa_s *sa_entry)
> +{
> + u32 sa_index = cn10k_outb_get_sa_index(pf, sa_entry);
> +
> + if (sa_index < 0)
> + return 0;
Should the type of sa_index be int?
That would match the return type of cn10k_outb_get_sa_index.
Otherwise, testing for < 0 will always be false.
Likewise in cn10k_outb_free_sa and cn10k_ipsec_del_state.
Flagged by Smatch.
> + return pf->ipsec.outb_sa->iova + sa_index * pf->ipsec.sa_size;
> +}
...
> +static int cn10k_outb_write_sa(struct otx2_nic *pf, struct cn10k_tx_sa_s *sa_cptr)
> +{
> + dma_addr_t res_iova, dptr_iova, sa_iova;
> + struct cn10k_tx_sa_s *sa_dptr;
> + struct cpt_inst_s inst;
> + struct cpt_res_s *res;
> + u32 sa_size, off;
> + u64 reg_val;
> + int ret;
> +
> + sa_iova = cn10k_outb_get_sa_iova(pf, sa_cptr);
> + if (!sa_iova)
> + return -EINVAL;
> +
> + res = dma_alloc_coherent(pf->dev, sizeof(struct cpt_res_s),
> + &res_iova, GFP_ATOMIC);
> + if (!res)
> + return -ENOMEM;
> +
> + sa_size = sizeof(struct cn10k_tx_sa_s);
> + sa_dptr = dma_alloc_coherent(pf->dev, sa_size, &dptr_iova, GFP_ATOMIC);
> + if (!sa_dptr) {
> + dma_free_coherent(pf->dev, sizeof(struct cpt_res_s), res,
> + res_iova);
> + return -ENOMEM;
> + }
> +
> + for (off = 0; off < (sa_size / 8); off++)
> + *((u64 *)sa_dptr + off) = cpu_to_be64(*((u64 *)sa_cptr + off));
Given the layout of struct cn10k_tx_sa_s, it's not clear
to me how it makes sense for it to be used to store big endian quadwords.
Which is a something that probably ought to be addressed.
But if not, Sparse complains about the endienness of the types used
above. I think it wants:
*((__be64 *)sa_dptr + off)
> +
> + memset(&inst, 0, sizeof(struct cpt_inst_s));
> +
> + res->compcode = CN10K_CPT_COMP_E_NOTDONE;
> + inst.res_addr = res_iova;
> + inst.dptr = (u64)dptr_iova;
> + inst.param2 = sa_size >> 3;
> + inst.dlen = sa_size;
> + inst.opcode_major = CN10K_IPSEC_MAJOR_OP_WRITE_SA;
> + inst.opcode_minor = CN10K_IPSEC_MINOR_OP_WRITE_SA;
> + inst.cptr = sa_iova;
> + inst.ctx_val = 1;
> + inst.egrp = CN10K_DEF_CPT_IPSEC_EGRP;
> +
> + cn10k_cpt_inst_flush(pf, &inst, sizeof(struct cpt_inst_s));
> + dma_wmb();
> + ret = cn10k_wait_for_cpt_respose(pf, res);
> + if (ret)
> + goto out;
> +
> + /* Trigger CTX flush to write dirty data back to DRAM */
> + reg_val = FIELD_PREP(CPT_LF_CTX_FLUSH, sa_iova >> 7);
> + otx2_write64(pf, CN10K_CPT_LF_CTX_FLUSH, reg_val);
> +
> +out:
> + dma_free_coherent(pf->dev, sa_size, sa_dptr, dptr_iova);
> + dma_free_coherent(pf->dev, sizeof(struct cpt_res_s), res, res_iova);
> + return ret;
> +}
...
> +static void cn10k_outb_prepare_sa(struct xfrm_state *x,
> + struct cn10k_tx_sa_s *sa_entry)
> +{
> + int key_len = (x->aead->alg_key_len + 7) / 8;
> + struct net_device *netdev = x->xso.dev;
> + u8 *key = x->aead->alg_key;
> + struct otx2_nic *pf;
> + u32 *tmp_salt;
> + u64 *tmp_key;
> + int idx;
> +
> + memset(sa_entry, 0, sizeof(struct cn10k_tx_sa_s));
> +
> + /* context size, 128 Byte aligned up */
> + pf = netdev_priv(netdev);
> + sa_entry->ctx_size = (pf->ipsec.sa_size / OTX2_ALIGN) & 0xF;
> + sa_entry->hw_ctx_off = cn10k_ipsec_get_hw_ctx_offset();
> + sa_entry->ctx_push_size = cn10k_ipsec_get_ctx_push_size();
> +
> + /* Ucode to skip two words of CPT_CTX_HW_S */
> + sa_entry->ctx_hdr_size = 1;
> +
> + /* Allow Atomic operation (AOP) */
> + sa_entry->aop_valid = 1;
> +
> + /* Outbound, ESP TRANSPORT/TUNNEL Mode, AES-GCM with AES key length
> + * 128bit.
> + */
> + sa_entry->sa_dir = CN10K_IPSEC_SA_DIR_OUTB;
> + sa_entry->ipsec_protocol = CN10K_IPSEC_SA_IPSEC_PROTO_ESP;
> + sa_entry->enc_type = CN10K_IPSEC_SA_ENCAP_TYPE_AES_GCM;
> + if (x->props.mode == XFRM_MODE_TUNNEL)
> + sa_entry->ipsec_mode = CN10K_IPSEC_SA_IPSEC_MODE_TUNNEL;
> + else
> + sa_entry->ipsec_mode = CN10K_IPSEC_SA_IPSEC_MODE_TRANSPORT;
> +
> + sa_entry->spi = cpu_to_be32(x->id.spi);
The type of spi is a 32-bit bitfield of a 64-bit unsigned host endien integer.
1. I suspect it would make more sense to declare that field as a 32bit integer.
2. It is being assigned a big endian value. That doesn't seem right.
The second issue was flagged by Sparse.
> +
> + /* Last 4 bytes are salt */
> + key_len -= 4;
> + sa_entry->aes_key_len = cn10k_ipsec_get_aes_key_len(key_len);
> + memcpy(sa_entry->cipher_key, key, key_len);
> + tmp_key = (u64 *)sa_entry->cipher_key;
> +
> + for (idx = 0; idx < key_len / 8; idx++)
> + tmp_key[idx] = be64_to_cpu(tmp_key[idx]);
More endian problems flagged by Sparse on this line.
An integer variable should typically be used to store
a big endian value, a little endian value, or a host endian value.
Not more than one of these.
This is because tooling such as Sparse can then be used to verify
the correctness of the endian used.
> +
> + memcpy(&sa_entry->iv_gcm_salt, key + key_len, 4);
> + tmp_salt = (u32 *)&sa_entry->iv_gcm_salt;
> + *tmp_salt = be32_to_cpu(*tmp_salt);
Likewise here.
> +
> + /* Write SA context data to memory before enabling */
> + wmb();
> +
> + /* Enable SA */
> + sa_entry->sa_valid = 1;
> +}
...
> +static int cn10k_ipsec_add_state(struct xfrm_state *x,
> + struct netlink_ext_ack *extack)
> +{
> + struct net_device *netdev = x->xso.dev;
> + struct cn10k_tx_sa_s *sa_entry;
> + struct cpt_ctx_info_s *sa_info;
> + struct otx2_nic *pf;
> + int err;
> +
> + err = cn10k_ipsec_validate_state(x);
> + if (err)
> + return err;
> +
> + if (x->xso.dir == XFRM_DEV_OFFLOAD_IN) {
> + netdev_err(netdev, "xfrm inbound offload not supported\n");
> + err = -ENODEV;
This path results in pf being dereferenced while uninitialised
towards the bottom of this function.
Flagged by Smatch, and Clang-18 W=1 build
> + } else {
> + pf = netdev_priv(netdev);
> + if (!mutex_trylock(&pf->ipsec.lock)) {
> + netdev_err(netdev, "IPSEC device is busy\n");
> + return -EBUSY;
> + }
> +
> + if (!(pf->flags & OTX2_FLAG_INLINE_IPSEC_ENABLED)) {
> + netdev_err(netdev, "IPSEC not enabled/supported on device\n");
> + err = -ENODEV;
> + goto unlock;
> + }
> +
> + sa_entry = cn10k_outb_alloc_sa(pf);
> + if (!sa_entry) {
> + netdev_err(netdev, "SA maximum limit %x reached\n",
> + CN10K_IPSEC_OUTB_MAX_SA);
> + err = -EBUSY;
> + goto unlock;
> + }
> +
> + cn10k_outb_prepare_sa(x, sa_entry);
> +
> + err = cn10k_outb_write_sa(pf, sa_entry);
> + if (err) {
> + netdev_err(netdev, "Error writing outbound SA\n");
> + cn10k_outb_free_sa(pf, sa_entry);
> + goto unlock;
> + }
> +
> + sa_info = kmalloc(sizeof(*sa_info), GFP_KERNEL);
> + sa_info->sa_entry = sa_entry;
> + sa_info->sa_iova = cn10k_outb_get_sa_iova(pf, sa_entry);
> + x->xso.offload_handle = (unsigned long)sa_info;
> + }
> +
> +unlock:
> + mutex_unlock(&pf->ipsec.lock);
> + return err;
> +}
...
> +static const struct xfrmdev_ops cn10k_ipsec_xfrmdev_ops = {
> + .xdo_dev_state_add = cn10k_ipsec_add_state,
> + .xdo_dev_state_delete = cn10k_ipsec_del_state,
> +};
> +
cn10k_ipsec_xfrmdev_ops is unused.
Perhaps it, along with it's callbacks,
should be added by the function that uses it?
Flagged by W=1 builds.
> int cn10k_ipsec_ethtool_init(struct net_device *netdev, bool enable)
> {
> struct otx2_nic *pf = netdev_priv(netdev);
...
> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/cn10k_ipsec.h b/drivers/net/ethernet/marvell/octeontx2/nic/cn10k_ipsec.h
...
> +struct cn10k_tx_sa_s {
> + u64 esn_en : 1; /* W0 */
> + u64 rsvd_w0_1_8 : 8;
> + u64 hw_ctx_off : 7;
> + u64 ctx_id : 16;
> + u64 rsvd_w0_32_47 : 16;
> + u64 ctx_push_size : 7;
> + u64 rsvd_w0_55 : 1;
> + u64 ctx_hdr_size : 2;
> + u64 aop_valid : 1;
> + u64 rsvd_w0_59 : 1;
> + u64 ctx_size : 4;
> + u64 w1; /* W1 */
> + u64 sa_valid : 1; /* W2 */
> + u64 sa_dir : 1;
> + u64 rsvd_w2_2_3 : 2;
> + u64 ipsec_mode : 1;
> + u64 ipsec_protocol : 1;
> + u64 aes_key_len : 2;
> + u64 enc_type : 3;
> + u64 rsvd_w2_11_31 : 21;
> + u64 spi : 32;
> + u64 w3; /* W3 */
> + u8 cipher_key[32]; /* W4 - W7 */
> + u32 rsvd_w8_0_31; /* W8 : IV */
> + u32 iv_gcm_salt;
> + u64 rsvd_w9_w30[22]; /* W9 - W30 */
> + u64 hw_ctx[6]; /* W31 - W36 */
> +};
...
Powered by blists - more mailing lists