netdev - RE: [PATCH main v2 1/2] macsec: add extended packet number (XPN) support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <IA1PR12MB6353F9B26B5141F5F6F1EC82AB7B9@IA1PR12MB6353.namprd12.prod.outlook.com>
Date:   Thu, 1 Sep 2022 12:54:51 +0000
From:   Emeel Hakim <ehakim@...dia.com>
To:     David Ahern <dsahern@...nel.org>,
        "sd@...asysnail.net" <sd@...asysnail.net>
CC:     Tariq Toukan <tariqt@...dia.com>, Raed Salem <raeds@...dia.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: RE: [PATCH main v2 1/2] macsec: add extended packet number (XPN)
 support

> -----Original Message-----
> From: David Ahern <dsahern@...nel.org>
> Sent: Thursday, 1 September 2022 5:53
> To: Emeel Hakim <ehakim@...dia.com>; sd@...asysnail.net
> Cc: Tariq Toukan <tariqt@...dia.com>; Raed Salem <raeds@...dia.com>;
> netdev@...r.kernel.org
> Subject: Re: [PATCH main v2 1/2] macsec: add extended packet number (XPN)
> support
> 
> 
> 
> On 8/24/22 3:17 AM, Emeel Hakim wrote:
> > @@ -174,14 +181,34 @@ 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.pn32 != 0)
> 
> pn64 to cover the entire range? ie., pn and xpn on the same command line.

Didn’t really get the comment if to have "pn" as the only parameter in the command line or just to save all the packet numbers in a 64-bit variable and preserve the current API , can you please elaborate?
please notice that kernel has a check for the pn length , it expects 32 bits in case of none extended packet number (xpn), hence passing 64-bit in case of none xpn will fail.  Xpn is a secure channel property where pn is an SA property so during the parsing of the SA command line (which includes the pn) I don’t have a legit way to distinguish between xpn or none xpn cases hence the separation.

> 
> >                               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 == 0)
> > +                     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.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_u32(&sa->ssci, *argv, 0);
> 
> that can fail, so check ret and throw an error message

Ack

> 
> >               } else if (strcmp(*argv, "key") == 0) {
> >                       unsigned int len;
> >
> 
> ...
> 
> 
> > @@ -1388,6 +1458,14 @@ 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();
> > +                     int i;
> > +
> > +                     i = parse_on_off("xpn", *argv, &ret);
> 
> drop the 'i' and just
> xpn = parse_on_off("xpn", *argv, &ret);
> 
> besides you have i as an int when xpn is bool and parse_on_off returns a bool.

Ack