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

["Radu Pirea (OSS)" <radu-nicolae.pirea@oss.nxp.com>]

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.