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

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

[ehakim]

From: Emeel Hakim <ehakim@nvidia.com>

Currently when configuring macsec with replay protection,
replay protection and window gets a default value of -1,
the above is leading to passing replay protection and
replay window attributes to the kernel while replay is
explicitly set to off, leading for an invalid argument
error when configured with extended packet number (XPN).
since the default window value which is 0xFFFFFFFF is
passed to the kernel and while XPN is configured the above
value is an invalid window value.

Example:
ip link add link eth2 macsec0 type macsec sci 1 cipher
gcm-aes-xpn-128 replay off

RTNETLINK answers: Invalid argument

Fix by using a boolean variable for replay protect with a default value
of false.

Fixes: b26fc590ce62 ("ip: add MACsec support")
Signed-off-by: Emeel Hakim <ehakim@nvidia.com>
Reviewed-by: Raed Salem <raeds@nvidia.com>
---
 ip/ipmacsec.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

diff --git a/ip/ipmacsec.c b/ip/ipmacsec.c
index 6dd73827..e2809fd6 100644
--- a/ip/ipmacsec.c
+++ b/ip/ipmacsec.c
@@ -1356,8 +1356,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;
-	int replay_protect = -1;
+	bool es = false, scb = false, send_sci = false, replay_protect = false;
 	struct sci sci = { 0 };
 
 	ret = get_sci_portaddr(&sci, &argc, &argv, true, true);
@@ -1453,7 +1452,7 @@ static int macsec_parse_opt(struct link_util *lu, int argc, char **argv,
 			i = parse_on_off("replay", *argv, &ret);
 			if (ret != 0)
 				return ret;
-			replay_protect = !!i;
+			replay_protect = i;
 		} else if (strcmp(*argv, "window") == 0) {
 			NEXT_ARG();
 			ret = get_u32(&window, *argv, 0);
@@ -1498,12 +1497,12 @@ static int macsec_parse_opt(struct link_util *lu, int argc, char **argv,
 		return -1;
 	}
 
-	if (window != -1 && replay_protect == -1) {
+	if (window != -1 && !replay_protect) {
 		fprintf(stderr,
 			"replay window set, but replay protection not enabled. did you mean 'replay on window %u'?\n",
 			window);
 		return -1;
-	} else if (window == -1 && replay_protect == 1) {
+	} else if (window == -1 && replay_protect) {
 		fprintf(stderr,
 			"replay protection enabled, but no window set. did you mean 'replay on window VALUE'?\n");
 		return -1;
@@ -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) {
 		addattr32(n, MACSEC_BUFLEN, IFLA_MACSEC_WINDOW, window);
 		addattr8(n, MACSEC_BUFLEN, IFLA_MACSEC_REPLAY_PROTECT,
 			 replay_protect);
-- 
2.21.3

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

[Sabrina Dubroca]

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:

    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?


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

-- 
Sabrina

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

[Emeel Hakim]

> -----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?

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

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

[Sabrina Dubroca]

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

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

[Emeel Hakim]

> -----Original Message-----
> From: Sabrina Dubroca <sd@queasysnail.net>
> Sent: Tuesday, 10 January 2023 14:03
> 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, 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.

Ack, the initial proposed fix is good enough since we can't re-set replay to on without
providing a window, we will fall on the test:

else if (window == -1 && replay_protect == 1) { 
                fprintf(stderr,
                        "replay protection enabled, but no window set. did you mean 'replay on window VALUE'?\n");
                return -1;
}

I will send a V2 with the proposed fix.

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