netdev - Re: [PATCH] IB/IPoIB: Check the headroom size

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:   Wed, 26 Apr 2017 09:27:38 -0400
From:   Doug Ledford <dledford@...hat.com>
To:     Honggang LI <honli@...hat.com>, Paolo Abeni <pabeni@...hat.com>
Cc:     Or Gerlitz <gerlitz.or@...il.com>,
        Erez Shitrit <erezsh@....mellanox.co.il>,
        Erez Shitrit <erezsh@...lanox.com>,
        "linux-rdma@...r.kernel.org" <linux-rdma@...r.kernel.org>,
        Linux Netdev List <netdev@...r.kernel.org>,
        David Miller <davem@...emloft.net>
Subject: Re: [PATCH] IB/IPoIB: Check the headroom size

On Wed, 2017-04-26 at 20:52 +0800, Honggang LI wrote:
> On Wed, Apr 26, 2017 at 09:46:34AM +0200, Paolo Abeni wrote:
> > 
> > On Wed, 2017-04-26 at 00:50 +0300, Or Gerlitz wrote:
> > > 
> > > so maybe @ least for the time being, we should be picking Hong's
> > > patch
> > > with proper change log and without the giant stack dump till we
> > > have
> > > something better. If you agree, can you do the re-write of the
> > > change
> > > log?
> > 
> > I think that Hong's patch is following the correct way to fix the
> > issue: ipoib_hard_header() can't assume that the skb headroom is at
> > least IPOIB_HARD_LEN bytes, as wrongly implied by commit
> > fc791b633515
> > (my fault, I'm sorry).
> > 
> > Perhaps we can make the code a little more robust with something
> > alongside the following (only compile tested):
> > ---
> > diff --git a/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > index d1d3fb7..d53d2e1 100644
> > --- a/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > +++ b/drivers/infiniband/ulp/ipoib/ipoib_main.c
> > @@ -1160,6 +1160,11 @@ static int ipoib_hard_header(struct sk_buff
> > *skb,
> >                              const void *daddr, const void *saddr,
> > unsigned len)
> >  {
> >         struct ipoib_header *header;
> > +       int ret;
> > +
> > +       ret = skb_cow_head(skb, IPOIB_HARD_LEN);
> 
> I don't think so. When this skb_under_panic arise, all slaves had
> been
> removed from a busy bonding interface,

I'm not sure this entirely makes sense.  If all slaves had been
removed, then we should never end up in ipoib_hard_header.  That should
only be called as long as there is at least one slave still attached to
the bond.  It might be during the process of removing the final slave,
but I don't think it can be after the final slave has been fully
removed from the bond.  Paolo, what should the bond driver be doing
once the slaves are gone?  Wouldn't it just be dropping every skb on
the floor without calling anyone's hard header routine?

>  so it is better to immediately
> give up and return error.
> 
> > 
> > +       if (ret)
> > +               return ret;
> >  
> >         header = (struct ipoib_header *) skb_push(skb, sizeof
> > *header);
> > ---
> > 
> > Paolo
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-
> > rdma" in
> > the body of a message to majordomo@...r.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
-- 
Doug Ledford <dledford@...hat.com>
    GPG KeyID: B826A3330E572FDD
   
Key fingerprint = AE6B 1BDA 122B 23B4 265B  1274 B826 A333 0E57 2FDD