netdev - Re: [PATCH 2/2] macsec: Netlink support of XPN cipher suites (IEEE 802.1AEbw)

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

Message-ID: <CAMdQvKuzaBuKGj1-HR6+r=FY4X9GhZPvEHwRt3BjErFiu1+bgw@mail.gmail.com>
Date:   Thu, 5 Mar 2020 23:53:29 +0000
From:   Era Mayflower <mayflowerera@...il.com>
To:     Michal Kubecek <mkubecek@...e.cz>
Cc:     netdev@...r.kernel.org, davem@...emloft.net,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] macsec: Netlink support of XPN cipher suites (IEEE 802.1AEbw)

Do you think that inserting those new enum values after *_PAD would be
a good solution?


On Thu, Mar 5, 2020 at 11:51 PM Era Mayflower <mayflowerera@...il.com> wrote:
>
> Do you think that inserting those new enum values after *_PAD would be a good solution?
>
> On Thu, Mar 5, 2020 at 2:02 PM Michal Kubecek <mkubecek@...e.cz> wrote:
>>
>> On Thu, Mar 05, 2020 at 10:01:08PM +0000, Era Mayflower wrote:
>> > Netlink support of extended packet number cipher suites,
>> > allows adding and updating XPN macsec interfaces.
>> >
>> > Added support in:
>> >     * Creating interfaces with GCM-AES-XPN-128 and GCM-AES-XPN-256.
>> >     * Setting and getting packet numbers with 64bit of SAs.
>> >     * Settings and getting ssci of SCs.
>> >     * Settings and getting salt of SecYs.
>> >
>> > Depends on: macsec: Support XPN frame handling - IEEE 802.1AEbw.
>> >
>> > Signed-off-by: Era Mayflower <mayflowerera@...il.com>
>> > ---
>> [...]
>> > diff --git a/include/net/macsec.h b/include/net/macsec.h
>> > index a0b1d0b5c..3c7914ff1 100644
>> > --- a/include/net/macsec.h
>> > +++ b/include/net/macsec.h
>> > @@ -11,6 +11,9 @@
>> >  #include <uapi/linux/if_link.h>
>> >  #include <uapi/linux/if_macsec.h>
>> >
>> > +#define MACSEC_DEFAULT_PN_LEN 4
>> > +#define MACSEC_XPN_PN_LEN 8
>> > +
>> >  #define MACSEC_SALT_LEN 12
>> >
>> >  typedef u64 __bitwise sci_t;
>> > diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
>> > index 024af2d1d..ee424d915 100644
>> > --- a/include/uapi/linux/if_link.h
>> > +++ b/include/uapi/linux/if_link.h
>> > @@ -462,6 +462,8 @@ enum {
>> >       IFLA_MACSEC_SCB,
>> >       IFLA_MACSEC_REPLAY_PROTECT,
>> >       IFLA_MACSEC_VALIDATION,
>> > +     IFLA_MACSEC_SSCI,
>> > +     IFLA_MACSEC_SALT,
>> >       IFLA_MACSEC_PAD,
>> >       __IFLA_MACSEC_MAX,
>> >  };
>>
>> Doesn't this break backword compatibility? You change the value of
>> IFLA_MACSEC_PAD; even if it's only used as padding, if an old client
>> uses it, new kernel will interpret it as IFLA_MACSEC_SSCI (an the same
>> holds for new client with old kernel).
>>
>> > diff --git a/include/uapi/linux/if_macsec.h b/include/uapi/linux/if_macsec.h
>> > index 1d63c43c3..c8fab9673 100644
>> > --- a/include/uapi/linux/if_macsec.h
>> > +++ b/include/uapi/linux/if_macsec.h
>> > @@ -25,6 +25,8 @@
>> >  /* cipher IDs as per IEEE802.1AEbn-2011 */
>> >  #define MACSEC_CIPHER_ID_GCM_AES_128 0x0080C20001000001ULL
>> >  #define MACSEC_CIPHER_ID_GCM_AES_256 0x0080C20001000002ULL
>> > +#define MACSEC_CIPHER_ID_GCM_AES_XPN_128 0x0080C20001000003ULL
>> > +#define MACSEC_CIPHER_ID_GCM_AES_XPN_256 0x0080C20001000004ULL
>> >
>> >  /* deprecated cipher ID for GCM-AES-128 */
>> >  #define MACSEC_DEFAULT_CIPHER_ID     0x0080020001000001ULL
>> > @@ -66,6 +68,8 @@ enum macsec_secy_attrs {
>> >       MACSEC_SECY_ATTR_INC_SCI,
>> >       MACSEC_SECY_ATTR_ES,
>> >       MACSEC_SECY_ATTR_SCB,
>> > +     MACSEC_SECY_ATTR_SSCI,
>> > +     MACSEC_SECY_ATTR_SALT,
>> >       MACSEC_SECY_ATTR_PAD,
>> >       __MACSEC_SECY_ATTR_END,
>> >       NUM_MACSEC_SECY_ATTR = __MACSEC_SECY_ATTR_END,
>> > @@ -78,6 +82,7 @@ enum macsec_rxsc_attrs {
>> >       MACSEC_RXSC_ATTR_ACTIVE,  /* config/dump, u8 0..1 */
>> >       MACSEC_RXSC_ATTR_SA_LIST, /* dump, nested */
>> >       MACSEC_RXSC_ATTR_STATS,   /* dump, nested, macsec_rxsc_stats_attr */
>> > +     MACSEC_RXSC_ATTR_SSCI,    /* config/dump, u32 */
>> >       MACSEC_RXSC_ATTR_PAD,
>> >       __MACSEC_RXSC_ATTR_END,
>> >       NUM_MACSEC_RXSC_ATTR = __MACSEC_RXSC_ATTR_END,
>>
>> The same problem with these two.
>>
>> I'm also a bit unsure about the change of type and length of
>> MACSEC_SA_ATTR_PN but I would have to get more familiar with the code to
>> see if it is really a problem.
>>
>> Michal