netdev - Re: [PATCH 1/2] xfrm: interface with if

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

Date:   Sun, 12 Dec 2021 11:33:20 +0100
From:   Antony Antony <antony.antony@...unet.com>
To:     Eyal Birger <eyal.birger@...il.com>
CC:     <antony.antony@...unet.com>,
        Steffen Klassert <steffen.klassert@...unet.com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Linux Kernel Network Developers <netdev@...r.kernel.org>
Subject: Re: [PATCH 1/2] xfrm: interface with if_id 0 should return error

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@...unet.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@...unet.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@...il.com>

thanks Eyal.
-antony