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.
--
paul-moore.com
