Re: [PATCH main 1/1] macsec: Fix Macsec replay protection

[Sabrina Dubroca <sd@queasysnail.net>]

2023-01-10, 11:23:26 +0000, Emeel Hakim wrote:
> 
> 
> > -----Original Message-----
> > From: Sabrina Dubroca <sd@queasysnail.net>
> > Sent: Tuesday, 10 January 2023 12:02
> > To: Emeel Hakim <ehakim@nvidia.com>
> > Cc: dsahern@kernel.org; netdev@vger.kernel.org; Raed Salem
> > <raeds@nvidia.com>
> > Subject: Re: [PATCH main 1/1] macsec: Fix Macsec replay protection
> > 
> > External email: Use caution opening links or attachments
> > 
> > 
> > 2023-01-10, 10:02:19 +0200, ehakim@nvidia.com wrote:
> > > @@ -1516,7 +1515,7 @@ static int macsec_parse_opt(struct link_util *lu, int
> > argc, char **argv,
> > >               addattr_l(n, MACSEC_BUFLEN, IFLA_MACSEC_ICV_LEN,
> > >                         &cipher.icv_len, sizeof(cipher.icv_len));
> > >
> > > -     if (replay_protect != -1) {
> > > +     if (replay_protect) {
> > 
> > This will silently break disabling replay protection on an existing device. This:
> >
> 
> Thanks for catching that.
> 
> >     ip link set macsec0 type macsec replay off
> > 
> > would now appear to succeed but will not do anything. That's why I used an int with
> > -1 in iproute, and a U8 netlink attribute rather a flag.
> > 
> > I think this would be a better fix:
> > 
> >         if (replay_protect != -1) {
> > -               addattr32(n, MACSEC_BUFLEN, IFLA_MACSEC_WINDOW, window);
> > +               if (replay_protect)
> > +                       addattr32(n, MACSEC_BUFLEN, IFLA_MACSEC_WINDOW,
> > + window);
> >                 addattr8(n, MACSEC_BUFLEN, IFLA_MACSEC_REPLAY_PROTECT,
> >                          replay_protect);
> >         }
> > 
> > Does that work for all your test cases?
> 
> The main test case works however I wonder if it should be allowed to pass a window with replay off
> for example:
> ip link set macsec0 type macsec replay off window 32 
> 
> because now this will silently ignore the window attribute
> 
> a possible scenario:
> we start with a macsec device with replay enabled and window set to 64
> now we perform:
> ip link set macsec0 type macsec replay off window 32
> ip link set macsec0 type macsec replay on
> 
> we expect to move to a 32-bit window but we silently failed to do so.
> 
> what do you think?

The kernel currently doesn't allow that. From macsec_validate_attr:

	if ((data[IFLA_MACSEC_REPLAY_PROTECT] &&
	     nla_get_u8(data[IFLA_MACSEC_REPLAY_PROTECT])) &&
	    !data[IFLA_MACSEC_WINDOW])
		return -EINVAL;

So we can set the size of the replay window, but it's ignored and will
be overwritten when we enable replay protection.

We could check for window != -1 instead of replay_protect before
adding IFLA_MACSEC_WINDOW, and I think that should take care of both
cases.

> 
> > 
> > >               addattr32(n, MACSEC_BUFLEN, IFLA_MACSEC_WINDOW, window);
> > >               addattr8(n, MACSEC_BUFLEN, IFLA_MACSEC_REPLAY_PROTECT,
> > >                        replay_protect);
> > 
> > --
> > Sabrina
> 

-- 
Sabrina