netdev - Re: [PATCH net-next v3 3/3] nfp: implement xfrm callbacks and expose ipsec offload feature to upper layer

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]

Message-ID: <Y2gPelnt3xfgDGYd@unreal>
Date:   Sun, 6 Nov 2022 21:48:10 +0200
From:   Leon Romanovsky <leon@...nel.org>
To:     Simon Horman <simon.horman@...igine.com>
Cc:     David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Steffen Klassert <steffen.klassert@...unet.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Chentian Liu <chengtian.liu@...igine.com>,
        Huanhuan Wang <huanhuan.wang@...igine.com>,
        Yinjun Zhang <yinjun.zhang@...igine.com>,
        Louis Peens <louis.peens@...igine.com>, netdev@...r.kernel.org,
        oss-drivers@...igine.com
Subject: Re: [PATCH net-next v3 3/3] nfp: implement xfrm callbacks and expose
 ipsec offload feature to upper layer

On Tue, Nov 01, 2022 at 12:02:48PM +0100, Simon Horman wrote:
> From: Huanhuan Wang <huanhuan.wang@...igine.com>
> 
> Xfrm callbacks are implemented to offload SA info into firmware
> by mailbox. It supports 16K SA info in total.
> 
> Expose ipsec offload feature to upper layer, this feature will
> signal the availability of the offload.
> 
> Based on initial work of Norm Bagley <norman.bagley@...ronome.com>.
> 
> Signed-off-by: Huanhuan Wang <huanhuan.wang@...igine.com>
> Reviewed-by: Louis Peens <louis.peens@...igine.com>
> Signed-off-by: Simon Horman <simon.horman@...igine.com>
> ---
>  .../net/ethernet/netronome/nfp/crypto/ipsec.c | 532 +++++++++++++++++-
>  .../ethernet/netronome/nfp/nfp_net_common.c   |   6 +
>  .../net/ethernet/netronome/nfp/nfp_net_ctrl.h |   4 +-
>  3 files changed, 538 insertions(+), 4 deletions(-)

<...>

>  static int nfp_net_xfrm_add_state(struct xfrm_state *x)
>  {
> -	return -EOPNOTSUPP;
> +	struct net_device *netdev = x->xso.dev;
> +	struct nfp_ipsec_cfg_mssg msg = {0};

I think that I already wrote it {0} -> {};

> +	int i, key_len, trunc_len, err = 0;
> +	struct nfp_ipsec_cfg_add_sa *cfg;
> +	struct nfp_net *nn;
> +	unsigned int saidx;
> +	__be32 *p;

<...>

> +		if (trunc_len == 96)
> +			cfg->ctrl_word.hash = NFP_IPSEC_HASH_MD5_96;
> +		else if (trunc_len == 128)
> +			cfg->ctrl_word.hash = NFP_IPSEC_HASH_MD5_128;
> +		else
> +			trunc_len = 0;

IMHO, this is better to write as switch-case in separate function.

> +		break;
> +	case SADB_AALG_SHA1HMAC:
> +		if (trunc_len == 96)
> +			cfg->ctrl_word.hash = NFP_IPSEC_HASH_SHA1_96;
> +		else if (trunc_len == 80)
> +			cfg->ctrl_word.hash = NFP_IPSEC_HASH_SHA1_80;
> +		else
> +			trunc_len = 0;
> +		break;

Ditto.

> +	case SADB_X_AALG_SHA2_256HMAC:
> +		if (trunc_len == 96)
> +			cfg->ctrl_word.hash = NFP_IPSEC_HASH_SHA256_96;
> +		else if (trunc_len == 128)
> +			cfg->ctrl_word.hash = NFP_IPSEC_HASH_SHA256_128;
> +		else
> +			trunc_len = 0;
> +		break;
> +	case SADB_X_AALG_SHA2_384HMAC:
> +		if (trunc_len == 96)
> +			cfg->ctrl_word.hash = NFP_IPSEC_HASH_SHA384_96;
> +		else if (trunc_len == 192)
> +			cfg->ctrl_word.hash = NFP_IPSEC_HASH_SHA384_192;
> +		else
> +			trunc_len = 0;
> +		break;
> +	case SADB_X_AALG_SHA2_512HMAC:
> +		if (trunc_len == 96)
> +			cfg->ctrl_word.hash = NFP_IPSEC_HASH_SHA512_96;
> +		else if (trunc_len == 256)
> +			cfg->ctrl_word.hash = NFP_IPSEC_HASH_SHA512_256;
> +		else
> +			trunc_len = 0;
> +		break;
> +	default:
> +		nn_err(nn, "Unsupported authentication algorithm\n");
> +		return -EINVAL;
> +	}
> +
> +	if (!trunc_len) {
> +		nn_err(nn, "Unsupported authentication algorithm trunc length\n");
> +		return -EINVAL;
> +	}
> +
> +	if (x->aalg) {
> +		p = (__be32 *)x->aalg->alg_key;
> +		key_len = DIV_ROUND_UP(x->aalg->alg_key_len, BITS_PER_BYTE);
> +		if (key_len > sizeof(cfg->auth_key)) {
> +			nn_err(nn, "Insufficient space for offloaded auth key\n");
> +			return -EINVAL;
> +		}
> +		for (i = 0; i < key_len / sizeof(cfg->auth_key[0]) ; i++)
> +			cfg->auth_key[i] = ntohl(*p++);

I wonder if you can't declare p as u32 and use memcpy here instead of
u32->__be32->u32 conversions.

Thanks