linux-kernel - Re: [PATCH] net: macsec: XPN Salt copied before passing offload context

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 05 May 2022 15:28:05 +0200
From:   Antoine Tenart <atenart@...nel.org>
To:     Carlos Fernandez <carlos.escuin@...il.com>,
        carlos.fernandez@...hnica-enineering.de, davem@...emloft.net,
        edumazet@...gle.com, kuba@...nel.org, linux-kernel@...r.kernel.org,
        netdev@...r.kernel.org, pabeni@...hat.com
Cc:     Carlos Fernandez <carlos.fernandez@...hnica-engineering.de>
Subject: Re: [PATCH] net: macsec: XPN Salt copied before passing offload context

Hello,

(Note: please use "[PATCH net]" for fixes and "[PATCH net-next]" for
improvements in the subject when submitting patches to the networking
subsystem).

Quoting Carlos Fernandez (2022-05-05 14:38:03)
> When macsec offloading is used with XPN, before mdo_add_rxsa
> and mdo_add_txsa functions are called, the key salt is not
> copied to the macsec context struct.
> 
> Fix by copying salt to context struct before calling the
> offloading functions.

The commit message and title are referring to the XPN salt only, but
there is another XPN specific entry being moved by this commit. I would
suggest to update the commit title to:
"net: macsec: retrieve the XPN attributes before offloading"

> Fixes: 48ef50fa866a ("macsec: Netlink support of XPN cipher suites")
> Signed-off-by: Carlos Fernandez <carlos.fernandez@...hnica-engineering.de>
> ---
>  drivers/net/macsec.c | 30 ++++++++++++++++--------------
>  1 file changed, 16 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
> index 832f09ac075e..4f2bd3d722c3 100644
> --- a/drivers/net/macsec.c
> +++ b/drivers/net/macsec.c
> @@ -1804,6 +1804,14 @@ static int macsec_add_rxsa(struct sk_buff *skb, struct genl_info *info)
>  
>         rx_sa->sc = rx_sc;
>  
> +       if (secy->xpn) {
> +               rx_sa->ssci = nla_get_ssci(tb_sa[MACSEC_SA_ATTR_SSCI]);
> +               nla_memcpy(rx_sa->key.salt.bytes, tb_sa[MACSEC_SA_ATTR_SALT],
> +                          MACSEC_SALT_LEN);
> +       }
> +
> +       nla_memcpy(rx_sa->key.id, tb_sa[MACSEC_SA_ATTR_KEYID], MACSEC_KEYID_LEN);

Is the key id part related to the XPN offloading not working?

Otherwise, it makes sense to copy all attributes before offloading the
operation but this should probably be in its own patch targeted at
net-next. (Same for the txsa part).

>         /* If h/w offloading is available, propagate to the device */
>         if (macsec_is_offloaded(netdev_priv(dev))) {
>                 const struct macsec_ops *ops;
> @@ -1826,13 +1834,6 @@ static int macsec_add_rxsa(struct sk_buff *skb, struct genl_info *info)
>                         goto cleanup;
>         }
>  
> -       if (secy->xpn) {
> -               rx_sa->ssci = nla_get_ssci(tb_sa[MACSEC_SA_ATTR_SSCI]);
> -               nla_memcpy(rx_sa->key.salt.bytes, tb_sa[MACSEC_SA_ATTR_SALT],
> -                          MACSEC_SALT_LEN);
> -       }
> -
> -       nla_memcpy(rx_sa->key.id, tb_sa[MACSEC_SA_ATTR_KEYID], MACSEC_KEYID_LEN);
>         rcu_assign_pointer(rx_sc->sa[assoc_num], rx_sa);
>  
>         rtnl_unlock();

Thanks!
Antoine