[PATCH net-next v2 00/13] macsec: replace custom netlink attribute checks with policy-level checks

[PATCH net-next v2 00/13] macsec: replace custom netlink attribute checks with policy-level checks

[Sabrina Dubroca]

We can simplify attribute validation a lot by describing the accepted
ranges more precisely in the policies, using NLA_POLICY_MAX etc.

Some of the checks still need to be done later on, because the
attribute length and acceptable range can vary based on values that
can't be known when the policy is validated (cipher suite determines
the key length and valid ICV length, presence of XPN changes the PN
length, detection of duplicate SCIs or ANs, etc).

As a bonus, we get a few extack messages from the policy
validation. I'll add extack to the rest of the checks (mostly in the
genl commands) in an future series.

v2: remove no-longer-unused csid variable (thanks Jakub)
    rebase on top of net-next
    use NLA_UINT for PN
v1: https://lore.kernel.org/netdev/cover.1664379352.git.sd@queasysnail.net/

Sabrina Dubroca (13):
  macsec: replace custom checks on MACSEC_SA_ATTR_AN with NLA_POLICY_MAX
  macsec: replace custom checks on MACSEC_*_ATTR_ACTIVE with
    NLA_POLICY_MAX
  macsec: replace custom checks on MACSEC_SA_ATTR_SALT with
    NLA_POLICY_EXACT_LEN
  macsec: replace custom checks on MACSEC_SA_ATTR_KEYID with
    NLA_POLICY_EXACT_LEN
  macsec: use NLA_POLICY_MAX_LEN for MACSEC_SA_ATTR_KEY
  macsec: use NLA_UINT for MACSEC_SA_ATTR_PN
  macsec: remove validate_add_rxsc
  macsec: add NLA_POLICY_MAX for MACSEC_OFFLOAD_ATTR_TYPE and
    IFLA_MACSEC_OFFLOAD
  macsec: replace custom checks on IFLA_MACSEC_ICV_LEN with
    NLA_POLICY_RANGE
  macsec: use NLA_POLICY_VALIDATE_FN to validate
    IFLA_MACSEC_CIPHER_SUITE
  macsec: validate IFLA_MACSEC_VALIDATION with NLA_POLICY_MAX
  macsec: replace custom checks for IFLA_MACSEC_* flags with
    NLA_POLICY_MAX
  macsec: replace custom check on IFLA_MACSEC_ENCODING_SA with
    NLA_POLICY_MAX

 drivers/net/macsec.c | 173 ++++++++++---------------------------------
 1 file changed, 38 insertions(+), 135 deletions(-)

-- 
2.50.0

[PATCH net-next v2 01/13] macsec: replace custom checks on MACSEC_SA_ATTR_AN with NLA_POLICY_MAX

[Sabrina Dubroca]

Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 drivers/net/macsec.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index 4c75d1fea552..96ca1b00438f 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -1583,9 +1583,6 @@ static struct macsec_tx_sa *get_txsa_from_nl(struct net *net,
 	if (IS_ERR(dev))
 		return ERR_CAST(dev);
 
-	if (*assoc_num >= MACSEC_NUM_AN)
-		return ERR_PTR(-EINVAL);
-
 	secy = &macsec_priv(dev)->secy;
 	tx_sc = &secy->tx_sc;
 
@@ -1646,8 +1643,6 @@ static struct macsec_rx_sa *get_rxsa_from_nl(struct net *net,
 		return ERR_PTR(-EINVAL);
 
 	*assoc_num = nla_get_u8(tb_sa[MACSEC_SA_ATTR_AN]);
-	if (*assoc_num >= MACSEC_NUM_AN)
-		return ERR_PTR(-EINVAL);
 
 	rx_sc = get_rxsc_from_nl(net, attrs, tb_rxsc, devp, secyp);
 	if (IS_ERR(rx_sc))
@@ -1674,7 +1669,7 @@ static const struct nla_policy macsec_genl_rxsc_policy[NUM_MACSEC_RXSC_ATTR] = {
 };
 
 static const struct nla_policy macsec_genl_sa_policy[NUM_MACSEC_SA_ATTR] = {
-	[MACSEC_SA_ATTR_AN] = { .type = NLA_U8 },
+	[MACSEC_SA_ATTR_AN] = NLA_POLICY_MAX(NLA_U8, MACSEC_NUM_AN - 1),
 	[MACSEC_SA_ATTR_ACTIVE] = { .type = NLA_U8 },
 	[MACSEC_SA_ATTR_PN] = NLA_POLICY_MIN_LEN(4),
 	[MACSEC_SA_ATTR_KEYID] = { .type = NLA_BINARY,
@@ -1739,8 +1734,6 @@ static bool validate_add_rxsa(struct nlattr **attrs)
 	    !attrs[MACSEC_SA_ATTR_KEYID])
 		return false;
 
-	if (nla_get_u8(attrs[MACSEC_SA_ATTR_AN]) >= MACSEC_NUM_AN)
-		return false;
 
 	if (attrs[MACSEC_SA_ATTR_PN] &&
 	    nla_get_u64(attrs[MACSEC_SA_ATTR_PN]) == 0)
@@ -1984,9 +1977,6 @@ static bool validate_add_txsa(struct nlattr **attrs)
 	    !attrs[MACSEC_SA_ATTR_KEYID])
 		return false;
 
-	if (nla_get_u8(attrs[MACSEC_SA_ATTR_AN]) >= MACSEC_NUM_AN)
-		return false;
-
 	if (nla_get_u64(attrs[MACSEC_SA_ATTR_PN]) == 0)
 		return false;
 
@@ -2339,9 +2329,6 @@ static bool validate_upd_sa(struct nlattr **attrs)
 	    attrs[MACSEC_SA_ATTR_SALT])
 		return false;
 
-	if (nla_get_u8(attrs[MACSEC_SA_ATTR_AN]) >= MACSEC_NUM_AN)
-		return false;
-
 	if (attrs[MACSEC_SA_ATTR_PN] && nla_get_u64(attrs[MACSEC_SA_ATTR_PN]) == 0)
 		return false;
 
-- 
2.50.0

[PATCH net-next v2 02/13] macsec: replace custom checks on MACSEC_*_ATTR_ACTIVE with NLA_POLICY_MAX

[Sabrina Dubroca]

Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 drivers/net/macsec.c | 25 ++-----------------------
 1 file changed, 2 insertions(+), 23 deletions(-)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index 96ca1b00438f..7386335cc038 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -1665,12 +1665,12 @@ static const struct nla_policy macsec_genl_policy[NUM_MACSEC_ATTR] = {
 
 static const struct nla_policy macsec_genl_rxsc_policy[NUM_MACSEC_RXSC_ATTR] = {
 	[MACSEC_RXSC_ATTR_SCI] = { .type = NLA_U64 },
-	[MACSEC_RXSC_ATTR_ACTIVE] = { .type = NLA_U8 },
+	[MACSEC_RXSC_ATTR_ACTIVE] = NLA_POLICY_MAX(NLA_U8, 1),
 };
 
 static const struct nla_policy macsec_genl_sa_policy[NUM_MACSEC_SA_ATTR] = {
 	[MACSEC_SA_ATTR_AN] = NLA_POLICY_MAX(NLA_U8, MACSEC_NUM_AN - 1),
-	[MACSEC_SA_ATTR_ACTIVE] = { .type = NLA_U8 },
+	[MACSEC_SA_ATTR_ACTIVE] = NLA_POLICY_MAX(NLA_U8, 1),
 	[MACSEC_SA_ATTR_PN] = NLA_POLICY_MIN_LEN(4),
 	[MACSEC_SA_ATTR_KEYID] = { .type = NLA_BINARY,
 				   .len = MACSEC_KEYID_LEN, },
@@ -1734,16 +1734,10 @@ static bool validate_add_rxsa(struct nlattr **attrs)
 	    !attrs[MACSEC_SA_ATTR_KEYID])
 		return false;
 
-
 	if (attrs[MACSEC_SA_ATTR_PN] &&
 	    nla_get_u64(attrs[MACSEC_SA_ATTR_PN]) == 0)
 		return false;
 
-	if (attrs[MACSEC_SA_ATTR_ACTIVE]) {
-		if (nla_get_u8(attrs[MACSEC_SA_ATTR_ACTIVE]) > 1)
-			return false;
-	}
-
 	if (nla_len(attrs[MACSEC_SA_ATTR_KEYID]) != MACSEC_KEYID_LEN)
 		return false;
 
@@ -1893,11 +1887,6 @@ static bool validate_add_rxsc(struct nlattr **attrs)
 	if (!attrs[MACSEC_RXSC_ATTR_SCI])
 		return false;
 
-	if (attrs[MACSEC_RXSC_ATTR_ACTIVE]) {
-		if (nla_get_u8(attrs[MACSEC_RXSC_ATTR_ACTIVE]) > 1)
-			return false;
-	}
-
 	return true;
 }
 
@@ -1980,11 +1969,6 @@ static bool validate_add_txsa(struct nlattr **attrs)
 	if (nla_get_u64(attrs[MACSEC_SA_ATTR_PN]) == 0)
 		return false;
 
-	if (attrs[MACSEC_SA_ATTR_ACTIVE]) {
-		if (nla_get_u8(attrs[MACSEC_SA_ATTR_ACTIVE]) > 1)
-			return false;
-	}
-
 	if (nla_len(attrs[MACSEC_SA_ATTR_KEYID]) != MACSEC_KEYID_LEN)
 		return false;
 
@@ -2332,11 +2316,6 @@ static bool validate_upd_sa(struct nlattr **attrs)
 	if (attrs[MACSEC_SA_ATTR_PN] && nla_get_u64(attrs[MACSEC_SA_ATTR_PN]) == 0)
 		return false;
 
-	if (attrs[MACSEC_SA_ATTR_ACTIVE]) {
-		if (nla_get_u8(attrs[MACSEC_SA_ATTR_ACTIVE]) > 1)
-			return false;
-	}
-
 	return true;
 }
 
-- 
2.50.0

[PATCH net-next v2 03/13] macsec: replace custom checks on MACSEC_SA_ATTR_SALT with NLA_POLICY_EXACT_LEN

[Sabrina Dubroca]

The existing checks already specify that MACSEC_SA_ATTR_SALT must have
length MACSEC_SALT_LEN.

Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 drivers/net/macsec.c | 19 +------------------
 1 file changed, 1 insertion(+), 18 deletions(-)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index 7386335cc038..4bff2c90ff49 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -1677,8 +1677,7 @@ static const struct nla_policy macsec_genl_sa_policy[NUM_MACSEC_SA_ATTR] = {
 	[MACSEC_SA_ATTR_KEY] = { .type = NLA_BINARY,
 				 .len = MACSEC_MAX_KEY_LEN, },
 	[MACSEC_SA_ATTR_SSCI] = { .type = NLA_U32 },
-	[MACSEC_SA_ATTR_SALT] = { .type = NLA_BINARY,
-				  .len = MACSEC_SALT_LEN, },
+	[MACSEC_SA_ATTR_SALT] = NLA_POLICY_EXACT_LEN(MACSEC_SALT_LEN),
 };
 
 static const struct nla_policy macsec_genl_offload_policy[NUM_MACSEC_OFFLOAD_ATTR] = {
@@ -1799,14 +1798,6 @@ static int macsec_add_rxsa(struct sk_buff *skb, struct genl_info *info)
 			rtnl_unlock();
 			return -EINVAL;
 		}
-
-		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_SALT_LEN);
-			rtnl_unlock();
-			return -EINVAL;
-		}
 	}
 
 	rx_sa = rtnl_dereference(rx_sc->sa[assoc_num]);
@@ -2029,14 +2020,6 @@ static int macsec_add_txsa(struct sk_buff *skb, struct genl_info *info)
 			rtnl_unlock();
 			return -EINVAL;
 		}
-
-		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_SALT_LEN);
-			rtnl_unlock();
-			return -EINVAL;
-		}
 	}
 
 	tx_sa = rtnl_dereference(tx_sc->sa[assoc_num]);
-- 
2.50.0

[PATCH net-next v2 04/13] macsec: replace custom checks on MACSEC_SA_ATTR_KEYID with NLA_POLICY_EXACT_LEN

[Sabrina Dubroca]

The existing checks already specify that MACSEC_SA_ATTR_KEYID must
have length MACSEC_KEYID_LEN.

Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 drivers/net/macsec.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index 4bff2c90ff49..1b59f2c6b393 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -1672,8 +1672,7 @@ static const struct nla_policy macsec_genl_sa_policy[NUM_MACSEC_SA_ATTR] = {
 	[MACSEC_SA_ATTR_AN] = NLA_POLICY_MAX(NLA_U8, MACSEC_NUM_AN - 1),
 	[MACSEC_SA_ATTR_ACTIVE] = NLA_POLICY_MAX(NLA_U8, 1),
 	[MACSEC_SA_ATTR_PN] = NLA_POLICY_MIN_LEN(4),
-	[MACSEC_SA_ATTR_KEYID] = { .type = NLA_BINARY,
-				   .len = MACSEC_KEYID_LEN, },
+	[MACSEC_SA_ATTR_KEYID] = NLA_POLICY_EXACT_LEN(MACSEC_KEYID_LEN),
 	[MACSEC_SA_ATTR_KEY] = { .type = NLA_BINARY,
 				 .len = MACSEC_MAX_KEY_LEN, },
 	[MACSEC_SA_ATTR_SSCI] = { .type = NLA_U32 },
@@ -1737,9 +1736,6 @@ static bool validate_add_rxsa(struct nlattr **attrs)
 	    nla_get_u64(attrs[MACSEC_SA_ATTR_PN]) == 0)
 		return false;
 
-	if (nla_len(attrs[MACSEC_SA_ATTR_KEYID]) != MACSEC_KEYID_LEN)
-		return false;
-
 	return true;
 }
 
@@ -1960,9 +1956,6 @@ static bool validate_add_txsa(struct nlattr **attrs)
 	if (nla_get_u64(attrs[MACSEC_SA_ATTR_PN]) == 0)
 		return false;
 
-	if (nla_len(attrs[MACSEC_SA_ATTR_KEYID]) != MACSEC_KEYID_LEN)
-		return false;
-
 	return true;
 }
 
-- 
2.50.0

[PATCH net-next v2 05/13] macsec: use NLA_POLICY_MAX_LEN for MACSEC_SA_ATTR_KEY

[Sabrina Dubroca]

Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 drivers/net/macsec.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index 1b59f2c6b393..b613ce1e3a7e 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -1673,8 +1673,7 @@ static const struct nla_policy macsec_genl_sa_policy[NUM_MACSEC_SA_ATTR] = {
 	[MACSEC_SA_ATTR_ACTIVE] = NLA_POLICY_MAX(NLA_U8, 1),
 	[MACSEC_SA_ATTR_PN] = NLA_POLICY_MIN_LEN(4),
 	[MACSEC_SA_ATTR_KEYID] = NLA_POLICY_EXACT_LEN(MACSEC_KEYID_LEN),
-	[MACSEC_SA_ATTR_KEY] = { .type = NLA_BINARY,
-				 .len = MACSEC_MAX_KEY_LEN, },
+	[MACSEC_SA_ATTR_KEY] = NLA_POLICY_MAX_LEN(MACSEC_MAX_KEY_LEN),
 	[MACSEC_SA_ATTR_SSCI] = { .type = NLA_U32 },
 	[MACSEC_SA_ATTR_SALT] = NLA_POLICY_EXACT_LEN(MACSEC_SALT_LEN),
 };
-- 
2.50.0

[PATCH net-next v2 06/13] macsec: use NLA_UINT for MACSEC_SA_ATTR_PN

[Sabrina Dubroca]

MACSEC_SA_ATTR_PN is either a u32 or a u64, we can now use NLA_UINT
for this instead of a custom binary type. We can then use a min check
within the policy.

We need to keep the length checks done in macsec_{add,upd}_{rx,tx}sa
based on whether the device is set up for XPN (with 64b PNs instead of
32b).

On the dump side, keep the existing custom code as userspace may
expect a u64 when using XPN, and nla_put_uint may only output a u32
attribute if the value fits.

Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 drivers/net/macsec.c | 12 +-----------
 1 file changed, 1 insertion(+), 11 deletions(-)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index b613ce1e3a7e..83158dbc1628 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -1671,7 +1671,7 @@ static const struct nla_policy macsec_genl_rxsc_policy[NUM_MACSEC_RXSC_ATTR] = {
 static const struct nla_policy macsec_genl_sa_policy[NUM_MACSEC_SA_ATTR] = {
 	[MACSEC_SA_ATTR_AN] = NLA_POLICY_MAX(NLA_U8, MACSEC_NUM_AN - 1),
 	[MACSEC_SA_ATTR_ACTIVE] = NLA_POLICY_MAX(NLA_U8, 1),
-	[MACSEC_SA_ATTR_PN] = NLA_POLICY_MIN_LEN(4),
+	[MACSEC_SA_ATTR_PN] = NLA_POLICY_MIN(NLA_UINT, 1),
 	[MACSEC_SA_ATTR_KEYID] = NLA_POLICY_EXACT_LEN(MACSEC_KEYID_LEN),
 	[MACSEC_SA_ATTR_KEY] = NLA_POLICY_MAX_LEN(MACSEC_MAX_KEY_LEN),
 	[MACSEC_SA_ATTR_SSCI] = { .type = NLA_U32 },
@@ -1731,10 +1731,6 @@ static bool validate_add_rxsa(struct nlattr **attrs)
 	    !attrs[MACSEC_SA_ATTR_KEYID])
 		return false;
 
-	if (attrs[MACSEC_SA_ATTR_PN] &&
-	    nla_get_u64(attrs[MACSEC_SA_ATTR_PN]) == 0)
-		return false;
-
 	return true;
 }
 
@@ -1952,9 +1948,6 @@ static bool validate_add_txsa(struct nlattr **attrs)
 	    !attrs[MACSEC_SA_ATTR_KEYID])
 		return false;
 
-	if (nla_get_u64(attrs[MACSEC_SA_ATTR_PN]) == 0)
-		return false;
-
 	return true;
 }
 
@@ -2288,9 +2281,6 @@ static bool validate_upd_sa(struct nlattr **attrs)
 	    attrs[MACSEC_SA_ATTR_SALT])
 		return false;
 
-	if (attrs[MACSEC_SA_ATTR_PN] && nla_get_u64(attrs[MACSEC_SA_ATTR_PN]) == 0)
-		return false;
-
 	return true;
 }
 
-- 
2.50.0

[PATCH net-next v2 07/13] macsec: remove validate_add_rxsc

[Sabrina Dubroca]

It's not doing much anymore.

Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 drivers/net/macsec.c | 12 ++----------
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index 83158dbc1628..ca47c9a95cf4 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -1864,14 +1864,6 @@ static int macsec_add_rxsa(struct sk_buff *skb, struct genl_info *info)
 	return err;
 }
 
-static bool validate_add_rxsc(struct nlattr **attrs)
-{
-	if (!attrs[MACSEC_RXSC_ATTR_SCI])
-		return false;
-
-	return true;
-}
-
 static int macsec_add_rxsc(struct sk_buff *skb, struct genl_info *info)
 {
 	struct net_device *dev;
@@ -1889,7 +1881,7 @@ static int macsec_add_rxsc(struct sk_buff *skb, struct genl_info *info)
 	if (parse_rxsc_config(attrs, tb_rxsc))
 		return -EINVAL;
 
-	if (!validate_add_rxsc(tb_rxsc))
+	if (!tb_rxsc[MACSEC_RXSC_ATTR_SCI])
 		return -EINVAL;
 
 	rtnl_lock();
@@ -2487,7 +2479,7 @@ static int macsec_upd_rxsc(struct sk_buff *skb, struct genl_info *info)
 	if (parse_rxsc_config(attrs, tb_rxsc))
 		return -EINVAL;
 
-	if (!validate_add_rxsc(tb_rxsc))
+	if (!tb_rxsc[MACSEC_RXSC_ATTR_SCI])
 		return -EINVAL;
 
 	rtnl_lock();
-- 
2.50.0

[PATCH net-next v2 08/13] macsec: add NLA_POLICY_MAX for MACSEC_OFFLOAD_ATTR_TYPE and IFLA_MACSEC_OFFLOAD

[Sabrina Dubroca]

This is equivalent to the existing checks allowing either
MACSEC_OFFLOAD_OFF or calling macsec_check_offload.

Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 drivers/net/macsec.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

v2: rebase on net-next (IFLA_MACSEC_OFFLOAD policy was added)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index ca47c9a95cf4..463fd9650b31 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -1679,7 +1679,7 @@ static const struct nla_policy macsec_genl_sa_policy[NUM_MACSEC_SA_ATTR] = {
 };
 
 static const struct nla_policy macsec_genl_offload_policy[NUM_MACSEC_OFFLOAD_ATTR] = {
-	[MACSEC_OFFLOAD_ATTR_TYPE] = { .type = NLA_U8 },
+	[MACSEC_OFFLOAD_ATTR_TYPE] = NLA_POLICY_MAX(NLA_U8, MACSEC_OFFLOAD_MAX),
 };
 
 /* Offloads an operation to a device driver */
@@ -3771,7 +3771,7 @@ static const struct nla_policy macsec_rtnl_policy[IFLA_MACSEC_MAX + 1] = {
 	[IFLA_MACSEC_SCB] = { .type = NLA_U8 },
 	[IFLA_MACSEC_REPLAY_PROTECT] = { .type = NLA_U8 },
 	[IFLA_MACSEC_VALIDATION] = { .type = NLA_U8 },
-	[IFLA_MACSEC_OFFLOAD] = { .type = NLA_U8 },
+	[IFLA_MACSEC_OFFLOAD] = NLA_POLICY_MAX(NLA_U8, MACSEC_OFFLOAD_MAX),
 };
 
 static void macsec_free_netdev(struct net_device *dev)
-- 
2.50.0

[PATCH net-next v2 09/13] macsec: replace custom checks on IFLA_MACSEC_ICV_LEN with NLA_POLICY_RANGE

[Sabrina Dubroca]

The existing checks already force this range.

Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 drivers/net/macsec.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index 463fd9650b31..9589e8f9a8c9 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -3760,7 +3760,7 @@ static const struct device_type macsec_type = {
 static const struct nla_policy macsec_rtnl_policy[IFLA_MACSEC_MAX + 1] = {
 	[IFLA_MACSEC_SCI] = { .type = NLA_U64 },
 	[IFLA_MACSEC_PORT] = { .type = NLA_U16 },
-	[IFLA_MACSEC_ICV_LEN] = { .type = NLA_U8 },
+	[IFLA_MACSEC_ICV_LEN] = NLA_POLICY_RANGE(NLA_U8, MACSEC_MIN_ICV_LEN, MACSEC_STD_ICV_LEN),
 	[IFLA_MACSEC_CIPHER_SUITE] = { .type = NLA_U64 },
 	[IFLA_MACSEC_WINDOW] = { .type = NLA_U32 },
 	[IFLA_MACSEC_ENCODING_SA] = { .type = NLA_U8 },
@@ -4260,9 +4260,6 @@ static int macsec_validate_attr(struct nlattr *tb[], struct nlattr *data[],
 	case MACSEC_CIPHER_ID_GCM_AES_XPN_128:
 	case MACSEC_CIPHER_ID_GCM_AES_XPN_256:
 	case MACSEC_DEFAULT_CIPHER_ID:
-		if (icv_len < MACSEC_MIN_ICV_LEN ||
-		    icv_len > MACSEC_STD_ICV_LEN)
-			return -EINVAL;
 		break;
 	default:
 		return -EINVAL;
-- 
2.50.0

[PATCH net-next v2 10/13] macsec: use NLA_POLICY_VALIDATE_FN to validate IFLA_MACSEC_CIPHER_SUITE

[Sabrina Dubroca]

Unfortunately, since the value of MACSEC_DEFAULT_CIPHER_ID doesn't fit
near the others, we can't use a simple range in the policy.

Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 drivers/net/macsec.c | 34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

v2: remove unused csid variable (spotted by Jakub Kicinski and kernel
    test robot)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index 9589e8f9a8c9..5680e4b78dda 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -3757,11 +3757,13 @@ static const struct device_type macsec_type = {
 	.name = "macsec",
 };
 
+static int validate_cipher_suite(const struct nlattr *attr,
+				 struct netlink_ext_ack *extack);
 static const struct nla_policy macsec_rtnl_policy[IFLA_MACSEC_MAX + 1] = {
 	[IFLA_MACSEC_SCI] = { .type = NLA_U64 },
 	[IFLA_MACSEC_PORT] = { .type = NLA_U16 },
 	[IFLA_MACSEC_ICV_LEN] = NLA_POLICY_RANGE(NLA_U8, MACSEC_MIN_ICV_LEN, MACSEC_STD_ICV_LEN),
-	[IFLA_MACSEC_CIPHER_SUITE] = { .type = NLA_U64 },
+	[IFLA_MACSEC_CIPHER_SUITE] = NLA_POLICY_VALIDATE_FN(NLA_U64, validate_cipher_suite),
 	[IFLA_MACSEC_WINDOW] = { .type = NLA_U32 },
 	[IFLA_MACSEC_ENCODING_SA] = { .type = NLA_U8 },
 	[IFLA_MACSEC_ENCRYPT] = { .type = NLA_U8 },
@@ -4225,10 +4227,24 @@ static int macsec_newlink(struct net_device *dev,
 	return err;
 }
 
+static int validate_cipher_suite(const struct nlattr *attr,
+				 struct netlink_ext_ack *extack)
+{
+	switch (nla_get_u64(attr)) {
+	case MACSEC_CIPHER_ID_GCM_AES_128:
+	case MACSEC_CIPHER_ID_GCM_AES_256:
+	case MACSEC_CIPHER_ID_GCM_AES_XPN_128:
+	case MACSEC_CIPHER_ID_GCM_AES_XPN_256:
+	case MACSEC_DEFAULT_CIPHER_ID:
+		return 0;
+	default:
+		return -EINVAL;
+	}
+}
+
 static int macsec_validate_attr(struct nlattr *tb[], struct nlattr *data[],
 				struct netlink_ext_ack *extack)
 {
-	u64 csid = MACSEC_DEFAULT_CIPHER_ID;
 	u8 icv_len = MACSEC_DEFAULT_ICV_LEN;
 	int flag;
 	bool es, scb, sci;
@@ -4236,9 +4252,6 @@ static int macsec_validate_attr(struct nlattr *tb[], struct nlattr *data[],
 	if (!data)
 		return 0;
 
-	if (data[IFLA_MACSEC_CIPHER_SUITE])
-		csid = nla_get_u64(data[IFLA_MACSEC_CIPHER_SUITE]);
-
 	if (data[IFLA_MACSEC_ICV_LEN]) {
 		icv_len = nla_get_u8(data[IFLA_MACSEC_ICV_LEN]);
 		if (icv_len != MACSEC_DEFAULT_ICV_LEN) {
@@ -4254,17 +4267,6 @@ static int macsec_validate_attr(struct nlattr *tb[], struct nlattr *data[],
 		}
 	}
 
-	switch (csid) {
-	case MACSEC_CIPHER_ID_GCM_AES_128:
-	case MACSEC_CIPHER_ID_GCM_AES_256:
-	case MACSEC_CIPHER_ID_GCM_AES_XPN_128:
-	case MACSEC_CIPHER_ID_GCM_AES_XPN_256:
-	case MACSEC_DEFAULT_CIPHER_ID:
-		break;
-	default:
-		return -EINVAL;
-	}
-
 	if (data[IFLA_MACSEC_ENCODING_SA]) {
 		if (nla_get_u8(data[IFLA_MACSEC_ENCODING_SA]) >= MACSEC_NUM_AN)
 			return -EINVAL;
-- 
2.50.0

[PATCH net-next v2 11/13] macsec: validate IFLA_MACSEC_VALIDATION with NLA_POLICY_MAX

[Sabrina Dubroca]

Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 drivers/net/macsec.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index 5680e4b78dda..dc17b91dce2d 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -3772,7 +3772,7 @@ static const struct nla_policy macsec_rtnl_policy[IFLA_MACSEC_MAX + 1] = {
 	[IFLA_MACSEC_ES] = { .type = NLA_U8 },
 	[IFLA_MACSEC_SCB] = { .type = NLA_U8 },
 	[IFLA_MACSEC_REPLAY_PROTECT] = { .type = NLA_U8 },
-	[IFLA_MACSEC_VALIDATION] = { .type = NLA_U8 },
+	[IFLA_MACSEC_VALIDATION] = NLA_POLICY_MAX(NLA_U8, MACSEC_VALIDATE_MAX),
 	[IFLA_MACSEC_OFFLOAD] = NLA_POLICY_MAX(NLA_U8, MACSEC_OFFLOAD_MAX),
 };
 
@@ -4288,10 +4288,6 @@ static int macsec_validate_attr(struct nlattr *tb[], struct nlattr *data[],
 	if ((sci && (scb || es)) || (scb && es))
 		return -EINVAL;
 
-	if (data[IFLA_MACSEC_VALIDATION] &&
-	    nla_get_u8(data[IFLA_MACSEC_VALIDATION]) > MACSEC_VALIDATE_MAX)
-		return -EINVAL;
-
 	if ((data[IFLA_MACSEC_REPLAY_PROTECT] &&
 	     nla_get_u8(data[IFLA_MACSEC_REPLAY_PROTECT])) &&
 	    !data[IFLA_MACSEC_WINDOW])
-- 
2.50.0

[PATCH net-next v2 12/13] macsec: replace custom checks for IFLA_MACSEC_* flags with NLA_POLICY_MAX

[Sabrina Dubroca]

Those are all off/on flags.

Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 drivers/net/macsec.c | 22 ++++++----------------
 1 file changed, 6 insertions(+), 16 deletions(-)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index dc17b91dce2d..115cdf347643 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -3766,12 +3766,12 @@ static const struct nla_policy macsec_rtnl_policy[IFLA_MACSEC_MAX + 1] = {
 	[IFLA_MACSEC_CIPHER_SUITE] = NLA_POLICY_VALIDATE_FN(NLA_U64, validate_cipher_suite),
 	[IFLA_MACSEC_WINDOW] = { .type = NLA_U32 },
 	[IFLA_MACSEC_ENCODING_SA] = { .type = NLA_U8 },
-	[IFLA_MACSEC_ENCRYPT] = { .type = NLA_U8 },
-	[IFLA_MACSEC_PROTECT] = { .type = NLA_U8 },
-	[IFLA_MACSEC_INC_SCI] = { .type = NLA_U8 },
-	[IFLA_MACSEC_ES] = { .type = NLA_U8 },
-	[IFLA_MACSEC_SCB] = { .type = NLA_U8 },
-	[IFLA_MACSEC_REPLAY_PROTECT] = { .type = NLA_U8 },
+	[IFLA_MACSEC_ENCRYPT] = NLA_POLICY_MAX(NLA_U8, 1),
+	[IFLA_MACSEC_PROTECT] = NLA_POLICY_MAX(NLA_U8, 1),
+	[IFLA_MACSEC_INC_SCI] = NLA_POLICY_MAX(NLA_U8, 1),
+	[IFLA_MACSEC_ES] = NLA_POLICY_MAX(NLA_U8, 1),
+	[IFLA_MACSEC_SCB] = NLA_POLICY_MAX(NLA_U8, 1),
+	[IFLA_MACSEC_REPLAY_PROTECT] = NLA_POLICY_MAX(NLA_U8, 1),
 	[IFLA_MACSEC_VALIDATION] = NLA_POLICY_MAX(NLA_U8, MACSEC_VALIDATE_MAX),
 	[IFLA_MACSEC_OFFLOAD] = NLA_POLICY_MAX(NLA_U8, MACSEC_OFFLOAD_MAX),
 };
@@ -4246,7 +4246,6 @@ static int macsec_validate_attr(struct nlattr *tb[], struct nlattr *data[],
 				struct netlink_ext_ack *extack)
 {
 	u8 icv_len = MACSEC_DEFAULT_ICV_LEN;
-	int flag;
 	bool es, scb, sci;
 
 	if (!data)
@@ -4272,15 +4271,6 @@ static int macsec_validate_attr(struct nlattr *tb[], struct nlattr *data[],
 			return -EINVAL;
 	}
 
-	for (flag = IFLA_MACSEC_ENCODING_SA + 1;
-	     flag < IFLA_MACSEC_VALIDATION;
-	     flag++) {
-		if (data[flag]) {
-			if (nla_get_u8(data[flag]) > 1)
-				return -EINVAL;
-		}
-	}
-
 	es  = nla_get_u8_default(data[IFLA_MACSEC_ES], false);
 	sci = nla_get_u8_default(data[IFLA_MACSEC_INC_SCI], false);
 	scb = nla_get_u8_default(data[IFLA_MACSEC_SCB], false);
-- 
2.50.0

[PATCH net-next v2 13/13] macsec: replace custom check on IFLA_MACSEC_ENCODING_SA with NLA_POLICY_MAX

[Sabrina Dubroca]

Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 drivers/net/macsec.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/net/macsec.c b/drivers/net/macsec.c
index 115cdf347643..316c5352ec2f 100644
--- a/drivers/net/macsec.c
+++ b/drivers/net/macsec.c
@@ -3765,7 +3765,7 @@ static const struct nla_policy macsec_rtnl_policy[IFLA_MACSEC_MAX + 1] = {
 	[IFLA_MACSEC_ICV_LEN] = NLA_POLICY_RANGE(NLA_U8, MACSEC_MIN_ICV_LEN, MACSEC_STD_ICV_LEN),
 	[IFLA_MACSEC_CIPHER_SUITE] = NLA_POLICY_VALIDATE_FN(NLA_U64, validate_cipher_suite),
 	[IFLA_MACSEC_WINDOW] = { .type = NLA_U32 },
-	[IFLA_MACSEC_ENCODING_SA] = { .type = NLA_U8 },
+	[IFLA_MACSEC_ENCODING_SA] = NLA_POLICY_MAX(NLA_U8, MACSEC_NUM_AN - 1),
 	[IFLA_MACSEC_ENCRYPT] = NLA_POLICY_MAX(NLA_U8, 1),
 	[IFLA_MACSEC_PROTECT] = NLA_POLICY_MAX(NLA_U8, 1),
 	[IFLA_MACSEC_INC_SCI] = NLA_POLICY_MAX(NLA_U8, 1),
@@ -4266,11 +4266,6 @@ static int macsec_validate_attr(struct nlattr *tb[], struct nlattr *data[],
 		}
 	}
 
-	if (data[IFLA_MACSEC_ENCODING_SA]) {
-		if (nla_get_u8(data[IFLA_MACSEC_ENCODING_SA]) >= MACSEC_NUM_AN)
-			return -EINVAL;
-	}
-
 	es  = nla_get_u8_default(data[IFLA_MACSEC_ES], false);
 	sci = nla_get_u8_default(data[IFLA_MACSEC_INC_SCI], false);
 	scb = nla_get_u8_default(data[IFLA_MACSEC_SCB], false);
-- 
2.50.0

Re: [PATCH net-next v2 01/13] macsec: replace custom checks on MACSEC_SA_ATTR_AN with NLA_POLICY_MAX

[Simon Horman]

On Tue, Aug 26, 2025 at 03:16:19PM +0200, Sabrina Dubroca wrote:
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next v2 02/13] macsec: replace custom checks on MACSEC_*_ATTR_ACTIVE with NLA_POLICY_MAX

[Simon Horman]

On Tue, Aug 26, 2025 at 03:16:20PM +0200, Sabrina Dubroca wrote:
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next v2 03/13] macsec: replace custom checks on MACSEC_SA_ATTR_SALT with NLA_POLICY_EXACT_LEN

[Simon Horman]

On Tue, Aug 26, 2025 at 03:16:21PM +0200, Sabrina Dubroca wrote:
> The existing checks already specify that MACSEC_SA_ATTR_SALT must have
> length MACSEC_SALT_LEN.
> 
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next v2 04/13] macsec: replace custom checks on MACSEC_SA_ATTR_KEYID with NLA_POLICY_EXACT_LEN

[Simon Horman]

On Tue, Aug 26, 2025 at 03:16:22PM +0200, Sabrina Dubroca wrote:
> The existing checks already specify that MACSEC_SA_ATTR_KEYID must
> have length MACSEC_KEYID_LEN.
> 
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next v2 05/13] macsec: use NLA_POLICY_MAX_LEN for MACSEC_SA_ATTR_KEY

[Simon Horman]

On Tue, Aug 26, 2025 at 03:16:23PM +0200, Sabrina Dubroca wrote:
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next v2 06/13] macsec: use NLA_UINT for MACSEC_SA_ATTR_PN

[Simon Horman]

On Tue, Aug 26, 2025 at 03:16:24PM +0200, Sabrina Dubroca wrote:
> MACSEC_SA_ATTR_PN is either a u32 or a u64, we can now use NLA_UINT
> for this instead of a custom binary type. We can then use a min check
> within the policy.
> 
> We need to keep the length checks done in macsec_{add,upd}_{rx,tx}sa
> based on whether the device is set up for XPN (with 64b PNs instead of
> 32b).
> 
> On the dump side, keep the existing custom code as userspace may
> expect a u64 when using XPN, and nla_put_uint may only output a u32
> attribute if the value fits.
> 
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next v2 07/13] macsec: remove validate_add_rxsc

[Simon Horman]

On Tue, Aug 26, 2025 at 03:16:25PM +0200, Sabrina Dubroca wrote:
> It's not doing much anymore.
> 
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next v2 08/13] macsec: add NLA_POLICY_MAX for MACSEC_OFFLOAD_ATTR_TYPE and IFLA_MACSEC_OFFLOAD

[Simon Horman]

On Tue, Aug 26, 2025 at 03:16:26PM +0200, Sabrina Dubroca wrote:
> This is equivalent to the existing checks allowing either
> MACSEC_OFFLOAD_OFF or calling macsec_check_offload.
> 
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next v2 09/13] macsec: replace custom checks on IFLA_MACSEC_ICV_LEN with NLA_POLICY_RANGE

[Simon Horman]

On Tue, Aug 26, 2025 at 03:16:27PM +0200, Sabrina Dubroca wrote:
> The existing checks already force this range.
> 
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next v2 10/13] macsec: use NLA_POLICY_VALIDATE_FN to validate IFLA_MACSEC_CIPHER_SUITE

[Simon Horman]

On Tue, Aug 26, 2025 at 03:16:28PM +0200, Sabrina Dubroca wrote:
> Unfortunately, since the value of MACSEC_DEFAULT_CIPHER_ID doesn't fit
> near the others, we can't use a simple range in the policy.
> 
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next v2 11/13] macsec: validate IFLA_MACSEC_VALIDATION with NLA_POLICY_MAX

[Simon Horman]

On Tue, Aug 26, 2025 at 03:16:29PM +0200, Sabrina Dubroca wrote:
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next v2 12/13] macsec: replace custom checks for IFLA_MACSEC_* flags with NLA_POLICY_MAX

[Simon Horman]

On Tue, Aug 26, 2025 at 03:16:30PM +0200, Sabrina Dubroca wrote:
> Those are all off/on flags.
> 
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next v2 13/13] macsec: replace custom check on IFLA_MACSEC_ENCODING_SA with NLA_POLICY_MAX

[Simon Horman]

On Tue, Aug 26, 2025 at 03:16:31PM +0200, Sabrina Dubroca wrote:
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>

Reviewed-by: Simon Horman <horms@kernel.org>

Re: [PATCH net-next v2 08/13] macsec: add NLA_POLICY_MAX for MACSEC_OFFLOAD_ATTR_TYPE and IFLA_MACSEC_OFFLOAD

[Jakub Kicinski]

On Tue, 26 Aug 2025 15:16:26 +0200 Sabrina Dubroca wrote:
> This is equivalent to the existing checks allowing either
> MACSEC_OFFLOAD_OFF or calling macsec_check_offload.

AFAICT "equivalent" is a bit misleading as we didn't have validation.
But seems low risk.

Re: [PATCH net-next v2 06/13] macsec: use NLA_UINT for MACSEC_SA_ATTR_PN

[Jakub Kicinski]

On Tue, 26 Aug 2025 15:16:24 +0200 Sabrina Dubroca wrote:
> MACSEC_SA_ATTR_PN is either a u32 or a u64, we can now use NLA_UINT
> for this instead of a custom binary type. We can then use a min check
> within the policy.
> 
> We need to keep the length checks done in macsec_{add,upd}_{rx,tx}sa
> based on whether the device is set up for XPN (with 64b PNs instead of
> 32b).
> 
> On the dump side, keep the existing custom code as userspace may
> expect a u64 when using XPN, and nla_put_uint may only output a u32
> attribute if the value fits.

I think this is a slight functional change on big endian.
I suppose we don't care..

Re: [PATCH net-next v2 06/13] macsec: use NLA_UINT for MACSEC_SA_ATTR_PN

[Jakub Kicinski]

On Wed, 27 Aug 2025 18:54:15 -0700 Jakub Kicinski wrote:
> On Tue, 26 Aug 2025 15:16:24 +0200 Sabrina Dubroca wrote:
> > MACSEC_SA_ATTR_PN is either a u32 or a u64, we can now use NLA_UINT
> > for this instead of a custom binary type. We can then use a min check
> > within the policy.
> > 
> > We need to keep the length checks done in macsec_{add,upd}_{rx,tx}sa
> > based on whether the device is set up for XPN (with 64b PNs instead of
> > 32b).
> > 
> > On the dump side, keep the existing custom code as userspace may
> > expect a u64 when using XPN, and nla_put_uint may only output a u32
> > attribute if the value fits.  
> 
> I think this is a slight functional change on big endian.
> I suppose we don't care..

we don't care == the change is not intentional, so in the unlikely case
BE users exist aligning with LE is better in the first place.

Re: [PATCH net-next v2 00/13] macsec: replace custom netlink attribute checks with policy-level checks

[patchwork-bot+netdevbpf]

Hello:

This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 26 Aug 2025 15:16:18 +0200 you wrote:
> We can simplify attribute validation a lot by describing the accepted
> ranges more precisely in the policies, using NLA_POLICY_MAX etc.
> 
> Some of the checks still need to be done later on, because the
> attribute length and acceptable range can vary based on values that
> can't be known when the policy is validated (cipher suite determines
> the key length and valid ICV length, presence of XPN changes the PN
> length, detection of duplicate SCIs or ANs, etc).
> 
> [...]

Here is the summary with links:
  - [net-next,v2,01/13] macsec: replace custom checks on MACSEC_SA_ATTR_AN with NLA_POLICY_MAX
    https://git.kernel.org/netdev/net-next/c/d5e0a8cec12c
  - [net-next,v2,02/13] macsec: replace custom checks on MACSEC_*_ATTR_ACTIVE with NLA_POLICY_MAX
    https://git.kernel.org/netdev/net-next/c/ae6a8f5abed1
  - [net-next,v2,03/13] macsec: replace custom checks on MACSEC_SA_ATTR_SALT with NLA_POLICY_EXACT_LEN
    https://git.kernel.org/netdev/net-next/c/8cf22afc152c
  - [net-next,v2,04/13] macsec: replace custom checks on MACSEC_SA_ATTR_KEYID with NLA_POLICY_EXACT_LEN
    https://git.kernel.org/netdev/net-next/c/d29ae0d7753a
  - [net-next,v2,05/13] macsec: use NLA_POLICY_MAX_LEN for MACSEC_SA_ATTR_KEY
    https://git.kernel.org/netdev/net-next/c/15a700a8429e
  - [net-next,v2,06/13] macsec: use NLA_UINT for MACSEC_SA_ATTR_PN
    https://git.kernel.org/netdev/net-next/c/82f3116132fc
  - [net-next,v2,07/13] macsec: remove validate_add_rxsc
    https://git.kernel.org/netdev/net-next/c/80810c89d39c
  - [net-next,v2,08/13] macsec: add NLA_POLICY_MAX for MACSEC_OFFLOAD_ATTR_TYPE and IFLA_MACSEC_OFFLOAD
    https://git.kernel.org/netdev/net-next/c/35a35279e8ff
  - [net-next,v2,09/13] macsec: replace custom checks on IFLA_MACSEC_ICV_LEN with NLA_POLICY_RANGE
    https://git.kernel.org/netdev/net-next/c/17882d23a6c6
  - [net-next,v2,10/13] macsec: use NLA_POLICY_VALIDATE_FN to validate IFLA_MACSEC_CIPHER_SUITE
    https://git.kernel.org/netdev/net-next/c/4d844cb1ea1f
  - [net-next,v2,11/13] macsec: validate IFLA_MACSEC_VALIDATION with NLA_POLICY_MAX
    https://git.kernel.org/netdev/net-next/c/b81d1e958867
  - [net-next,v2,12/13] macsec: replace custom checks for IFLA_MACSEC_* flags with NLA_POLICY_MAX
    https://git.kernel.org/netdev/net-next/c/b46f5ddb40c8
  - [net-next,v2,13/13] macsec: replace custom check on IFLA_MACSEC_ENCODING_SA with NLA_POLICY_MAX
    https://git.kernel.org/netdev/net-next/c/db9dfc4d30dd

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

Re: [PATCH net-next v2 08/13] macsec: add NLA_POLICY_MAX for MACSEC_OFFLOAD_ATTR_TYPE and IFLA_MACSEC_OFFLOAD

[Sabrina Dubroca]

2025-08-27, 18:51:49 -0700, Jakub Kicinski wrote:
> On Tue, 26 Aug 2025 15:16:26 +0200 Sabrina Dubroca wrote:
> > This is equivalent to the existing checks allowing either
> > MACSEC_OFFLOAD_OFF or calling macsec_check_offload.
> 
> AFAICT "equivalent" is a bit misleading as we didn't have validation.
> But seems low risk.

We didn't have validation, but macsec_check_offload would return false
if offload was not one of {MACSEC_OFFLOAD_PHY,MACSEC_OFFLOAD_MAC}.
So I don't think this is adding any constraints compared to what we
already did.

-- 
Sabrina

Re: [PATCH net-next v2 06/13] macsec: use NLA_UINT for MACSEC_SA_ATTR_PN

[Sabrina Dubroca]

2025-08-27, 18:55:40 -0700, Jakub Kicinski wrote:
> On Wed, 27 Aug 2025 18:54:15 -0700 Jakub Kicinski wrote:
> > On Tue, 26 Aug 2025 15:16:24 +0200 Sabrina Dubroca wrote:
> > > MACSEC_SA_ATTR_PN is either a u32 or a u64, we can now use NLA_UINT
> > > for this instead of a custom binary type. We can then use a min check
> > > within the policy.
> > > 
> > > We need to keep the length checks done in macsec_{add,upd}_{rx,tx}sa
> > > based on whether the device is set up for XPN (with 64b PNs instead of
> > > 32b).
> > > 
> > > On the dump side, keep the existing custom code as userspace may
> > > expect a u64 when using XPN, and nla_put_uint may only output a u32
> > > attribute if the value fits.  
> > 
> > I think this is a slight functional change on big endian.
> > I suppose we don't care..
> 
> we don't care == the change is not intentional, so in the unlikely case
> BE users exist aligning with LE is better in the first place.

I don't think this is changing the behavior. The previous check was
copying whatever bytes were in the attribute into a u64 (incorrectly
on BE) and setting the rest to 0, and then checking that this u64 is
!= 0. The new check is reading the value correctly and also checking
that it's != 0.

Converting the nla_get_u64(MACSEC_SA_ATTR_PN) in
macsec_{add,upd}_{rx,tx}sa to get_uint would change the behavior on
BE, but the current code hasn't worked correctly since XPN was
introduced? We use 1<<32 instead of 1 as our PN, which doesn't make
sense when using 32bit PNs. So I think we'll want to change all the
nla_get_u64(MACSEC_SA_ATTR_PN) into
nla_get_uint(MACSEC_SA_ATTR_PN). WDYT?

-- 
Sabrina

Re: [PATCH net-next v2 06/13] macsec: use NLA_UINT for MACSEC_SA_ATTR_PN

[Jakub Kicinski]

On Thu, 28 Aug 2025 16:25:42 +0200 Sabrina Dubroca wrote:
> 2025-08-27, 18:55:40 -0700, Jakub Kicinski wrote:
> > On Wed, 27 Aug 2025 18:54:15 -0700 Jakub Kicinski wrote:  
> > > On Tue, 26 Aug 2025 15:16:24 +0200 Sabrina Dubroca wrote:  
> > > I think this is a slight functional change on big endian.
> > > I suppose we don't care..  
> > 
> > we don't care == the change is not intentional, so in the unlikely case
> > BE users exist aligning with LE is better in the first place.  
> 
> I don't think this is changing the behavior. The previous check was
> copying whatever bytes were in the attribute into a u64 (incorrectly
> on BE) and setting the rest to 0, and then checking that this u64 is
> != 0. The new check is reading the value correctly and also checking
> that it's != 0.
> 
> Converting the nla_get_u64(MACSEC_SA_ATTR_PN) in
> macsec_{add,upd}_{rx,tx}sa to get_uint would change the behavior on
> BE, but the current code hasn't worked correctly since XPN was
> introduced? We use 1<<32 instead of 1 as our PN, which doesn't make
> sense when using 32bit PNs. So I think we'll want to change all the
> nla_get_u64(MACSEC_SA_ATTR_PN) into
> nla_get_uint(MACSEC_SA_ATTR_PN). WDYT?

Heh, TBH I misconstrued the patch, I thought you already did
switch to get_uint. I think switching makes sense, I don't think
anyone is expecting the difference in behavior between LE and BE.