| 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
| ||
|
Message-ID: <IA1PR12MB635394958A10D3A17513086CAB419@IA1PR12MB6353.namprd12.prod.outlook.com>
Date: Wed, 7 Sep 2022 14:03:15 +0000
From: Emeel Hakim <ehakim@...dia.com>
To: Sabrina Dubroca <sd@...asysnail.net>
CC: "dsahern@...nel.org" <dsahern@...nel.org>,
"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
Raed Salem <raeds@...dia.com>, Tariq Toukan <tariqt@...dia.com>
Subject: RE: [PATCH main v3 1/2] macsec: add Extended Packet Number support
> -----Original Message-----
> From: Sabrina Dubroca <sd@...asysnail.net>
> Sent: Sunday, 4 September 2022 18:44
> To: Emeel Hakim <ehakim@...dia.com>
> Cc: dsahern@...nel.org; netdev@...r.kernel.org; Raed Salem
> <raeds@...dia.com>; Tariq Toukan <tariqt@...dia.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
Powered by blists - more mailing lists