* Re: [PATCH 1/2] xfrm: interface with if_id 0 should return error
2021-12-09 15:35 [PATCH 1/2] xfrm: interface with if_id 0 should return error Antony Antony
@ 2021-12-10 17:22 ` Eyal Birger
2021-12-12 10:33 ` Antony Antony
2021-12-12 10:34 ` [PATCH v2 ipsec-next " Antony Antony
2021-12-12 10:35 ` [PATCH v2 ipsec-next 2/2] xfrm: state and policy should fail if XFRMA_IF_ID 0 Antony Antony
2 siblings, 1 reply; 7+ messages in thread
From: Eyal Birger @ 2021-12-10 17:22 UTC (permalink / raw)
To: antony.antony
Cc: Steffen Klassert, Herbert Xu, David S. Miller, Jakub Kicinski,
Linux Kernel Network Developers
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
>
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH 1/2] xfrm: interface with if_id 0 should return error
2021-12-10 17:22 ` Eyal Birger
@ 2021-12-12 10:33 ` Antony Antony
0 siblings, 0 replies; 7+ messages in thread
From: Antony Antony @ 2021-12-12 10:33 UTC (permalink / raw)
To: Eyal Birger
Cc: antony.antony, Steffen Klassert, Herbert Xu, David S. Miller,
Jakub Kicinski, Linux Kernel Network Developers
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 ipsec-next 1/2] xfrm: interface with if_id 0 should return error
2021-12-09 15:35 [PATCH 1/2] xfrm: interface with if_id 0 should return error Antony Antony
2021-12-10 17:22 ` Eyal Birger
@ 2021-12-12 10:34 ` Antony Antony
2021-12-20 8:35 ` Steffen Klassert
2021-12-12 10:35 ` [PATCH v2 ipsec-next 2/2] xfrm: state and policy should fail if XFRMA_IF_ID 0 Antony Antony
2 siblings, 1 reply; 7+ messages in thread
From: Antony Antony @ 2021-12-12 10:34 UTC (permalink / raw)
To: Steffen Klassert
Cc: Eyal Birger, Herbert Xu, Antony Antony, David S. Miller,
Jakub Kicinski, netdev
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
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v2 ipsec-next 1/2] xfrm: interface with if_id 0 should return error
2021-12-12 10:34 ` [PATCH v2 ipsec-next " Antony Antony
@ 2021-12-20 8:35 ` Steffen Klassert
0 siblings, 0 replies; 7+ messages in thread
From: Steffen Klassert @ 2021-12-20 8:35 UTC (permalink / raw)
To: Antony Antony
Cc: Eyal Birger, Herbert Xu, David S. Miller, Jakub Kicinski, netdev
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!
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 ipsec-next 2/2] xfrm: state and policy should fail if XFRMA_IF_ID 0
2021-12-09 15:35 [PATCH 1/2] xfrm: interface with if_id 0 should return error Antony Antony
2021-12-10 17:22 ` Eyal Birger
2021-12-12 10:34 ` [PATCH v2 ipsec-next " Antony Antony
@ 2021-12-12 10:35 ` Antony Antony
2021-12-20 8:36 ` Steffen Klassert
2 siblings, 1 reply; 7+ messages in thread
From: Antony Antony @ 2021-12-12 10:35 UTC (permalink / raw)
To: Steffen Klassert
Cc: Eyal Birger, Herbert Xu, Antony Antony, David S. Miller,
Jakub Kicinski, netdev
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
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH v2 ipsec-next 2/2] xfrm: state and policy should fail if XFRMA_IF_ID 0
2021-12-12 10:35 ` [PATCH v2 ipsec-next 2/2] xfrm: state and policy should fail if XFRMA_IF_ID 0 Antony Antony
@ 2021-12-20 8:36 ` Steffen Klassert
0 siblings, 0 replies; 7+ messages in thread
From: Steffen Klassert @ 2021-12-20 8:36 UTC (permalink / raw)
To: Antony Antony
Cc: Eyal Birger, Herbert Xu, David S. Miller, Jakub Kicinski, netdev
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!
^ permalink raw reply [flat|nested] 7+ messages in thread