On Mon, Oct 10, 2022 at 2:19 AM Alexei Starovoitov
<alexei.starovoitov@xxxxxxxxx> wrote:
> On Sun, Oct 9, 2022 at 3:01 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> > On Fri, Oct 7, 2022 at 5:55 PM Alexei Starovoitov
> > <alexei.starovoitov@xxxxxxxxx> wrote:
> > > On Fri, Oct 7, 2022 at 1:06 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> > > > On Fri, Oct 7, 2022 at 3:13 PM Alexei Starovoitov
> > > > <alexei.starovoitov@xxxxxxxxx> wrote:
> > > > > On Fri, Oct 7, 2022 at 10:43 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> > > > > > On Wed, Oct 5, 2022 at 4:44 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> > > > > > >
> > > > > > > Hi Martin,
> > > > > > >
> > > > > > > In commit 4ff09db1b79b ("bpf: net: Change sk_getsockopt() to take the
> > > > > > > sockptr_t argument") I see you wrapped the getsockopt value/len
> > > > > > > pointers with sockptr_t and in the SO_PEERSEC case you pass the
> > > > > > > sockptr_t:user field to avoid having to update the LSM hook and
> > > > > > > implementations. I think that's fine, especially as you note that
> > > > > > > eBPF does not support fetching the SO_PEERSEC information, but I think
> > > > > > > it would be good to harden this case to prevent someone from calling
> > > > > > > sk_getsockopt(SO_PEERSEC) with kernel pointers. What do you think of
> > > > > > > something like this?
> > > > > > >
> > > > > > > static int sk_getsockopt(...)
> > > > > > > {
> > > > > > > /* ... */
> > > > > > > case SO_PEERSEC:
> > > > > > > if (optval.is_kernel || optlen.is_kernel)
> > > > > > > return -EINVAL;
> > > > > > > return security_socket_getpeersec_stream(...);
> > > > > > > /* ... */
> > > > > > > }
> > > > > >
> > > > > > Any thoughts on this Martin, Alexei? It would be nice to see this
> > > > > > fixed soon ...
> > > > >
> > > > > 'fixed' ?
> > > > > I don't see any bug.
> > > > > Maybe WARN_ON_ONCE can be added as a precaution, but also dubious value.
> > > >
> > > > Prior to the change it was impossible to call
> > > > sock_getsockopt(SO_PEERSEC) with a kernel address space pointer, now
> > > > with 4ff09db1b79b is it possible to call sk_getsockopt(SO_PEERSEC)
> > > > with a kernel address space pointer and cause problems.
> > >
> > > No. It's not possible. There is no path in the kernel that
> > > can do that.
> >
> > If we look at the very next sentence in my last reply you see that I
> > acknowledge that there may be no callers that currently do that, but
> > it seems like an easy mistake for someone to make. I've seen kernel
> > coding errors similar to this in the past, it seems like a reasonable
> > thing to protect against, especially considering it is well outside of
> > any performance critical path.
> >
> > > > Perhaps there
> > > > are no callers in the kernel that do such a thing at the moment, but
> > > > it seems like an easy mistake for someone to make, and the code to
> > > > catch it is both trivial and out of any critical path.
> > >
> > > Not easy at all.
> > > There is only way place in the whole kernel that does:
> > > return sk_getsockopt(sk, SOL_SOCKET, optname,
> > > KERNEL_SOCKPTR(optval),
> > > KERNEL_SOCKPTR(optlen));
> > >
> > > and there is an allowlist of optname-s right in front of it.
> > > SO_PEERSEC is not there.
> > > For security_socket_getpeersec_stream to be called with kernel
> > > address the developer would need to add SO_PEERSEC to that allowlist.
> > > Which will be trivially caught during the code review.
> >
> > A couple of things come to mind ... First, the concern isn't the
> > existing caller(s), as mentioned above, but future callers. Second,
> > while the kernel code review process is good, the number of serious
> > kernel bugs that have passed uncaught through the code review process
> > is staggering.
> >
> > > > This is one of those cases where preventing a future problem is easy,
> > > > I think it would be foolish of us to ignore it.
> > >
> > > Disagree. It's just a typical example of defensive programming
> > > which I'm strongly against.
> >
> > That's a pretty bold statement, good luck with that.
> >
> > > By that argument we should be checking all pointers for NULL
> > > "because it's easy to do".
> >
> > That's not the argument being made here, but based on your previous
> > statements of trusting code review to catch bugs and your opposition
> > to defensive programming it seems pretty unlikely we're going to find
> > common ground.
> >
> > I'll take care of this in the LSM tree.
>
> Are you saying you'll add a patch to sk_getsockopt
> in net/core/sock.c without going through net or bpf trees?
> Paul, you're crossing the line.
I believe my exact comment was "I'll take care of this in the LSM
tree." I haven't thought tpo hard about the details yet, but thinking
quickly I can imagine several different approaches with varying levels
of change required in sk_getsockopt(); it would be premature to
comment much beyond that. It also looks like David Laight has similar
concerns, so it's possible he might work on resolving this too,
discussions are (obviously) ongoing.
As far as crossing a line is concerned, I suggest you first look in
the mirror with respect to changes in the security/ subdir that did
not go through one of the LSM trees. There are quite a few patches
from netdev/bpf that have touched security/ without going through a
LSM tree or getting a Reviewed-by/Acked-by/etc. from a LSM developer.
In fact I don't even have to go back a year and I see at least one
patch that touches code under security/ that was committed by you
without any LSM developer reviews/acks/etc.
--
paul-moore.com
