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] [day] [month] [year] [list]

Message-ID: <3b367f43-7081-18d6-128a-6ad8998099b3@kernel.org>
Date:   Thu, 1 Sep 2022 08:11:38 -0600
From:   David Ahern <dsahern@...nel.org>
To:     Emeel Hakim <ehakim@...dia.com>,
        "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

On 9/1/22 6:54 AM, Emeel Hakim wrote:
>> -----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.

you are checking for duplicate pn argument. My comment is that if you
check sa->pn.pn64 you can check for a duplicate xpn and pn where a
command line has an xpn argument with the upper 32bits set folllowed by
a pn argument.

> 
>>
>>>                               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");

this check right here but above for the "pn" case.


>>> +                     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);