netdev - Re: [PATCH net v4 3/3] net: prevent address rewrite in kernel

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

Message-ID: <9944248dba1bce861375fcce9de663934d933ba9.camel@redhat.com>
Date: Thu, 21 Sep 2023 20:08:56 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Jordan Rife <jrife@...gle.com>
Cc: Willem de Bruijn <willemdebruijn.kernel@...il.com>, davem@...emloft.net,
  edumazet@...gle.com, kuba@...nel.org, netdev@...r.kernel.org,
 dborkman@...nel.org,  philipp.reisner@...bit.com,
 lars.ellenberg@...bit.com,  christoph.boehmwalder@...bit.com,
 axboe@...nel.dk, airlied@...hat.com,  chengyou@...ux.alibaba.com,
 kaishen@...ux.alibaba.com, jgg@...pe.ca,  leon@...nel.org,
 bmt@...ich.ibm.com, isdn@...ux-pingi.de, ccaulfie@...hat.com, 
 teigland@...hat.com, mark@...heh.com, jlbec@...lplan.org, 
 joseph.qi@...ux.alibaba.com, sfrench@...ba.org, pc@...guebit.com, 
 lsahlber@...hat.com, sprasad@...rosoft.com, tom@...pey.com,
 horms@...ge.net.au,  ja@....bg, pablo@...filter.org, kadlec@...filter.org,
 fw@...len.de,  santosh.shilimkar@...cle.com, stable@...r.kernel.org
Subject: Re: [PATCH net v4 3/3] net: prevent address rewrite in kernel_bind()

On Thu, 2023-09-21 at 10:01 -0700, Jordan Rife wrote:
> On Thu, Sep 21, 2023 at 8:26 AM Paolo Abeni <pabeni@...hat.com> wrote:
> > 
> > On Thu, 2023-09-21 at 09:30 -0400, Willem de Bruijn wrote:
> > > On Thu, Sep 21, 2023 at 4:35 AM Paolo Abeni <pabeni@...hat.com> wrote:
> > > > 
> > > > On Wed, 2023-09-20 at 09:30 -0400, Willem de Bruijn wrote:
> > > > > Jordan Rife wrote:
> > > > > > Similar to the change in commit 0bdf399342c5("net: Avoid address
> > > > > > overwrite in kernel_connect"), BPF hooks run on bind may rewrite the
> > > > > > address passed to kernel_bind(). This change
> > > > > > 
> > > > > > 1) Makes a copy of the bind address in kernel_bind() to insulate
> > > > > >    callers.
> > > > > > 2) Replaces direct calls to sock->ops->bind() with kernel_bind()
> > > > > > 
> > > > > > Link: https://lore.kernel.org/netdev/20230912013332.2048422-1-jrife@google.com/
> > > > > > Fixes: 4fbac77d2d09 ("bpf: Hooks for sys_bind")
> > > > > > Cc: stable@...r.kernel.org
> > > > > > Signed-off-by: Jordan Rife <jrife@...gle.com>
> > > > > 
> > > > > Reviewed-by: Willem de Bruijn <willemb@...gle.com>
> > > > 
> > > > I fear this is going to cause a few conflicts with other trees. We can
> > > > still take it, but at very least we will need some acks from the
> > > > relevant maintainers.
> > > > 
> > > > I *think* it would be easier split this and patch 1/3 in individual
> > > > patches targeting the different trees, hopefully not many additional
> > > > patches will be required. What do you think?
> > > 
> > > Roughly how many patches would result from this one patch. From the
> > > stat line I count { block/drbd, char/agp, infiniband, isdn, fs/dlm,
> > > fs/ocfs2, fs/smb, netfilter, rds }. That's worst case nine callers
> > > plus the core patch to net/socket.c?
> > 
> > I think there should not be problems taking directly changes for rds
> > and nf/ipvs.
> > 
> > Additionally, I think the non network changes could consolidate the
> > bind and connect changes in a single patch.
> > 
> > It should be 7 not-network patches overall.
> > 
> > > If logistically simpler and you prefer the approach, we can also
> > > revisit Jordan's original approach, which embedded the memcpy inside
> > > the BPF branches.
> > > 
> > > That has the slight benefit to in-kernel callers that it limits the
> > > cost of the memcpy to cgroup_bpf_enabled. But adds a superfluous
> > > second copy to the more common userspace callers, again at least only
> > > if cgroup_bpf_enabled.
> > > 
> > > If so, it should at least move the whole logic around those BPF hooks
> > > into helper functions.
> > 
> > IMHO the approach implemented here is preferable, I suggest going
> > forward with it.
> > 
> > Thanks,
> > 
> > Paolo
> > 
> 
> > Additionally, I think the non network changes could consolidate the
> > bind and connect changes in a single patch.
> > 
> > It should be 7 not-network patches overall.
> 
> I'm fine with this. If there are no objections, I can drop the non-net
> changes in this patch series and send out several
> kernel_connect/kernel_bind patches to the appropriate trees as a
> follow up. Shall we wait to hear back from the maintainers or just go
> ahead with this plan?

I'm guessing you can go ahead with that: it should better fit anybody.

Thanks

Paolo

p.s. also using this reply to check if finally vger accepts my message
again...