* [PATCH main v3 1/2] macsec: add Extended Packet Number support
@ 2022-09-04 7:47 Emeel Hakim
2022-09-04 7:47 ` [PATCH main v3 2/2] macsec: add user manual description for extended packet number feature Emeel Hakim
2022-09-04 15:44 ` [PATCH main v3 1/2] macsec: add Extended Packet Number support Sabrina Dubroca
0 siblings, 2 replies; 4+ messages in thread
From: Emeel Hakim @ 2022-09-04 7:47 UTC (permalink / raw)
To: dsahern, sd; +Cc: netdev, raeds, tariqt, Emeel Hakim
This patch adds support for extended packet number (XPN).
XPN can be configured by passing 'xpn on' as part of the ip
link add command using macsec type.
In addition, using 'xpn' keyword instead of the 'pn', passing a 12
bytes salt using the 'salt' keyword and passing short secure channel
id (ssci) using the 'ssci' keyword as part of the ip macsec command
is required (see example).
e.g:
create a MACsec device on link eth0 with enabled xpn
# ip link add link eth0 macsec0 type macsec port 11
encrypt on xpn on
configure a secure association on the device
# ip macsec add macsec0 tx sa 0 xpn 1024 on ssci 5
salt 838383838383838383838383
key 01 81818181818181818181818181818181
configure a secure association on the device with ssci = 5
# ip macsec add macsec0 tx sa 0 xpn 1024 on ssci 5
salt 838383838383838383838383
key 01 82828282828282828282828282828282
Signed-off-by: Emeel Hakim <ehakim@nvidia.com>
---
V1->V2:
- Updated commit message.
- Related uapi change got accepted upstream.
"https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=5d8175783585"
- Allowed ssci to be empty to leave it up to the kernel to reject invalid
requests.
- Removed the flag option and exchanged it by a property for xpn.
- Added the 64b xpn, ssci as part of the dump.
V2->V3:
- Add dedicated function to read ssci correctly.
- Check for duplicate xpn and pn where command line has an xpn argument
with upper 32bits set followed by a pn argument.
- Don't use int to hold a boolean result.
ip/ipmacsec.c | 131 ++++++++++++++++++++++++++++++++++++++++++--------
1 file changed, 111 insertions(+), 20 deletions(-)
diff --git a/ip/ipmacsec.c b/ip/ipmacsec.c
index bf48e8b5..3bff765d 100644
--- a/ip/ipmacsec.c
+++ b/ip/ipmacsec.c
@@ -43,11 +43,17 @@ struct sci {
struct sa_desc {
__u8 an;
- __u32 pn;
+ union {
+ __u32 pn32;
+ __u64 pn64;
+ } pn;
__u8 key_id[MACSEC_KEYID_LEN];
__u32 key_len;
__u8 key[MACSEC_MAX_KEY_LEN];
__u8 active;
+ __u8 salt[MACSEC_SALT_LEN];
+ __u32 ssci;
+ bool xpn;
};
struct cipher_args {
@@ -98,10 +104,11 @@ static void ipmacsec_usage(void)
" ip macsec show\n"
" ip macsec show DEV\n"
" ip macsec offload DEV [ off | phy | mac ]\n"
- "where OPTS := [ pn <u32> ] [ on | off ]\n"
+ "where OPTS := [ pn <u32> | xpn <u64> ] [ salt SALT ] [ ssci <u32> ] [ on | off ]\n"
" ID := 128-bit hex string\n"
" KEY := 128-bit or 256-bit hex string\n"
- " SCI := { sci <u64> | port { 1..2^16-1 } address <lladdr> }\n");
+ " SCI := { sci <u64> | port { 1..2^16-1 } address <lladdr> }\n"
+ " SALT := 96-bit hex string\n");
exit(-1);
}
@@ -124,6 +131,11 @@ static int get_sci(__u64 *sci, const char *arg)
return get_be64(sci, arg, 16);
}
+static int get_ssci(__u32 *ssci, const char *arg)
+{
+ return get_be32(ssci, arg, 16);
+}
+
static int get_port(__be16 *port, const char *arg)
{
return get_be16(port, arg, 0);
@@ -174,14 +186,36 @@ static int parse_sa_args(int *argcp, char ***argvp, struct sa_desc *sa)
while (argc > 0) {
if (strcmp(*argv, "pn") == 0) {
- if (sa->pn != 0)
+ if (sa->pn.pn64 != 0)
duparg2("pn", "pn");
NEXT_ARG();
- ret = get_u32(&sa->pn, *argv, 0);
+ ret = get_u32(&sa->pn.pn32, *argv, 0);
+ if (ret)
+ invarg("expected pn", *argv);
+ if (sa->pn.pn32 == 0)
+ invarg("expected pn != 0", *argv);
+ } else if (strcmp(*argv, "xpn") == 0) {
+ if (sa->pn.pn64 != 0)
+ duparg2("xpn", "xpn");
+ NEXT_ARG();
+ ret = get_u64(&sa->pn.pn64, *argv, 0);
if (ret)
invarg("expected pn", *argv);
- if (sa->pn == 0)
+ if (sa->pn.pn64 == 0)
invarg("expected pn != 0", *argv);
+ sa->xpn = true;
+ } else if (strcmp(*argv, "salt") == 0) {
+ unsigned int len;
+
+ NEXT_ARG();
+ if (!hexstring_a2n(*argv, sa->salt, MACSEC_SALT_LEN,
+ &len))
+ invarg("expected salt", *argv);
+ } else if (strcmp(*argv, "ssci") == 0) {
+ NEXT_ARG();
+ ret = get_ssci(&sa->ssci, *argv);
+ if (ret)
+ invarg("expected ssci", *argv);
} else if (strcmp(*argv, "key") == 0) {
unsigned int len;
@@ -392,9 +426,22 @@ static int do_modify_nl(enum cmd c, enum macsec_nl_commands cmd, int ifindex,
addattr8(&req.n, MACSEC_BUFLEN, MACSEC_SA_ATTR_AN, sa->an);
if (c != CMD_DEL) {
- if (sa->pn)
+ if (sa->xpn) {
+ if (sa->pn.pn64)
+ addattr64(&req.n, MACSEC_BUFLEN, MACSEC_SA_ATTR_PN,
+ sa->pn.pn64);
+ if (sa->salt[0] != '\0')
+ addattr_l(&req.n, MACSEC_BUFLEN, MACSEC_SA_ATTR_SALT,
+ sa->salt, MACSEC_SALT_LEN);
+ if (sa->ssci != 0)
+ addattr32(&req.n, MACSEC_BUFLEN, MACSEC_SA_ATTR_SSCI,
+ sa->ssci);
+ }
+
+ if (sa->pn.pn32 && !sa->xpn) {
addattr32(&req.n, MACSEC_BUFLEN, MACSEC_SA_ATTR_PN,
- sa->pn);
+ sa->pn.pn32);
+ }
if (sa->key_len) {
addattr_l(&req.n, MACSEC_BUFLEN, MACSEC_SA_ATTR_KEYID,
@@ -426,10 +473,11 @@ static bool check_sa_args(enum cmd c, struct sa_desc *sa)
return -1;
}
- if (sa->pn == 0) {
+ if (sa->pn.pn64 == 0) {
fprintf(stderr, "must specify a packet number != 0\n");
return -1;
}
+
} else if (c == CMD_UPD) {
if (sa->key_len) {
fprintf(stderr, "cannot change key on SA\n");
@@ -615,6 +663,9 @@ static void print_key(struct rtattr *key)
#define CIPHER_NAME_GCM_AES_128 "GCM-AES-128"
#define CIPHER_NAME_GCM_AES_256 "GCM-AES-256"
+#define CIPHER_NAME_GCM_AES_XPN_128 "GCM-AES-XPN-128"
+#define CIPHER_NAME_GCM_AES_XPN_256 "GCM-AES-XPN-256"
+
#define DEFAULT_CIPHER_NAME CIPHER_NAME_GCM_AES_128
static const char *cs_id_to_name(__u64 cid)
@@ -627,6 +678,10 @@ static const char *cs_id_to_name(__u64 cid)
return CIPHER_NAME_GCM_AES_128;
case MACSEC_CIPHER_ID_GCM_AES_256:
return CIPHER_NAME_GCM_AES_256;
+ case MACSEC_CIPHER_ID_GCM_AES_XPN_128:
+ return CIPHER_NAME_GCM_AES_XPN_128;
+ case MACSEC_CIPHER_ID_GCM_AES_XPN_256:
+ return CIPHER_NAME_GCM_AES_XPN_256;
default:
return "(unknown)";
}
@@ -846,8 +901,8 @@ static void print_txsa_stats(const char *prefix, struct rtattr *attr)
}
static void print_tx_sc(const char *prefix, __u64 sci, __u8 encoding_sa,
- struct rtattr *txsc_stats, struct rtattr *secy_stats,
- struct rtattr *sa)
+ bool is_xpn, struct rtattr *txsc_stats,
+ struct rtattr *secy_stats, struct rtattr *sa)
{
struct rtattr *sa_attr[MACSEC_SA_ATTR_MAX + 1];
struct rtattr *a;
@@ -875,8 +930,16 @@ static void print_tx_sc(const char *prefix, __u64 sci, __u8 encoding_sa,
print_string(PRINT_FP, NULL, "%s", prefix);
print_uint(PRINT_ANY, "an", "%d:",
rta_getattr_u8(sa_attr[MACSEC_SA_ATTR_AN]));
- print_uint(PRINT_ANY, "pn", " PN %u,",
- rta_getattr_u32(sa_attr[MACSEC_SA_ATTR_PN]));
+ if (!is_xpn) {
+ print_uint(PRINT_ANY, "pn", " PN %u,",
+ rta_getattr_u32(sa_attr[MACSEC_SA_ATTR_PN]));
+ } else {
+ print_uint(PRINT_ANY, "pn", " PN %u,",
+ rta_getattr_u64(sa_attr[MACSEC_SA_ATTR_PN]));
+ print_0xhex(PRINT_ANY, "ssci",
+ "SSCI %08x",
+ ntohl(rta_getattr_u32(sa_attr[MACSEC_SA_ATTR_SSCI])));
+ }
print_bool(PRINT_JSON, "active", NULL, state);
print_string(PRINT_FP, NULL,
@@ -916,7 +979,8 @@ static void print_rxsc_stats(const char *prefix, struct rtattr *attr)
}
static void print_rx_sc(const char *prefix, __be64 sci, __u8 active,
- struct rtattr *rxsc_stats, struct rtattr *sa)
+ bool is_xpn, struct rtattr *rxsc_stats,
+ struct rtattr *sa)
{
struct rtattr *sa_attr[MACSEC_SA_ATTR_MAX + 1];
struct rtattr *a;
@@ -943,8 +1007,16 @@ static void print_rx_sc(const char *prefix, __be64 sci, __u8 active,
print_string(PRINT_FP, NULL, "%s", prefix);
print_uint(PRINT_ANY, "an", "%u:",
rta_getattr_u8(sa_attr[MACSEC_SA_ATTR_AN]));
- print_uint(PRINT_ANY, "pn", " PN %u,",
- rta_getattr_u32(sa_attr[MACSEC_SA_ATTR_PN]));
+ if (!is_xpn) {
+ print_uint(PRINT_ANY, "pn", " PN %u,",
+ rta_getattr_u32(sa_attr[MACSEC_SA_ATTR_PN]));
+ } else {
+ print_uint(PRINT_ANY, "pn", " PN %u,",
+ rta_getattr_u64(sa_attr[MACSEC_SA_ATTR_PN]));
+ print_0xhex(PRINT_ANY, "ssci",
+ "SSCI %08x",
+ ntohl(rta_getattr_u32(sa_attr[MACSEC_SA_ATTR_SSCI])));
+ }
print_bool(PRINT_JSON, "active", NULL, state);
print_string(PRINT_FP, NULL, " state %s,",
@@ -958,7 +1030,7 @@ static void print_rx_sc(const char *prefix, __be64 sci, __u8 active,
close_json_array(PRINT_JSON, NULL);
}
-static void print_rxsc_list(struct rtattr *sc)
+static void print_rxsc_list(struct rtattr *sc, bool is_xpn)
{
int rem = RTA_PAYLOAD(sc);
struct rtattr *c;
@@ -973,6 +1045,7 @@ static void print_rxsc_list(struct rtattr *sc)
print_rx_sc(" ",
rta_getattr_u64(sc_attr[MACSEC_RXSC_ATTR_SCI]),
rta_getattr_u32(sc_attr[MACSEC_RXSC_ATTR_ACTIVE]),
+ is_xpn,
sc_attr[MACSEC_RXSC_ATTR_STATS],
sc_attr[MACSEC_RXSC_ATTR_SA_LIST]);
close_json_object();
@@ -989,6 +1062,8 @@ static int process(struct nlmsghdr *n, void *arg)
int ifindex;
__u64 sci;
__u8 encoding_sa;
+ __u64 cid;
+ bool is_xpn = false;
if (n->nlmsg_type != genl_family)
return -1;
@@ -1032,13 +1107,16 @@ static int process(struct nlmsghdr *n, void *arg)
print_attrs(attrs_secy);
- print_tx_sc(" ", sci, encoding_sa,
+ cid = rta_getattr_u64(attrs_secy[MACSEC_SECY_ATTR_CIPHER_SUITE]);
+ if (cid == MACSEC_CIPHER_ID_GCM_AES_XPN_128 || cid == MACSEC_CIPHER_ID_GCM_AES_XPN_256)
+ is_xpn = true;
+ print_tx_sc(" ", sci, encoding_sa, is_xpn,
attrs[MACSEC_ATTR_TXSC_STATS],
attrs[MACSEC_ATTR_SECY_STATS],
attrs[MACSEC_ATTR_TXSA_LIST]);
if (attrs[MACSEC_ATTR_RXSC_LIST])
- print_rxsc_list(attrs[MACSEC_ATTR_RXSC_LIST]);
+ print_rxsc_list(attrs[MACSEC_ATTR_RXSC_LIST], is_xpn);
if (attrs[MACSEC_ATTR_OFFLOAD]) {
struct rtattr *attrs_offload[MACSEC_OFFLOAD_ATTR_MAX + 1];
@@ -1251,6 +1329,7 @@ static void usage(FILE *f)
" [ send_sci { on | off } ]\n"
" [ end_station { on | off } ]\n"
" [ scb { on | off } ]\n"
+ " [ xpn { on | off } ]\n"
" [ protect { on | off } ]\n"
" [ replay { on | off} window { 0..2^32-1 } ]\n"
" [ validate { strict | check | disabled } ]\n"
@@ -1268,7 +1347,7 @@ static int macsec_parse_opt(struct link_util *lu, int argc, char **argv,
enum macsec_offload offload;
struct cipher_args cipher = {0};
enum macsec_validation_type validate;
- bool es = false, scb = false, send_sci = false;
+ bool es = false, scb = false, send_sci = false, xpn = false;
int replay_protect = -1;
struct sci sci = { 0 };
@@ -1388,6 +1467,11 @@ static int macsec_parse_opt(struct link_util *lu, int argc, char **argv,
return ret;
addattr8(n, MACSEC_BUFLEN,
IFLA_MACSEC_OFFLOAD, offload);
+ } else if (strcmp(*argv, "xpn") == 0) {
+ NEXT_ARG();
+ xpn = parse_on_off("xpn", *argv, &ret);
+ if (ret != 0)
+ return ret;
} else {
fprintf(stderr, "macsec: unknown command \"%s\"?\n",
*argv);
@@ -1415,6 +1499,13 @@ static int macsec_parse_opt(struct link_util *lu, int argc, char **argv,
return -1;
}
+ if (xpn) {
+ if (cipher.id == MACSEC_CIPHER_ID_GCM_AES_256)
+ cipher.id = MACSEC_CIPHER_ID_GCM_AES_XPN_256;
+ else
+ cipher.id = MACSEC_CIPHER_ID_GCM_AES_XPN_128;
+ }
+
if (cipher.id)
addattr_l(n, MACSEC_BUFLEN, IFLA_MACSEC_CIPHER_SUITE,
&cipher.id, sizeof(cipher.id));
--
2.21.3
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH main v3 2/2] macsec: add user manual description for extended packet number feature
2022-09-04 7:47 [PATCH main v3 1/2] macsec: add Extended Packet Number support Emeel Hakim
@ 2022-09-04 7:47 ` Emeel Hakim
2022-09-04 15:44 ` [PATCH main v3 1/2] macsec: add Extended Packet Number support Sabrina Dubroca
1 sibling, 0 replies; 4+ messages in thread
From: Emeel Hakim @ 2022-09-04 7:47 UTC (permalink / raw)
To: dsahern, sd; +Cc: netdev, raeds, tariqt, Emeel Hakim
Update the user manual describing how to use extended packet number (XPN)
feature for macsec. As part of configuring XPN, providing ssci and salt is
required hence update user manual on how to provide the above as part of
the ip macsec command.
Signed-off-by: Emeel Hakim <ehakim@nvidia.com>
---
V1->V2:
- Updated documentation.
V2->V3:
man/man8/ip-macsec.8 | 52 +++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 51 insertions(+), 1 deletion(-)
diff --git a/man/man8/ip-macsec.8 b/man/man8/ip-macsec.8
index bb816157..991a775a 100644
--- a/man/man8/ip-macsec.8
+++ b/man/man8/ip-macsec.8
@@ -19,6 +19,7 @@ ip-macsec \- MACsec device configuration
.BR scb " { " on " | " off " } ] ["
.BR protect " { " on " | " off " } ] ["
.BR replay " { " on " | " off " } ] ["
+.BR xpn " { " on " | " off " } ] ["
.BI window " WINDOW"
] [
.BR validate " { " strict " | " check " | " disabled " } ] ["
@@ -63,7 +64,13 @@ ip-macsec \- MACsec device configuration
.IR OPTS " := [ "
.BR pn " { "
-.IR 1..2^32-1 " } ] ["
+.IR 1..2^32-1 " } |"
+.BR xpn " { "
+.IR 1..2^64-1 " } ] ["
+.B salt
+.IR SALT " ] ["
+.B ssci
+.IR <u32> " ] ["
.BR on " | " off " ]"
.br
.IR SCI " := { "
@@ -75,6 +82,8 @@ ip-macsec \- MACsec device configuration
}
.br
.IR PORT " := { " 1..2^16-1 " } "
+.br
+.IR SALT " := 96-bit hex string "
.SH DESCRIPTION
@@ -116,6 +125,29 @@ type.
.nf
# ip link add link eth0 macsec0 type macsec port 11 encrypt on offload mac
+.SH EXTENDED PACKET NUMBER EXAMPLES
+.PP
+.SS Create a MACsec device on link eth0 with enabled extended packet number (offload is disabled by default)
+.nf
+# ip link add link eth0 macsec0 type macsec port 11 encrypt on xpn on
+.PP
+.SS Configure a secure association on that device
+.nf
+# ip macsec add macsec0 tx sa 0 xpn 1024 on salt 838383838383838383838383 ssci 123 key 01 81818181818181818181818181818181
+.PP
+.SS Configure a receive channel
+.nf
+# ip macsec add macsec0 rx port 11 address c6:19:52:8f:e6:a0
+.PP
+.SS Configure a receive association
+.nf
+# ip macsec add macsec0 rx port 11 address c6:19:52:8f:e6:a0 sa 0 xpn 1 on salt 838383838383838383838383 ssci 123 key 00 82828282828282828282828282828282
+.PP
+.SS Display MACsec configuration
+.nf
+# ip macsec show
+.PP
+
.SH NOTES
This tool can be used to configure the 802.1AE keys of the interface. Note that 802.1AE uses GCM-AES
with a initialization vector (IV) derived from the packet number. The same key must not be used
@@ -125,6 +157,24 @@ that reconfigures the keys. It is wrong to just configure the keys statically an
indefinitely. The suggested and standardized way for key management is 802.1X-2010, which is implemented
by wpa_supplicant.
+.SH EXTENDED PACKET NUMBER NOTES
+Passing flag
+.B xpn
+to
+.B ip link add
+command using the
+.I macsec
+type requires using the keyword
+.B 'xpn'
+instead of
+.B 'pn'
+in addition to providing a salt using the
+.B 'salt'
+keyword when using the
+.B ip macsec
+command.
+
+
.SH SEE ALSO
.br
.BR ip-link (8)
--
2.21.3
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH main v3 1/2] macsec: add Extended Packet Number support
2022-09-04 7:47 [PATCH main v3 1/2] macsec: add Extended Packet Number support Emeel Hakim
2022-09-04 7:47 ` [PATCH main v3 2/2] macsec: add user manual description for extended packet number feature Emeel Hakim
@ 2022-09-04 15:44 ` Sabrina Dubroca
2022-09-07 14:03 ` Emeel Hakim
1 sibling, 1 reply; 4+ messages in thread
From: Sabrina Dubroca @ 2022-09-04 15:44 UTC (permalink / raw)
To: Emeel Hakim; +Cc: dsahern, netdev, raeds, tariqt
2022-09-04, 10:47:28 +0300, Emeel Hakim wrote:
> @@ -174,14 +186,36 @@ static int parse_sa_args(int *argcp, char ***argvp, struct sa_desc *sa)
>
> while (argc > 0) {
> if (strcmp(*argv, "pn") == 0) {
> - if (sa->pn != 0)
> + if (sa->pn.pn64 != 0)
> duparg2("pn", "pn");
> NEXT_ARG();
> - ret = get_u32(&sa->pn, *argv, 0);
> + ret = get_u32(&sa->pn.pn32, *argv, 0);
> + if (ret)
> + invarg("expected pn", *argv);
> + if (sa->pn.pn32 == 0)
> + invarg("expected pn != 0", *argv);
> + } else if (strcmp(*argv, "xpn") == 0) {
> + if (sa->pn.pn64 != 0)
> + duparg2("xpn", "xpn");
> + NEXT_ARG();
> + ret = get_u64(&sa->pn.pn64, *argv, 0);
> if (ret)
> invarg("expected pn", *argv);
> - if (sa->pn == 0)
> + if (sa->pn.pn64 == 0)
> invarg("expected pn != 0", *argv);
> + sa->xpn = true;
> + } else if (strcmp(*argv, "salt") == 0) {
> + unsigned int len;
> +
> + NEXT_ARG();
That should have a duparg check.
> + if (!hexstring_a2n(*argv, sa->salt, MACSEC_SALT_LEN,
> + &len))
> + invarg("expected salt", *argv);
> + } else if (strcmp(*argv, "ssci") == 0) {
> + NEXT_ARG();
Also worth a duparg check.
> + ret = get_ssci(&sa->ssci, *argv);
> + if (ret)
> + invarg("expected ssci", *argv);
> } else if (strcmp(*argv, "key") == 0) {
> unsigned int len;
>
> @@ -392,9 +426,22 @@ static int do_modify_nl(enum cmd c, enum macsec_nl_commands cmd, int ifindex,
> addattr8(&req.n, MACSEC_BUFLEN, MACSEC_SA_ATTR_AN, sa->an);
>
> if (c != CMD_DEL) {
> - if (sa->pn)
> + if (sa->xpn) {
> + if (sa->pn.pn64)
> + addattr64(&req.n, MACSEC_BUFLEN, MACSEC_SA_ATTR_PN,
> + sa->pn.pn64);
> + if (sa->salt[0] != '\0')
Does the specification say the salt can't start with a 0 byte, or is
it just a "salt was set" test? If that's coming from the spec, the
kernel should also check this.
> + addattr_l(&req.n, MACSEC_BUFLEN, MACSEC_SA_ATTR_SALT,
> + sa->salt, MACSEC_SALT_LEN);
> + if (sa->ssci != 0)
Same question as for the salt.
For both, I'd just add a salt_set/ssci_set flag to sa_desc, and use it
for duparg as well.
> + addattr32(&req.n, MACSEC_BUFLEN, MACSEC_SA_ATTR_SSCI,
> + sa->ssci);
> + }
> +
> + if (sa->pn.pn32 && !sa->xpn) {
Nit: combine this with the previous if:
} else if (sa->pn.pn32) {
> addattr32(&req.n, MACSEC_BUFLEN, MACSEC_SA_ATTR_PN,
> - sa->pn);
> + sa->pn.pn32);
> + }
>
> if (sa->key_len) {
> addattr_l(&req.n, MACSEC_BUFLEN, MACSEC_SA_ATTR_KEYID,
> @@ -426,10 +473,11 @@ static bool check_sa_args(enum cmd c, struct sa_desc *sa)
> return -1;
> }
>
> - if (sa->pn == 0) {
> + if (sa->pn.pn64 == 0) {
> fprintf(stderr, "must specify a packet number != 0\n");
> return -1;
> }
> +
Remove that blank line.
> } else if (c == CMD_UPD) {
> if (sa->key_len) {
> fprintf(stderr, "cannot change key on SA\n");
[...]
> @@ -875,8 +930,16 @@ static void print_tx_sc(const char *prefix, __u64 sci, __u8 encoding_sa,
> print_string(PRINT_FP, NULL, "%s", prefix);
> print_uint(PRINT_ANY, "an", "%d:",
> rta_getattr_u8(sa_attr[MACSEC_SA_ATTR_AN]));
> - print_uint(PRINT_ANY, "pn", " PN %u,",
> - rta_getattr_u32(sa_attr[MACSEC_SA_ATTR_PN]));
> + if (!is_xpn) {
Nit: I'd flip those branches so that the test is "if (is_xpn)"
> + print_uint(PRINT_ANY, "pn", " PN %u,",
> + rta_getattr_u32(sa_attr[MACSEC_SA_ATTR_PN]));
> + } else {
> + print_uint(PRINT_ANY, "pn", " PN %u,",
> + rta_getattr_u64(sa_attr[MACSEC_SA_ATTR_PN]));
> + print_0xhex(PRINT_ANY, "ssci",
> + "SSCI %08x",
> + ntohl(rta_getattr_u32(sa_attr[MACSEC_SA_ATTR_SSCI])));
> + }
>
> print_bool(PRINT_JSON, "active", NULL, state);
> print_string(PRINT_FP, NULL,
[...]
> @@ -943,8 +1007,16 @@ static void print_rx_sc(const char *prefix, __be64 sci, __u8 active,
> print_string(PRINT_FP, NULL, "%s", prefix);
> print_uint(PRINT_ANY, "an", "%u:",
> rta_getattr_u8(sa_attr[MACSEC_SA_ATTR_AN]));
> - print_uint(PRINT_ANY, "pn", " PN %u,",
> - rta_getattr_u32(sa_attr[MACSEC_SA_ATTR_PN]));
> + if (!is_xpn) {
Same nit, flip the test.
> + print_uint(PRINT_ANY, "pn", " PN %u,",
> + rta_getattr_u32(sa_attr[MACSEC_SA_ATTR_PN]));
> + } else {
> + print_uint(PRINT_ANY, "pn", " PN %u,",
> + rta_getattr_u64(sa_attr[MACSEC_SA_ATTR_PN]));
> + print_0xhex(PRINT_ANY, "ssci",
> + "SSCI %08x",
> + ntohl(rta_getattr_u32(sa_attr[MACSEC_SA_ATTR_SSCI])));
> + }
>
> print_bool(PRINT_JSON, "active", NULL, state);
> print_string(PRINT_FP, NULL, " state %s,",
[...]
> @@ -989,6 +1062,8 @@ static int process(struct nlmsghdr *n, void *arg)
> int ifindex;
> __u64 sci;
> __u8 encoding_sa;
> + __u64 cid;
> + bool is_xpn = false;
>
> if (n->nlmsg_type != genl_family)
> return -1;
> @@ -1032,13 +1107,16 @@ static int process(struct nlmsghdr *n, void *arg)
>
> print_attrs(attrs_secy);
>
> - print_tx_sc(" ", sci, encoding_sa,
> + cid = rta_getattr_u64(attrs_secy[MACSEC_SECY_ATTR_CIPHER_SUITE]);
> + if (cid == MACSEC_CIPHER_ID_GCM_AES_XPN_128 || cid == MACSEC_CIPHER_ID_GCM_AES_XPN_256)
I'd stuff that in a ciphersuite_is_xpn(cid) helper, and then it's just
is_xpn = ciphersuite_is_xpn(cid);
> + is_xpn = true;
> + print_tx_sc(" ", sci, encoding_sa, is_xpn,
> attrs[MACSEC_ATTR_TXSC_STATS],
> attrs[MACSEC_ATTR_SECY_STATS],
> attrs[MACSEC_ATTR_TXSA_LIST]);
[...]
> @@ -1268,7 +1347,7 @@ static int macsec_parse_opt(struct link_util *lu, int argc, char **argv,
> enum macsec_offload offload;
> struct cipher_args cipher = {0};
> enum macsec_validation_type validate;
> - bool es = false, scb = false, send_sci = false;
> + bool es = false, scb = false, send_sci = false, xpn = false;
> int replay_protect = -1;
> struct sci sci = { 0 };
>
> @@ -1388,6 +1467,11 @@ static int macsec_parse_opt(struct link_util *lu, int argc, char **argv,
> return ret;
> addattr8(n, MACSEC_BUFLEN,
> IFLA_MACSEC_OFFLOAD, offload);
> + } else if (strcmp(*argv, "xpn") == 0) {
> + NEXT_ARG();
> + xpn = parse_on_off("xpn", *argv, &ret);
I'm not convinced the "xpn on/off" flag is the best API here. How
about just letting the admin pass the full cipher suite name
(GCM-AES-XPN-128 or GCM-AES-XPN-256)?
This xpn flag is not consistent with the dump side (which uses the
cipher suite only), and it's not consistent with the kernel API, which
also doesn't use an XPN flag but the cipher suite ID.
So I'd just add 2 options to "cipher" argument parsing (and extract
that to a new cs_name_to_id helper, since we'd now have 4 options),
and get rid of the "xpn on/off" option (and the cipher.id flip to XPN
below).
> + if (ret != 0)
> + return ret;
> } else {
> fprintf(stderr, "macsec: unknown command \"%s\"?\n",
> *argv);
> @@ -1415,6 +1499,13 @@ static int macsec_parse_opt(struct link_util *lu, int argc, char **argv,
> return -1;
> }
>
> + if (xpn) {
> + if (cipher.id == MACSEC_CIPHER_ID_GCM_AES_256)
> + cipher.id = MACSEC_CIPHER_ID_GCM_AES_XPN_256;
> + else
> + cipher.id = MACSEC_CIPHER_ID_GCM_AES_XPN_128;
> + }
> +
> if (cipher.id)
> addattr_l(n, MACSEC_BUFLEN, IFLA_MACSEC_CIPHER_SUITE,
> &cipher.id, sizeof(cipher.id));
> --
> 2.21.3
>
--
Sabrina
^ permalink raw reply [flat|nested] 4+ messages in thread
* RE: [PATCH main v3 1/2] macsec: add Extended Packet Number support
2022-09-04 15:44 ` [PATCH main v3 1/2] macsec: add Extended Packet Number support Sabrina Dubroca
@ 2022-09-07 14:03 ` Emeel Hakim
0 siblings, 0 replies; 4+ messages in thread
From: Emeel Hakim @ 2022-09-07 14:03 UTC (permalink / raw)
To: Sabrina Dubroca; +Cc: dsahern, netdev, Raed Salem, Tariq Toukan
> -----Original Message-----
> From: Sabrina Dubroca <sd@queasysnail.net>
> Sent: Sunday, 4 September 2022 18:44
> To: Emeel Hakim <ehakim@nvidia.com>
> Cc: dsahern@kernel.org; netdev@vger.kernel.org; Raed Salem
> <raeds@nvidia.com>; Tariq Toukan <tariqt@nvidia.com>
> Subject: Re: [PATCH main v3 1/2] macsec: add Extended Packet Number support
>
> External email: Use caution opening links or attachments
>
>
> 2022-09-04, 10:47:28 +0300, Emeel Hakim wrote:
> > @@ -174,14 +186,36 @@ static int parse_sa_args(int *argcp, char
> > ***argvp, struct sa_desc *sa)
> >
> > while (argc > 0) {
> > if (strcmp(*argv, "pn") == 0) {
> > - if (sa->pn != 0)
> > + if (sa->pn.pn64 != 0)
> > duparg2("pn", "pn");
> > NEXT_ARG();
> > - ret = get_u32(&sa->pn, *argv, 0);
> > + ret = get_u32(&sa->pn.pn32, *argv, 0);
> > + if (ret)
> > + invarg("expected pn", *argv);
> > + if (sa->pn.pn32 == 0)
> > + invarg("expected pn != 0", *argv);
> > + } else if (strcmp(*argv, "xpn") == 0) {
> > + if (sa->pn.pn64 != 0)
> > + duparg2("xpn", "xpn");
> > + NEXT_ARG();
> > + ret = get_u64(&sa->pn.pn64, *argv, 0);
> > if (ret)
> > invarg("expected pn", *argv);
> > - if (sa->pn == 0)
> > + if (sa->pn.pn64 == 0)
> > invarg("expected pn != 0", *argv);
> > + sa->xpn = true;
> > + } else if (strcmp(*argv, "salt") == 0) {
> > + unsigned int len;
> > +
> > + NEXT_ARG();
>
> That should have a duparg check.
>
ack
> > + if (!hexstring_a2n(*argv, sa->salt, MACSEC_SALT_LEN,
> > + &len))
> > + invarg("expected salt", *argv);
> > + } else if (strcmp(*argv, "ssci") == 0) {
> > + NEXT_ARG();
>
> Also worth a duparg check.
>
ack
> > + ret = get_ssci(&sa->ssci, *argv);
> > + if (ret)
> > + invarg("expected ssci", *argv);
> > } else if (strcmp(*argv, "key") == 0) {
> > unsigned int len;
> >
> > @@ -392,9 +426,22 @@ static int do_modify_nl(enum cmd c, enum
> macsec_nl_commands cmd, int ifindex,
> > addattr8(&req.n, MACSEC_BUFLEN, MACSEC_SA_ATTR_AN, sa->an);
> >
> > if (c != CMD_DEL) {
> > - if (sa->pn)
> > + if (sa->xpn) {
> > + if (sa->pn.pn64)
> > + addattr64(&req.n, MACSEC_BUFLEN, MACSEC_SA_ATTR_PN,
> > + sa->pn.pn64);
> > + if (sa->salt[0] != '\0')
>
> Does the specification say the salt can't start with a 0 byte, or is it just a "salt was
> set" test? If that's coming from the spec, the kernel should also check this.
>
it is just a "salt was set" test
> > + addattr_l(&req.n, MACSEC_BUFLEN, MACSEC_SA_ATTR_SALT,
> > + sa->salt, MACSEC_SALT_LEN);
> > + if (sa->ssci != 0)
>
> Same question as for the salt.
>
it is just a "ssci was set" test
> For both, I'd just add a salt_set/ssci_set flag to sa_desc, and use it for duparg as
> well.
>
> > + addattr32(&req.n, MACSEC_BUFLEN, MACSEC_SA_ATTR_SSCI,
> > + sa->ssci);
> > + }
> > +
> > + if (sa->pn.pn32 && !sa->xpn) {
>
> Nit: combine this with the previous if:
>
> } else if (sa->pn.pn32) {
>
>
> > addattr32(&req.n, MACSEC_BUFLEN, MACSEC_SA_ATTR_PN,
> > - sa->pn);
> > + sa->pn.pn32);
> > + }
> >
> > if (sa->key_len) {
> > addattr_l(&req.n, MACSEC_BUFLEN,
> > MACSEC_SA_ATTR_KEYID, @@ -426,10 +473,11 @@ static bool
> check_sa_args(enum cmd c, struct sa_desc *sa)
> > return -1;
> > }
> >
> > - if (sa->pn == 0) {
> > + if (sa->pn.pn64 == 0) {
> > fprintf(stderr, "must specify a packet number != 0\n");
> > return -1;
> > }
> > +
>
> Remove that blank line.
ack
> > } else if (c == CMD_UPD) {
> > if (sa->key_len) {
> > fprintf(stderr, "cannot change key on SA\n");
>
> [...]
> > @@ -875,8 +930,16 @@ static void print_tx_sc(const char *prefix, __u64 sci,
> __u8 encoding_sa,
> > print_string(PRINT_FP, NULL, "%s", prefix);
> > print_uint(PRINT_ANY, "an", "%d:",
> > rta_getattr_u8(sa_attr[MACSEC_SA_ATTR_AN]));
> > - print_uint(PRINT_ANY, "pn", " PN %u,",
> > - rta_getattr_u32(sa_attr[MACSEC_SA_ATTR_PN]));
> > + if (!is_xpn) {
>
> Nit: I'd flip those branches so that the test is "if (is_xpn)"
>
ack
> > + print_uint(PRINT_ANY, "pn", " PN %u,",
> > + rta_getattr_u32(sa_attr[MACSEC_SA_ATTR_PN]));
> > + } else {
> > + print_uint(PRINT_ANY, "pn", " PN %u,",
> > + rta_getattr_u64(sa_attr[MACSEC_SA_ATTR_PN]));
> > + print_0xhex(PRINT_ANY, "ssci",
> > + "SSCI %08x",
> > + ntohl(rta_getattr_u32(sa_attr[MACSEC_SA_ATTR_SSCI])));
> > + }
> >
> > print_bool(PRINT_JSON, "active", NULL, state);
> > print_string(PRINT_FP, NULL,
>
> [...]
> > @@ -943,8 +1007,16 @@ static void print_rx_sc(const char *prefix, __be64 sci,
> __u8 active,
> > print_string(PRINT_FP, NULL, "%s", prefix);
> > print_uint(PRINT_ANY, "an", "%u:",
> > rta_getattr_u8(sa_attr[MACSEC_SA_ATTR_AN]));
> > - print_uint(PRINT_ANY, "pn", " PN %u,",
> > - rta_getattr_u32(sa_attr[MACSEC_SA_ATTR_PN]));
> > + if (!is_xpn) {
>
> Same nit, flip the test.
ack
> > + print_uint(PRINT_ANY, "pn", " PN %u,",
> > + rta_getattr_u32(sa_attr[MACSEC_SA_ATTR_PN]));
> > + } else {
> > + print_uint(PRINT_ANY, "pn", " PN %u,",
> > + rta_getattr_u64(sa_attr[MACSEC_SA_ATTR_PN]));
> > + print_0xhex(PRINT_ANY, "ssci",
> > + "SSCI %08x",
> > + ntohl(rta_getattr_u32(sa_attr[MACSEC_SA_ATTR_SSCI])));
> > + }
> >
> > print_bool(PRINT_JSON, "active", NULL, state);
> > print_string(PRINT_FP, NULL, " state %s,",
>
> [...]
> > @@ -989,6 +1062,8 @@ static int process(struct nlmsghdr *n, void *arg)
> > int ifindex;
> > __u64 sci;
> > __u8 encoding_sa;
> > + __u64 cid;
> > + bool is_xpn = false;
> >
> > if (n->nlmsg_type != genl_family)
> > return -1;
> > @@ -1032,13 +1107,16 @@ static int process(struct nlmsghdr *n, void
> > *arg)
> >
> > print_attrs(attrs_secy);
> >
> > - print_tx_sc(" ", sci, encoding_sa,
> > + cid = rta_getattr_u64(attrs_secy[MACSEC_SECY_ATTR_CIPHER_SUITE]);
> > + if (cid == MACSEC_CIPHER_ID_GCM_AES_XPN_128 || cid ==
> > + MACSEC_CIPHER_ID_GCM_AES_XPN_256)
>
> I'd stuff that in a ciphersuite_is_xpn(cid) helper, and then it's just
> is_xpn = ciphersuite_is_xpn(cid);
ack
> > + is_xpn = true;
> > + print_tx_sc(" ", sci, encoding_sa, is_xpn,
> > attrs[MACSEC_ATTR_TXSC_STATS],
> > attrs[MACSEC_ATTR_SECY_STATS],
> > attrs[MACSEC_ATTR_TXSA_LIST]);
>
> [...]
> > @@ -1268,7 +1347,7 @@ static int macsec_parse_opt(struct link_util *lu, int
> argc, char **argv,
> > enum macsec_offload offload;
> > struct cipher_args cipher = {0};
> > enum macsec_validation_type validate;
> > - bool es = false, scb = false, send_sci = false;
> > + bool es = false, scb = false, send_sci = false, xpn = false;
> > int replay_protect = -1;
> > struct sci sci = { 0 };
> >
> > @@ -1388,6 +1467,11 @@ static int macsec_parse_opt(struct link_util *lu, int
> argc, char **argv,
> > return ret;
> > addattr8(n, MACSEC_BUFLEN,
> > IFLA_MACSEC_OFFLOAD, offload);
> > + } else if (strcmp(*argv, "xpn") == 0) {
> > + NEXT_ARG();
> > + xpn = parse_on_off("xpn", *argv, &ret);
>
> I'm not convinced the "xpn on/off" flag is the best API here. How about just
> letting the admin pass the full cipher suite name
> (GCM-AES-XPN-128 or GCM-AES-XPN-256)?
>
> This xpn flag is not consistent with the dump side (which uses the cipher suite
> only), and it's not consistent with the kernel API, which also doesn't use an XPN
> flag but the cipher suite ID.
>
> So I'd just add 2 options to "cipher" argument parsing (and extract that to a new
> cs_name_to_id helper, since we'd now have 4 options), and get rid of the "xpn
> on/off" option (and the cipher.id flip to XPN below).
>
I agree, I will send a V4 with the cipher approach instead of the xpn "on/off"
> > + if (ret != 0)
> > + return ret;
> > } else {
> > fprintf(stderr, "macsec: unknown command \"%s\"?\n",
> > *argv);
> > @@ -1415,6 +1499,13 @@ static int macsec_parse_opt(struct link_util *lu, int
> argc, char **argv,
> > return -1;
> > }
> >
> > + if (xpn) {
> > + if (cipher.id == MACSEC_CIPHER_ID_GCM_AES_256)
> > + cipher.id = MACSEC_CIPHER_ID_GCM_AES_XPN_256;
> > + else
> > + cipher.id = MACSEC_CIPHER_ID_GCM_AES_XPN_128;
> > + }
> > +
> > if (cipher.id)
> > addattr_l(n, MACSEC_BUFLEN, IFLA_MACSEC_CIPHER_SUITE,
> > &cipher.id, sizeof(cipher.id));
> > --
> > 2.21.3
> >
>
> --
> Sabrina
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-09-07 14:03 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-04 7:47 [PATCH main v3 1/2] macsec: add Extended Packet Number support Emeel Hakim
2022-09-04 7:47 ` [PATCH main v3 2/2] macsec: add user manual description for extended packet number feature Emeel Hakim
2022-09-04 15:44 ` [PATCH main v3 1/2] macsec: add Extended Packet Number support Sabrina Dubroca
2022-09-07 14:03 ` Emeel Hakim
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.