Re: [RFC net-next v3 4/6] net: phy: nxp-c45-tja11xx: add MACsec support

[Sabrina Dubroca <sd@queasysnail.net>]

2023-09-06, 19:01:32 +0300, Radu Pirea (NXP OSS) wrote:
> Changes in v3:
> - removed struct nxp_c45_rx_sc
> - replaced struct nxp_c45_tx_sa with struct nxp_c45_sa
> - reworked the implementation around struct nxp_c45_sa
> - various renamings
> - tried to better group the functions by SA type/SC type
> - no key is stored in the driver
> - TX SAs limited to 2 insted of 4(no key in the driver consequence)
> - used sci_to_cpu where in various functions
> - improved debug information
> - nxp_c45_secy_valid function reworked
> - merged TX/RX SA set key functions
> - merged TX/RX SA set pn functions
> - tried to stick to tx_sa/rx_sa/rx_sc/tx_sc function naming. 
> - nxp_c45_macsec_config_init will return an error if a write fails.
> - MACSEC_TXSC_CFG_SCI renamed to MACSEC_TXSC_CFG_SCB
> - return -ENOSPC if no SC/SA available in the hardware
> - phydev->macsec_ops allocated using devm_kzalloc

nit: not macsec_ops


[...]
> +static void nxp_c45_set_sci(struct phy_device *phydev, u16 sci_base_addr,
> +			    sci_t sci)
> +{
> +	u64 lsci = sci_to_cpu(sci);
> +
> +	nxp_c45_macsec_write(phydev, sci_base_addr, lsci >> 32);
> +	nxp_c45_macsec_write(phydev, sci_base_addr + 4, lsci);
> +}
> +
> +static bool nxp_c45_sci_valid(sci_t sci, bool scb)
> +{
> +	u16 port = sci_to_cpu(sci);
> +
> +	if (scb && port != 0)
> +		return false;
> +	if (!scb && port != 1)
> +		return false;

For non-SCB (ie normal case), only port 1 is allowed? That doesn't
seem to match what nxp_c45_rx_sc_valid was doing in v2, but it is also
called from nxp_c45_mdo_add_rxsc..

> +
> +	return true;
> +}
> +

[...]
> +static void nxp_c45_tx_sa_next(struct nxp_c45_secy *phy_secy,
> +			       struct nxp_c45_sa *next_sa, u8 encoding_sa)
> +{
> +	struct nxp_c45_sa *sa;
> +
> +	sa = nxp_c45_find_sa(&phy_secy->sa_list, TX_SA, encoding_sa);
> +	if (!IS_ERR(sa)) {
> +		memcpy(next_sa, sa, sizeof(*sa));
> +	} else {
> +		next_sa->is_key_a = true;
> +		next_sa->an = encoding_sa;
> +	}
> +}

What is this doing? Why are you filling a fake SA struct when none is
currently configured?



> +static int nxp_c45_mdo_upd_txsa(struct macsec_context *ctx)
> +{
> +	struct macsec_tx_sa *tx_sa = ctx->sa.tx_sa;
> +	struct phy_device *phydev = ctx->phydev;
> +	struct nxp_c45_phy *priv = phydev->priv;
> +	struct nxp_c45_secy *phy_secy;
> +	u8 an = ctx->sa.assoc_num;
> +	struct nxp_c45_sa *sa;
> +
> +	phydev_dbg(phydev, "update TX SA %u %s to TX SC %016llx\n",
> +		   an, ctx->sa.tx_sa->active ? "enabled" : "disabled",
> +		   sci_to_cpu(ctx->secy->sci));
> +
> +	phy_secy = nxp_c45_find_secy(&priv->macsec->secy_list, ctx->secy->sci);
> +	if (IS_ERR(phy_secy))
> +		return PTR_ERR(phy_secy);
> +
> +	sa = nxp_c45_find_sa(&phy_secy->sa_list, TX_SA, an);
> +	if (IS_ERR(sa))
> +		return PTR_ERR(sa);
> +
> +	nxp_c45_select_secy(phydev, phy_secy->secy_id);
> +	nxp_c45_sa_set_pn(phydev, sa, tx_sa->next_pn, 0);

The macsec core doesn't increment its PN when we're offloading. If
userspace didn't pass a new PN, aren't we resetting the HW's PN back
to its initial value here? That would cause replay protection to fail,
and the PN reuse would break GCM.

Could you check by inspecting the sequence numbers sent by the device
before and after this:

    ip macsec set macsec0 tx sa 0 on


And same for nxp_c45_mdo_upd_rxsa -> nxp_c45_sa_set_pn. Testing would
require enabling replay protection and making the PN go backward on
the TX side.

> +	nxp_c45_tx_sa_update(phydev, sa, ctx->secy->tx_sc.encoding_sa,
> +			     tx_sa->active);
> +	return 0;
> +}

-- 
Sabrina