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. 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.
This is one of those cases where preventing a future problem is easy,
I think it would be foolish of us to ignore it.
--
paul-moore.com
