Messages in this thread | | | Date | Mon, 11 Sep 2023 18:57:26 +0300 | Subject | Re: [RFC net-next v3 4/6] net: phy: nxp-c45-tja11xx: add MACsec support | From | "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.
| |