On Fri, Oct 7, 2022 at 1:06 PM Paul Moore [off-list ref] wrote:

On Fri, Oct 7, 2022 at 3:13 PM Alexei Starovoitov
[off-list ref] wrote:

quoted

On Fri, Oct 7, 2022 at 10:43 AM Paul Moore [off-list ref] wrote:

quoted

On Wed, Oct 5, 2022 at 4:44 PM Paul Moore [off-list ref] wrote:

quoted

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".