netdev - Re: [PATCH net-next] netdevsim: add dummy macsec offload

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <ZIcRxM/xvozZ+H9c@corigine.com>
Date: Mon, 12 Jun 2023 14:38:28 +0200
From: Simon Horman <simon.horman@...igine.com>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: netdev@...r.kernel.org, kuba@...nel.org
Subject: Re: [PATCH net-next] netdevsim: add dummy macsec offload

On Sun, Jun 11, 2023 at 05:45:33PM +0200, Sabrina Dubroca wrote:

...

> diff --git a/drivers/net/netdevsim/macsec.c b/drivers/net/netdevsim/macsec.c
> new file mode 100644
> index 000000000000..355ba2f313df
> --- /dev/null

...

> +static int nsim_macsec_add_secy(struct macsec_context *ctx)
> +{
> +	struct netdevsim *ns = netdev_priv(ctx->netdev);
> +	int idx;
> +
> +	if (ns->macsec.nsim_secy_count == NSIM_MACSEC_MAX_SECY_COUNT)
> +		return -ENOSPC;
> +
> +	for (idx = 0; idx < NSIM_MACSEC_MAX_SECY_COUNT; idx++) {
> +		if (!ns->macsec.nsim_secy[idx].used)
> +			break;
> +	}
> +
> +	if (idx == NSIM_MACSEC_MAX_SECY_COUNT)
> +		netdev_err(ctx->netdev, "%s: nsim_secy_count not full but all SecYs used\n",
> +			   __func__);

Hi Sabrina,

It seems that if this condition is met, then ns->macsec.nsim_secy will
overflow below.

> +
> +	netdev_dbg(ctx->netdev, "%s: adding new secy with sci %08llx at index %d\n",
> +		   __func__, be64_to_cpu(ctx->secy->sci), idx);
> +	ns->macsec.nsim_secy[idx].used = true;
> +	ns->macsec.nsim_secy[idx].nsim_rxsc_count = 0;
> +	ns->macsec.nsim_secy[idx].sci = ctx->secy->sci;
> +	ns->macsec.nsim_secy_count++;
> +
> +	return 0;
> +}

...

> +static int nsim_macsec_add_txsa(struct macsec_context *ctx)
> +{
> +	struct netdevsim *ns = netdev_priv(ctx->netdev);
> +	struct nsim_secy *secy;
> +	int idx;
> +
> +	idx = nsim_macsec_find_secy(ns, ctx->secy->sci);
> +	if (idx < 0) {
> +		netdev_err(ctx->netdev, "%s: sci %08llx not found in secy table\n",
> +			   __func__, be64_to_cpu(ctx->secy->sci));

Sparse seems pretty unhappy about the type of the argement to be64_to_cpu()
here and elsewhere. I'm unsure what is the best option but one that
sprang to mind would be conversion helpers, that cast appropriately.
f.e. sci_to_cpu()

> +		return -ENOENT;
> +	}
> +	secy = &ns->macsec.nsim_secy[idx];

As also reported by the kernel test robot, a W=1 build complains that secy
is set but unused here and in to other places below.

> +
> +	netdev_dbg(ctx->netdev, "%s: SECY with sci %08llx, AN %u\n",
> +		   __func__, be64_to_cpu(ctx->secy->sci), ctx->sa.assoc_num);
> +
> +	return 0;
> +}
> +
> +static int nsim_macsec_upd_txsa(struct macsec_context *ctx)
> +{
> +	struct netdevsim *ns = netdev_priv(ctx->netdev);
> +	struct nsim_secy *secy;
> +	int idx;
> +
> +	idx = nsim_macsec_find_secy(ns, ctx->secy->sci);
> +	if (idx < 0) {
> +		netdev_err(ctx->netdev, "%s: sci %08llx not found in secy table\n",
> +			   __func__, be64_to_cpu(ctx->secy->sci));
> +		return -ENOENT;
> +	}
> +	secy = &ns->macsec.nsim_secy[idx];
> +
> +	netdev_dbg(ctx->netdev, "%s: SECY with sci %08llx, AN %u\n",
> +		   __func__, be64_to_cpu(ctx->secy->sci), ctx->sa.assoc_num);
> +
> +	return 0;
> +}
> +
> +static int nsim_macsec_del_txsa(struct macsec_context *ctx)
> +{
> +	struct netdevsim *ns = netdev_priv(ctx->netdev);
> +	struct nsim_secy *secy;
> +	int idx;
> +
> +	idx = nsim_macsec_find_secy(ns, ctx->secy->sci);
> +	if (idx < 0) {
> +		netdev_err(ctx->netdev, "%s: sci %08llx not found in secy table\n",
> +			   __func__, be64_to_cpu(ctx->secy->sci));
> +		return -ENOENT;
> +	}
> +	secy = &ns->macsec.nsim_secy[idx];
> +
> +	netdev_dbg(ctx->netdev, "%s: SECY with sci %08llx, AN %u\n",
> +		   __func__, be64_to_cpu(ctx->secy->sci), ctx->sa.assoc_num);
> +
> +	return 0;
> +}

...