lkml.org 
[lkml]   [2023]   [Sep]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [RFC net-next v3 4/6] net: phy: nxp-c45-tja11xx: add MACsec support
From


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.

\
 
 \ /
  Last update: 2023-09-12 00:22    [W:0.104 / U:0.060 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site