2022-05-05 18:37:35

by Carlos Fernandez

Subject: [PATCH] net: macsec: XPN Salt copied before passing offload context

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.

Fixes: 48ef50fa866a ("macsec: Netlink support of XPN cipher suites")
Signed-off-by: Carlos Fernandez <[email protected]>
---
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);
+
/* 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();
@@ -2046,6 +2047,14 @@ static int macsec_add_txsa(struct sk_buff *skb, struct genl_info *info)
if (assoc_num == tx_sc->encoding_sa && tx_sa->active)
secy->operational = true;

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

- if (secy->xpn) {
- tx_sa->ssci = nla_get_ssci(tb_sa[MACSEC_SA_ATTR_SSCI]);
- nla_memcpy(tx_sa->key.salt.bytes, tb_sa[MACSEC_SA_ATTR_SALT],
- MACSEC_SALT_LEN);
- }
-
- nla_memcpy(tx_sa->key.id, tb_sa[MACSEC_SA_ATTR_KEYID], MACSEC_KEYID_LEN);
rcu_assign_pointer(tx_sc->sa[assoc_num], tx_sa);

rtnl_unlock();
--
2.25.1

2022-05-09 07:31:45

by Jakub Kicinski

Subject: Re: [PATCH] net: macsec: XPN Salt copied before passing offload context

On Thu, 05 May 2022 15:28:05 +0200 Antoine Tenart wrote:
> (Note: please use "[PATCH net]" for fixes and "[PATCH net-next]" for
> improvements in the subject when submitting patches to the networking
> subsystem).

Plus the version of the patch, FWIW, so [PATCH net v2] would have been
appropriate here, I think.

2022-05-09 09:02:28

by Antoine Tenart

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 <[email protected]>
> ---
> 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