On Fri, Oct 14, 2022 at 11:51 AM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
> On Thu, Oct 13, 2022 at 8:59 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> >
> > On Thu, Oct 13, 2022 at 11:53 AM Jakub Kicinski <kuba@xxxxxxxxxx> wrote:
> > > On Mon, 10 Oct 2022 17:58:29 -0400 Paul Moore wrote:
> > > > Commit 4ff09db1b79b ("bpf: net: Change sk_getsockopt() to take the
> > > > sockptr_t argument") made it possible to call sk_getsockopt()
> > > > with both user and kernel address space buffers through the use of
> > > > the sockptr_t type. Unfortunately at the time of conversion the
> > > > security_socket_getpeersec_stream() LSM hook was written to only
> > > > accept userspace buffers, and in a desire to avoid having to change
> > > > the LSM hook the commit author simply passed the sockptr_t's
> > > > userspace buffer pointer. Since the only sk_getsockopt() callers
> > > > at the time of conversion which used kernel sockptr_t buffers did
> > > > not allow SO_PEERSEC, and hence the
> > > > security_socket_getpeersec_stream() hook, this was acceptable but
> > > > also very fragile as future changes presented the possibility of
> > > > silently passing kernel space pointers to the LSM hook.
> > > >
> > > > There are several ways to protect against this, including careful
> > > > code review of future commits, but since relying on code review to
> > > > catch bugs is a recipe for disaster and the upstream eBPF maintainer
> > > > is "strongly against defensive programming", this patch updates the
> > > > LSM hook, and all of the implementations to support sockptr_t and
> > > > safely handle both user and kernel space buffers.
> > >
> > > Code seems sane, FWIW, but the commit message sounds petty,
> > > which is likely why nobody is willing to ack it.
> >
> > Heh, feel free to look at Alexei's comments to my original email; the
> > commit description seems spot on to me.
>
> Paul,
>
> The commit message:
> "
> also very fragile as future changes presented the possibility of
> silently passing kernel space pointers to the LSM hook.
> "
> shows that you do not understand how copy_from_user works.
>
> Martin's change didn't introduce any fragility.
> Do you realize that user space can pass any 64-bit value
> as 'user pointer' via syscall, right?
> And that value may just as well be a valid kernel address.
> copy_from_user always had a check to prevent reading kernel
> memory. It will simply return an error when it sees
> kernel address.
>
> Your patch itself is not wrong per-se, but it's doing
> not what you think it's doing.
> Right now the patch is useless, but
> if switch statement in sol_socket_sockopt() would be relaxed
> the bpf progs would be able to pass kernel pointers
> to security_socket_getpeersec which makes little sense at this point.
> So the code you're adding will be a dead code without a test
> for the foreseeable future.
> Because of that I can only add my Nack.
Oh don't worry, I've already registered your NACK because the patch
has a three line diff in net/core/sock.c and it's going in via the LSM
tree. I'll CC you on the pull request and mention your NACK to Linus,
you can feel free to make whatever argument you believe justifies your
objection at that point in time.
However, just so I'm clear on your new objection, you are basically
saying that copy_to_sockptr() shouldn't exist? If you honestly
believe that, and you aren't just picking on this patch because of a
grudge, I would encourage you to submit a patch removing
copy_to_sockptr() and friends from the kernel and let's see what
happens; please CC me on the patch(es) as I think the discussion for
that would be very interesting :)
--
paul-moore.com
