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.
> 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.
> 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.
By that argument we should be checking all pointers for NULL
"because it's easy to do".
