[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <e169bc7e-b0af-a87b-e549-5fdcf447f381@oss.nxp.com>
Date: Mon, 11 Sep 2023 18:57:26 +0300
From: "Radu Pirea (OSS)" <radu-nicolae.pirea@....nxp.com>
To: Sabrina Dubroca <sd@...asysnail.net>
Cc: andrew@...n.ch, hkallweit1@...il.com, linux@...linux.org.uk,
davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org,
pabeni@...hat.com, richardcochran@...il.com,
sebastian.tobuschat@....com, netdev@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [RFC net-next v3 4/6] net: phy: nxp-c45-tja11xx: add MACsec
support
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.
Powered by blists - more mailing lists