[PATCH 1/2] xfrm: interface with if_id 0 should return error

[PATCH 1/2] xfrm: interface with if_id 0 should return error

[Antony Antony]

xfrm interface if_id = 0 would cause xfrm policy lookup errors since
commit 9f8550e4bd9d ("xfrm: fix disable_xfrm sysctl when used on xfrm interfaces")

Now fail to create an xfrm interface when if_id = 0

With this commit:
 ip link add ipsec0  type xfrm dev lo  if_id 0
 Error: if_id must be non zero.

Signed-off-by: Antony Antony <antony.antony@secunet.com>
---
 net/xfrm/xfrm_interface.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/net/xfrm/xfrm_interface.c b/net/xfrm/xfrm_interface.c
index 41de46b5ffa9..57448fc519fc 100644
--- a/net/xfrm/xfrm_interface.c
+++ b/net/xfrm/xfrm_interface.c
@@ -637,11 +637,16 @@ static int xfrmi_newlink(struct net *src_net, struct net_device *dev,
 			struct netlink_ext_ack *extack)
 {
 	struct net *net = dev_net(dev);
-	struct xfrm_if_parms p;
+	struct xfrm_if_parms p = {};
 	struct xfrm_if *xi;
 	int err;
 
 	xfrmi_netlink_parms(data, &p);
+	if (!p.if_id) {
+		NL_SET_ERR_MSG(extack, "if_id must be non zero");
+		return -EINVAL;
+	}
+
 	xi = xfrmi_locate(net, &p);
 	if (xi)
 		return -EEXIST;
@@ -666,7 +671,12 @@ static int xfrmi_changelink(struct net_device *dev, struct nlattr *tb[],
 {
 	struct xfrm_if *xi = netdev_priv(dev);
 	struct net *net = xi->net;
-	struct xfrm_if_parms p;
+	struct xfrm_if_parms p = {};
+
+	if (!p.if_id) {
+		NL_SET_ERR_MSG(extack, "if_id must be non zero");
+		return -EINVAL;
+	}
 
 	xfrmi_netlink_parms(data, &p);
 	xi = xfrmi_locate(net, &p);
-- 
2.30.2

Re: [PATCH 1/2] xfrm: interface with if_id 0 should return error

[Eyal Birger]

On Thu, Dec 9, 2021 at 5:36 PM Antony Antony <antony.antony@secunet.com> wrote:
>
> xfrm interface if_id = 0 would cause xfrm policy lookup errors since
> commit 9f8550e4bd9d ("xfrm: fix disable_xfrm sysctl when used on xfrm interfaces")
>
> Now fail to create an xfrm interface when if_id = 0
>
> With this commit:
>  ip link add ipsec0  type xfrm dev lo  if_id 0
>  Error: if_id must be non zero.
>
> Signed-off-by: Antony Antony <antony.antony@secunet.com>
> ---
>  net/xfrm/xfrm_interface.c | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/net/xfrm/xfrm_interface.c b/net/xfrm/xfrm_interface.c
> index 41de46b5ffa9..57448fc519fc 100644
> --- a/net/xfrm/xfrm_interface.c
> +++ b/net/xfrm/xfrm_interface.c
> @@ -637,11 +637,16 @@ static int xfrmi_newlink(struct net *src_net, struct net_device *dev,
>                         struct netlink_ext_ack *extack)
>  {
>         struct net *net = dev_net(dev);
> -       struct xfrm_if_parms p;
> +       struct xfrm_if_parms p = {};
>         struct xfrm_if *xi;
>         int err;
>
>         xfrmi_netlink_parms(data, &p);
> +       if (!p.if_id) {
> +               NL_SET_ERR_MSG(extack, "if_id must be non zero");
> +               return -EINVAL;
> +       }
> +
>         xi = xfrmi_locate(net, &p);
>         if (xi)
>                 return -EEXIST;
> @@ -666,7 +671,12 @@ static int xfrmi_changelink(struct net_device *dev, struct nlattr *tb[],
>  {
>         struct xfrm_if *xi = netdev_priv(dev);
>         struct net *net = xi->net;
> -       struct xfrm_if_parms p;
> +       struct xfrm_if_parms p = {};
> +
> +       if (!p.if_id) {
> +               NL_SET_ERR_MSG(extack, "if_id must be non zero");
> +               return -EINVAL;
> +       }
>
>         xfrmi_netlink_parms(data, &p);
>         xi = xfrmi_locate(net, &p);

Looks good. Maybe this needs a "Fixes:" tag?

Reviewed-by: Eyal Birger <eyal.birger@gmail.com>

Thanks,
Eyal.

> --
> 2.30.2
>

Re: [PATCH 1/2] xfrm: interface with if_id 0 should return error

[Antony Antony]

On Fri, Dec 10, 2021 at 19:22:35 +0200, Eyal Birger wrote:
> On Thu, Dec 9, 2021 at 5:36 PM Antony Antony <antony.antony@secunet.com> wrote:
> >
> > xfrm interface if_id = 0 would cause xfrm policy lookup errors since
> > commit 9f8550e4bd9d ("xfrm: fix disable_xfrm sysctl when used on xfrm interfaces")
> >
> > Now fail to create an xfrm interface when if_id = 0
> >
> > With this commit:
> >  ip link add ipsec0  type xfrm dev lo  if_id 0
> >  Error: if_id must be non zero.
> >
> > Signed-off-by: Antony Antony <antony.antony@secunet.com>
> > ---
> >  net/xfrm/xfrm_interface.c | 14 ++++++++++++--
> >  1 file changed, 12 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/xfrm/xfrm_interface.c b/net/xfrm/xfrm_interface.c
> > index 41de46b5ffa9..57448fc519fc 100644
> > --- a/net/xfrm/xfrm_interface.c
> > +++ b/net/xfrm/xfrm_interface.c
> > @@ -637,11 +637,16 @@ static int xfrmi_newlink(struct net *src_net, struct net_device *dev,
> >                         struct netlink_ext_ack *extack)
> >  {
> >         struct net *net = dev_net(dev);
> > -       struct xfrm_if_parms p;
> > +       struct xfrm_if_parms p = {};
> >         struct xfrm_if *xi;
> >         int err;
> >
> >         xfrmi_netlink_parms(data, &p);
> > +       if (!p.if_id) {
> > +               NL_SET_ERR_MSG(extack, "if_id must be non zero");
> > +               return -EINVAL;
> > +       }
> > +
> >         xi = xfrmi_locate(net, &p);
> >         if (xi)
> >                 return -EEXIST;
> > @@ -666,7 +671,12 @@ static int xfrmi_changelink(struct net_device *dev, struct nlattr *tb[],
> >  {
> >         struct xfrm_if *xi = netdev_priv(dev);
> >         struct net *net = xi->net;
> > -       struct xfrm_if_parms p;
> > +       struct xfrm_if_parms p = {};
> > +
> > +       if (!p.if_id) {
> > +               NL_SET_ERR_MSG(extack, "if_id must be non zero");
> > +               return -EINVAL;
> > +       }
> >
> >         xfrmi_netlink_parms(data, &p);
> >         xi = xfrmi_locate(net, &p);
> 
> Looks good. Maybe this needs a "Fixes:" tag?

I assumed this patch is not ideal for stable releases!
There is a small chance someone was depending old semi broken behavior? And they would find this patch as surprise? So I preferred not add "Fixes: " tag.

Now I notice 9f8550e4bd9d is already in stable/linux-4.19.y. So I think
Fixes tag would be fine.
I will send out a v2 with "Fixes:" tag and let Steffen choose:)

> Reviewed-by: Eyal Birger <eyal.birger@gmail.com>

thanks Eyal.
-antony

[PATCH v2 ipsec-next 1/2] xfrm: interface with if_id 0 should return error

[Antony Antony]

xfrm interface if_id = 0 would cause xfrm policy lookup errors since
Commit 9f8550e4bd9d.

Now explicitly fail to create an xfrm interface when if_id = 0

With this commit:
 ip link add ipsec0  type xfrm dev lo  if_id 0
 Error: if_id must be non zero.

v1->v2 change:
 - add Fixes: tag

Fixes: 9f8550e4bd9d ("xfrm: fix disable_xfrm sysctl when used on xfrm interfaces")
Signed-off-by: Antony Antony <antony.antony@secunet.com>
---
 net/xfrm/xfrm_interface.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/net/xfrm/xfrm_interface.c b/net/xfrm/xfrm_interface.c
index 41de46b5ffa9..57448fc519fc 100644
--- a/net/xfrm/xfrm_interface.c
+++ b/net/xfrm/xfrm_interface.c
@@ -637,11 +637,16 @@ static int xfrmi_newlink(struct net *src_net, struct net_device *dev,
 			struct netlink_ext_ack *extack)
 {
 	struct net *net = dev_net(dev);
-	struct xfrm_if_parms p;
+	struct xfrm_if_parms p = {};
 	struct xfrm_if *xi;
 	int err;
 
 	xfrmi_netlink_parms(data, &p);
+	if (!p.if_id) {
+		NL_SET_ERR_MSG(extack, "if_id must be non zero");
+		return -EINVAL;
+	}
+
 	xi = xfrmi_locate(net, &p);
 	if (xi)
 		return -EEXIST;
@@ -666,7 +671,12 @@ static int xfrmi_changelink(struct net_device *dev, struct nlattr *tb[],
 {
 	struct xfrm_if *xi = netdev_priv(dev);
 	struct net *net = xi->net;
-	struct xfrm_if_parms p;
+	struct xfrm_if_parms p = {};
+
+	if (!p.if_id) {
+		NL_SET_ERR_MSG(extack, "if_id must be non zero");
+		return -EINVAL;
+	}
 
 	xfrmi_netlink_parms(data, &p);
 	xi = xfrmi_locate(net, &p);
-- 
2.30.2

[PATCH v2 ipsec-next 2/2] xfrm: state and policy should fail if XFRMA_IF_ID 0

[Antony Antony]

xfrm ineterface does not allow xfrm if_id = 0
fail to create or update xfrm state and policy.

With this commit:
 ip xfrm policy add src 192.0.2.1 dst 192.0.2.2 dir out if_id 0
 RTNETLINK answers: Invalid argument

 ip xfrm state add src 192.0.2.1 dst 192.0.2.2 proto esp spi 1 \
            reqid 1 mode tunnel aead 'rfc4106(gcm(aes))' \
            0x1111111111111111111111111111111111111111 96 if_id 0
 RTNETLINK answers: Invalid argument

v1->v2 change:
 - add Fixes: tag

Fixes: 9f8550e4bd9d ("xfrm: fix disable_xfrm sysctl when used on xfrm interfaces")
Signed-off-by: Antony Antony <antony.antony@secunet.com>
---
 net/xfrm/xfrm_user.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/net/xfrm/xfrm_user.c b/net/xfrm/xfrm_user.c
index 064f91cd2f01..3e5fb1648be3 100644
--- a/net/xfrm/xfrm_user.c
+++ b/net/xfrm/xfrm_user.c
@@ -626,8 +626,13 @@ static struct xfrm_state *xfrm_state_construct(struct net *net,
 
 	xfrm_smark_init(attrs, &x->props.smark);
 
-	if (attrs[XFRMA_IF_ID])
+	if (attrs[XFRMA_IF_ID]) {
 		x->if_id = nla_get_u32(attrs[XFRMA_IF_ID]);
+		if (!x->if_id) {
+			err = -EINVAL;
+			goto error;
+		}
+	}
 
 	err = __xfrm_init_state(x, false, attrs[XFRMA_OFFLOAD_DEV]);
 	if (err)
@@ -1418,8 +1423,13 @@ static int xfrm_alloc_userspi(struct sk_buff *skb, struct nlmsghdr *nlh,
 
 	mark = xfrm_mark_get(attrs, &m);
 
-	if (attrs[XFRMA_IF_ID])
+	if (attrs[XFRMA_IF_ID]) {
 		if_id = nla_get_u32(attrs[XFRMA_IF_ID]);
+		if (!if_id) {
+			err = -EINVAL;
+			goto out_noput;
+		}
+	}
 
 	if (p->info.seq) {
 		x = xfrm_find_acq_byseq(net, mark, p->info.seq);
@@ -1732,8 +1742,13 @@ static struct xfrm_policy *xfrm_policy_construct(struct net *net, struct xfrm_us
 
 	xfrm_mark_get(attrs, &xp->mark);
 
-	if (attrs[XFRMA_IF_ID])
+	if (attrs[XFRMA_IF_ID]) {
 		xp->if_id = nla_get_u32(attrs[XFRMA_IF_ID]);
+		if (!xp->if_id) {
+			err = -EINVAL;
+			goto error;
+		}
+	}
 
 	return xp;
  error:
-- 
2.30.2

Re: [PATCH v2 ipsec-next 1/2] xfrm: interface with if_id 0 should return error

[Steffen Klassert]

On Sun, Dec 12, 2021 at 11:34:30AM +0100, Antony Antony wrote:
> xfrm interface if_id = 0 would cause xfrm policy lookup errors since
> Commit 9f8550e4bd9d.
> 
> Now explicitly fail to create an xfrm interface when if_id = 0
> 
> With this commit:
>  ip link add ipsec0  type xfrm dev lo  if_id 0
>  Error: if_id must be non zero.
> 
> v1->v2 change:
>  - add Fixes: tag
> 
> Fixes: 9f8550e4bd9d ("xfrm: fix disable_xfrm sysctl when used on xfrm interfaces")
> Signed-off-by: Antony Antony <antony.antony@secunet.com>

Applied the ipsec tree, thanks Antony!

Re: [PATCH v2 ipsec-next 2/2] xfrm: state and policy should fail if XFRMA_IF_ID 0

[Steffen Klassert]

On Sun, Dec 12, 2021 at 11:35:00AM +0100, Antony Antony wrote:
> xfrm ineterface does not allow xfrm if_id = 0
> fail to create or update xfrm state and policy.
> 
> With this commit:
>  ip xfrm policy add src 192.0.2.1 dst 192.0.2.2 dir out if_id 0
>  RTNETLINK answers: Invalid argument
> 
>  ip xfrm state add src 192.0.2.1 dst 192.0.2.2 proto esp spi 1 \
>             reqid 1 mode tunnel aead 'rfc4106(gcm(aes))' \
>             0x1111111111111111111111111111111111111111 96 if_id 0
>  RTNETLINK answers: Invalid argument
> 
> v1->v2 change:
>  - add Fixes: tag
> 
> Fixes: 9f8550e4bd9d ("xfrm: fix disable_xfrm sysctl when used on xfrm interfaces")
> Signed-off-by: Antony Antony <antony.antony@secunet.com>

Also applied to the ipsec tree, thanks a lot!