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

[Radu Pirea (OSS)]

On 11.09.2023 15:00, Sabrina Dubroca wrote:
> 2023-09-06, 19:01:32 +0300, Radu Pirea (NXP OSS) wrote:
> > +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..

This port number restriction is valid only if end_station is true.
In nxp_c45_mdo_add_rxsc I forgot to check end_station flag.

> > +
> > +	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?

To reuse nxp_c45_tx_sa_update.

> > +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

Here the tx sa 0 pn is set 1. I will store the initial pn, and if the sa 
is updated with the same pn or if the new pn is 0, I will not update it.
Thank you for pointing that out.

> 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;
> > +}

--
Radu P.