netdev.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net 0/4] macsec: fix config issues
@ 2022-07-22  9:16 Sabrina Dubroca
  2022-07-22  9:16 ` [PATCH net 1/4] macsec: fix NULL deref in macsec_add_rxsa Sabrina Dubroca
                   ` (4 more replies)
  0 siblings, 5 replies; 6+ messages in thread
From: Sabrina Dubroca @ 2022-07-22  9:16 UTC (permalink / raw)
  To: netdev; +Cc: Era Mayflower, Sabrina Dubroca

The patch adding netlink support for XPN (commit 48ef50fa866a
("macsec: Netlink support of XPN cipher suites (IEEE 802.1AEbw)"))
introduced several issues, including a kernel panic reported at [1].

Reproducing those bugs with upstream iproute is limited, since iproute
doesn't currently support XPN. I'm also working on this.

[1] https://bugzilla.kernel.org/show_bug.cgi?id=208315

Sabrina Dubroca (4):
  macsec: fix NULL deref in macsec_add_rxsa
  macsec: fix error message in macsec_add_rxsa and _txsa
  macsec: limit replay window size with XPN
  macsec: always read MACSEC_SA_ATTR_PN as a u64

 drivers/net/macsec.c | 29 +++++++++++++++++++----------
 1 file changed, 19 insertions(+), 10 deletions(-)

-- 
2.36.1


^ permalink raw reply	[flat|nested] 6+ messages in thread
* [PATCH net 1/4] macsec: fix NULL deref in macsec_add_rxsa
  2022-07-22  9:16 [PATCH net 0/4] macsec: fix config issues Sabrina Dubroca
@ 2022-07-22  9:16 ` Sabrina Dubroca
  2022-07-22  9:16 ` [PATCH net 2/4] macsec: fix error message in macsec_add_rxsa and _txsa Sabrina Dubroca
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 6+ messages in thread
From: Sabrina Dubroca @ 2022-07-22  9:16 UTC (permalink / raw)
  To: netdev; +Cc: Era Mayflower, Sabrina Dubroca, Frantisek Sumsal

Commit 48ef50fa866a added a test on tb_sa[MACSEC_SA_ATTR_PN], but
nothing guarantees that it's not NULL at this point. The same code was
added to macsec_add_txsa, but there it's not a problem because
validate_add_txsa checks that the MACSEC_SA_ATTR_PN attribute is
present.

Note: it's not possible to reproduce with iproute, because iproute
doesn't allow creating an SA without specifying the PN.

Fixes: 48ef50fa866a ("macsec: Netlink support of XPN cipher suites (IEEE 802.1AEbw)")
Link: https://bugzilla.kernel.org/show_bug.cgi?id=208315
Reported-by: Frantisek Sumsal <fsumsal@redhat.com>
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 drivers/net/macsec.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index 817577e713d7..769a1eca6bd8 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -1753,7 +1753,8 @@ static int macsec_add_rxsa(struct sk_buff *skb, struct genl_info *info)
 	}
 
 	pn_len = secy->xpn ? MACSEC_XPN_PN_LEN : MACSEC_DEFAULT_PN_LEN;
-	if (nla_len(tb_sa[MACSEC_SA_ATTR_PN]) != pn_len) {
+	if (tb_sa[MACSEC_SA_ATTR_PN] &&
+	    nla_len(tb_sa[MACSEC_SA_ATTR_PN]) != pn_len) {
 		pr_notice("macsec: nl: add_rxsa: bad pn length: %d != %d\n",
 			  nla_len(tb_sa[MACSEC_SA_ATTR_PN]), pn_len);
 		rtnl_unlock();
-- 
2.36.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH net 2/4] macsec: fix error message in macsec_add_rxsa and _txsa
  2022-07-22  9:16 [PATCH net 0/4] macsec: fix config issues Sabrina Dubroca
  2022-07-22  9:16 ` [PATCH net 1/4] macsec: fix NULL deref in macsec_add_rxsa Sabrina Dubroca
@ 2022-07-22  9:16 ` Sabrina Dubroca
  2022-07-22  9:16 ` [PATCH net 3/4] macsec: limit replay window size with XPN Sabrina Dubroca
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 6+ messages in thread
From: Sabrina Dubroca @ 2022-07-22  9:16 UTC (permalink / raw)
  To: netdev; +Cc: Era Mayflower, Sabrina Dubroca

The expected length is MACSEC_SALT_LEN, not MACSEC_SA_ATTR_SALT.

Fixes: 48ef50fa866a ("macsec: Netlink support of XPN cipher suites (IEEE 802.1AEbw)")
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 drivers/net/macsec.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index 769a1eca6bd8..634452d3ecc5 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -1770,7 +1770,7 @@ static int macsec_add_rxsa(struct sk_buff *skb, struct genl_info *info)
 		if (nla_len(tb_sa[MACSEC_SA_ATTR_SALT]) != MACSEC_SALT_LEN) {
 			pr_notice("macsec: nl: add_rxsa: bad salt length: %d != %d\n",
 				  nla_len(tb_sa[MACSEC_SA_ATTR_SALT]),
-				  MACSEC_SA_ATTR_SALT);
+				  MACSEC_SALT_LEN);
 			rtnl_unlock();
 			return -EINVAL;
 		}
@@ -2012,7 +2012,7 @@ static int macsec_add_txsa(struct sk_buff *skb, struct genl_info *info)
 		if (nla_len(tb_sa[MACSEC_SA_ATTR_SALT]) != MACSEC_SALT_LEN) {
 			pr_notice("macsec: nl: add_txsa: bad salt length: %d != %d\n",
 				  nla_len(tb_sa[MACSEC_SA_ATTR_SALT]),
-				  MACSEC_SA_ATTR_SALT);
+				  MACSEC_SALT_LEN);
 			rtnl_unlock();
 			return -EINVAL;
 		}
-- 
2.36.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH net 3/4] macsec: limit replay window size with XPN
  2022-07-22  9:16 [PATCH net 0/4] macsec: fix config issues Sabrina Dubroca
  2022-07-22  9:16 ` [PATCH net 1/4] macsec: fix NULL deref in macsec_add_rxsa Sabrina Dubroca
  2022-07-22  9:16 ` [PATCH net 2/4] macsec: fix error message in macsec_add_rxsa and _txsa Sabrina Dubroca
@ 2022-07-22  9:16 ` Sabrina Dubroca
  2022-07-22  9:16 ` [PATCH net 4/4] macsec: always read MACSEC_SA_ATTR_PN as a u64 Sabrina Dubroca
  2022-07-25 11:00 ` [PATCH net 0/4] macsec: fix config issues patchwork-bot+netdevbpf
  4 siblings, 0 replies; 6+ messages in thread
From: Sabrina Dubroca @ 2022-07-22  9:16 UTC (permalink / raw)
  To: netdev; +Cc: Era Mayflower, Sabrina Dubroca

IEEE 802.1AEbw-2013 (section 10.7.8) specifies that the maximum value
of the replay window is 2^30-1, to help with recovery of the upper
bits of the PN.

To avoid leaving the existing macsec device in an inconsistent state
if this test fails during changelink, reuse the cleanup mechanism
introduced for HW offload. This wasn't needed until now because
macsec_changelink_common could not fail during changelink, as
modifying the cipher suite was not allowed.

Finally, this must happen after handling IFLA_MACSEC_CIPHER_SUITE so
that secy->xpn is set.

Fixes: 48ef50fa866a ("macsec: Netlink support of XPN cipher suites (IEEE 802.1AEbw)")
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 drivers/net/macsec.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index 634452d3ecc5..b3834e353c22 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -243,6 +243,7 @@ static struct macsec_cb *macsec_skb_cb(struct sk_buff *skb)
 #define DEFAULT_SEND_SCI true
 #define DEFAULT_ENCRYPT false
 #define DEFAULT_ENCODING_SA 0
+#define MACSEC_XPN_MAX_REPLAY_WINDOW (((1 << 30) - 1))
 
 static bool send_sci(const struct macsec_secy *secy)
 {
@@ -3746,9 +3747,6 @@ static int macsec_changelink_common(struct net_device *dev,
 		secy->operational = tx_sa && tx_sa->active;
 	}
 
-	if (data[IFLA_MACSEC_WINDOW])
-		secy->replay_window = nla_get_u32(data[IFLA_MACSEC_WINDOW]);
-
 	if (data[IFLA_MACSEC_ENCRYPT])
 		tx_sc->encrypt = !!nla_get_u8(data[IFLA_MACSEC_ENCRYPT]);
 
@@ -3794,6 +3792,16 @@ static int macsec_changelink_common(struct net_device *dev,
 		}
 	}
 
+	if (data[IFLA_MACSEC_WINDOW]) {
+		secy->replay_window = nla_get_u32(data[IFLA_MACSEC_WINDOW]);
+
+		/* IEEE 802.1AEbw-2013 10.7.8 - maximum replay window
+		 * for XPN cipher suites */
+		if (secy->xpn &&
+		    secy->replay_window > MACSEC_XPN_MAX_REPLAY_WINDOW)
+			return -EINVAL;
+	}
+
 	return 0;
 }
 
@@ -3823,7 +3831,7 @@ static int macsec_changelink(struct net_device *dev, struct nlattr *tb[],
 
 	ret = macsec_changelink_common(dev, data);
 	if (ret)
-		return ret;
+		goto cleanup;
 
 	/* If h/w offloading is available, propagate to the device */
 	if (macsec_is_offloaded(macsec)) {
-- 
2.36.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* [PATCH net 4/4] macsec: always read MACSEC_SA_ATTR_PN as a u64
  2022-07-22  9:16 [PATCH net 0/4] macsec: fix config issues Sabrina Dubroca
                   ` (2 preceding siblings ...)
  2022-07-22  9:16 ` [PATCH net 3/4] macsec: limit replay window size with XPN Sabrina Dubroca
@ 2022-07-22  9:16 ` Sabrina Dubroca
  2022-07-25 11:00 ` [PATCH net 0/4] macsec: fix config issues patchwork-bot+netdevbpf
  4 siblings, 0 replies; 6+ messages in thread
From: Sabrina Dubroca @ 2022-07-22  9:16 UTC (permalink / raw)
  To: netdev; +Cc: Era Mayflower, Sabrina Dubroca

Currently, MACSEC_SA_ATTR_PN is handled inconsistently, sometimes as a
u32, sometimes forced into a u64 without checking the actual length of
the attribute. Instead, we can use nla_get_u64 everywhere, which will
read up to 64 bits into a u64, capped by the actual length of the
attribute coming from userspace.

This fixes several issues:
 - the check in validate_add_rxsa doesn't work with 32-bit attributes
 - the checks in validate_add_txsa and validate_upd_sa incorrectly
   reject X << 32 (with X != 0)

Fixes: 48ef50fa866a ("macsec: Netlink support of XPN cipher suites (IEEE 802.1AEbw)")
Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 drivers/net/macsec.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index b3834e353c22..95578f04f212 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -1698,7 +1698,7 @@ static bool validate_add_rxsa(struct nlattr **attrs)
 		return false;
 
 	if (attrs[MACSEC_SA_ATTR_PN] &&
-	    *(u64 *)nla_data(attrs[MACSEC_SA_ATTR_PN]) == 0)
+	    nla_get_u64(attrs[MACSEC_SA_ATTR_PN]) == 0)
 		return false;
 
 	if (attrs[MACSEC_SA_ATTR_ACTIVE]) {
@@ -1941,7 +1941,7 @@ static bool validate_add_txsa(struct nlattr **attrs)
 	if (nla_get_u8(attrs[MACSEC_SA_ATTR_AN]) >= MACSEC_NUM_AN)
 		return false;
 
-	if (nla_get_u32(attrs[MACSEC_SA_ATTR_PN]) == 0)
+	if (nla_get_u64(attrs[MACSEC_SA_ATTR_PN]) == 0)
 		return false;
 
 	if (attrs[MACSEC_SA_ATTR_ACTIVE]) {
@@ -2295,7 +2295,7 @@ static bool validate_upd_sa(struct nlattr **attrs)
 	if (nla_get_u8(attrs[MACSEC_SA_ATTR_AN]) >= MACSEC_NUM_AN)
 		return false;
 
-	if (attrs[MACSEC_SA_ATTR_PN] && nla_get_u32(attrs[MACSEC_SA_ATTR_PN]) == 0)
+	if (attrs[MACSEC_SA_ATTR_PN] && nla_get_u64(attrs[MACSEC_SA_ATTR_PN]) == 0)
 		return false;
 
 	if (attrs[MACSEC_SA_ATTR_ACTIVE]) {
-- 
2.36.1


^ permalink raw reply related	[flat|nested] 6+ messages in thread
* Re: [PATCH net 0/4] macsec: fix config issues
  2022-07-22  9:16 [PATCH net 0/4] macsec: fix config issues Sabrina Dubroca
                   ` (3 preceding siblings ...)
  2022-07-22  9:16 ` [PATCH net 4/4] macsec: always read MACSEC_SA_ATTR_PN as a u64 Sabrina Dubroca
@ 2022-07-25 11:00 ` patchwork-bot+netdevbpf
  4 siblings, 0 replies; 6+ messages in thread
From: patchwork-bot+netdevbpf @ 2022-07-25 11:00 UTC (permalink / raw)
  To: Sabrina Dubroca; +Cc: netdev, mayflowerera

Hello:

This series was applied to netdev/net.git (master)
by David S. Miller <davem@davemloft.net>:

On Fri, 22 Jul 2022 11:16:26 +0200 you wrote:
> The patch adding netlink support for XPN (commit 48ef50fa866a
> ("macsec: Netlink support of XPN cipher suites (IEEE 802.1AEbw)"))
> introduced several issues, including a kernel panic reported at [1].
> 
> Reproducing those bugs with upstream iproute is limited, since iproute
> doesn't currently support XPN. I'm also working on this.
> 
> [...]

Here is the summary with links:
  - [net,1/4] macsec: fix NULL deref in macsec_add_rxsa
    https://git.kernel.org/netdev/net/c/f46040eeaf2e
  - [net,2/4] macsec: fix error message in macsec_add_rxsa and _txsa
    https://git.kernel.org/netdev/net/c/3240eac4ff20
  - [net,3/4] macsec: limit replay window size with XPN
    https://git.kernel.org/netdev/net/c/b07a0e204405
  - [net,4/4] macsec: always read MACSEC_SA_ATTR_PN as a u64
    https://git.kernel.org/netdev/net/c/c630d1fe6219

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 6+ messages in thread
end of thread, other threads:[~2022-07-25 11:00 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-22  9:16 [PATCH net 0/4] macsec: fix config issues Sabrina Dubroca
2022-07-22  9:16 ` [PATCH net 1/4] macsec: fix NULL deref in macsec_add_rxsa Sabrina Dubroca
2022-07-22  9:16 ` [PATCH net 2/4] macsec: fix error message in macsec_add_rxsa and _txsa Sabrina Dubroca
2022-07-22  9:16 ` [PATCH net 3/4] macsec: limit replay window size with XPN Sabrina Dubroca
2022-07-22  9:16 ` [PATCH net 4/4] macsec: always read MACSEC_SA_ATTR_PN as a u64 Sabrina Dubroca
2022-07-25 11:00 ` [PATCH net 0/4] macsec: fix config issues patchwork-bot+netdevbpf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).