Re: [PATCH bpf-next 02/17] bpf: net: Change sk_getsockopt() to take the sockptr_t argument

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Wed, Aug 24, 2022 at 3:29 PM Martin KaFai Lau <kafai@xxxxxx> wrote:
>
> This patch changes sk_getsockopt() to take the sockptr_t argument
> such that it can be used by bpf_getsockopt(SOL_SOCKET) in a
> latter patch.
>
> security_socket_getpeersec_stream() is not changed.  It stays
> with the __user ptr (optval.user and optlen.user) to avoid changes
> to other security hooks.  bpf_getsockopt(SOL_SOCKET) also does not
> support SO_PEERSEC.
>
> Signed-off-by: Martin KaFai Lau <kafai@xxxxxx>
> ---
>  include/linux/filter.h  |  3 +--
>  include/linux/sockptr.h |  5 +++++
>  net/core/filter.c       |  5 ++---
>  net/core/sock.c         | 43 +++++++++++++++++++++++------------------
>  4 files changed, 32 insertions(+), 24 deletions(-)
>
> diff --git a/include/linux/filter.h b/include/linux/filter.h
> index a5f21dc3c432..527ae1d64e27 100644
> --- a/include/linux/filter.h
> +++ b/include/linux/filter.h
> @@ -900,8 +900,7 @@ int sk_reuseport_attach_filter(struct sock_fprog *fprog, struct sock *sk);
>  int sk_reuseport_attach_bpf(u32 ufd, struct sock *sk);
>  void sk_reuseport_prog_free(struct bpf_prog *prog);
>  int sk_detach_filter(struct sock *sk);
> -int sk_get_filter(struct sock *sk, struct sock_filter __user *filter,
> -                 unsigned int len);
> +int sk_get_filter(struct sock *sk, sockptr_t optval, unsigned int len);
>
>  bool sk_filter_charge(struct sock *sk, struct sk_filter *fp);
>  void sk_filter_uncharge(struct sock *sk, struct sk_filter *fp);
> diff --git a/include/linux/sockptr.h b/include/linux/sockptr.h
> index d45902fb4cad..bae5e2369b4f 100644
> --- a/include/linux/sockptr.h
> +++ b/include/linux/sockptr.h
> @@ -64,6 +64,11 @@ static inline int copy_to_sockptr_offset(sockptr_t dst, size_t offset,
>         return 0;
>  }
>
> +static inline int copy_to_sockptr(sockptr_t dst, const void *src, size_t size)
> +{
> +       return copy_to_sockptr_offset(dst, 0, src, size);
> +}
> +
>  static inline void *memdup_sockptr(sockptr_t src, size_t len)
>  {
>         void *p = kmalloc_track_caller(len, GFP_USER | __GFP_NOWARN);
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 63e25d8ce501..0f6f86b9e487 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -10712,8 +10712,7 @@ int sk_detach_filter(struct sock *sk)
>  }
>  EXPORT_SYMBOL_GPL(sk_detach_filter);
>
> -int sk_get_filter(struct sock *sk, struct sock_filter __user *ubuf,
> -                 unsigned int len)
> +int sk_get_filter(struct sock *sk, sockptr_t optval, unsigned int len)
>  {
>         struct sock_fprog_kern *fprog;
>         struct sk_filter *filter;
> @@ -10744,7 +10743,7 @@ int sk_get_filter(struct sock *sk, struct sock_filter __user *ubuf,
>                 goto out;
>
>         ret = -EFAULT;
> -       if (copy_to_user(ubuf, fprog->filter, bpf_classic_proglen(fprog)))
> +       if (copy_to_sockptr(optval, fprog->filter, bpf_classic_proglen(fprog)))
>                 goto out;
>
>         /* Instead of bytes, the API requests to return the number
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 21bc4bf6b485..7fa30fd4b37f 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -712,8 +712,8 @@ static int sock_setbindtodevice(struct sock *sk, sockptr_t optval, int optlen)
>         return ret;
>  }
>
> -static int sock_getbindtodevice(struct sock *sk, char __user *optval,
> -                               int __user *optlen, int len)
> +static int sock_getbindtodevice(struct sock *sk, sockptr_t optval,
> +                               sockptr_t optlen, int len)
>  {
>         int ret = -ENOPROTOOPT;
>  #ifdef CONFIG_NETDEVICES
> @@ -737,12 +737,12 @@ static int sock_getbindtodevice(struct sock *sk, char __user *optval,
>         len = strlen(devname) + 1;
>
>         ret = -EFAULT;
> -       if (copy_to_user(optval, devname, len))
> +       if (copy_to_sockptr(optval, devname, len))
>                 goto out;
>
>  zero:
>         ret = -EFAULT;
> -       if (put_user(len, optlen))
> +       if (copy_to_sockptr(optlen, &len, sizeof(int)))
>                 goto out;
>
>         ret = 0;
> @@ -1568,20 +1568,23 @@ static void cred_to_ucred(struct pid *pid, const struct cred *cred,
>         }
>  }
>
> -static int groups_to_user(gid_t __user *dst, const struct group_info *src)
> +static int groups_to_user(sockptr_t dst, const struct group_info *src)
>  {
>         struct user_namespace *user_ns = current_user_ns();
>         int i;
>
> -       for (i = 0; i < src->ngroups; i++)
> -               if (put_user(from_kgid_munged(user_ns, src->gid[i]), dst + i))
> +       for (i = 0; i < src->ngroups; i++) {
> +               gid_t gid = from_kgid_munged(user_ns, src->gid[i]);
> +
> +               if (copy_to_sockptr_offset(dst, i * sizeof(gid), &gid, sizeof(gid)))
>                         return -EFAULT;
> +       }
>
>         return 0;
>  }
>
>  static int sk_getsockopt(struct sock *sk, int level, int optname,
> -                        char __user *optval, int __user *optlen)
> +                        sockptr_t optval, sockptr_t optlen)
>  {
>         struct socket *sock = sk->sk_socket;
>
> @@ -1600,7 +1603,7 @@ static int sk_getsockopt(struct sock *sk, int level, int optname,
>         int lv = sizeof(int);
>         int len;
>
> -       if (get_user(len, optlen))
> +       if (copy_from_sockptr(&len, optlen, sizeof(int)))

Do we want to be consistent wrt to sizeof?

copy_from_sockptr(&len, optlen, sizeof(int))
vs
copy_from_sockptr(&len, optlen, sizeof(optlen))

Alternatively, should we have put_sockptr/get_sockopt with a semantics
similar to put_user/get_user to remove all this ambiguity?

>                 return -EFAULT;
>         if (len < 0)
>                 return -EINVAL;
> @@ -1735,7 +1738,7 @@ static int sk_getsockopt(struct sock *sk, int level, int optname,
>                 cred_to_ucred(sk->sk_peer_pid, sk->sk_peer_cred, &peercred);
>                 spin_unlock(&sk->sk_peer_lock);
>
> -               if (copy_to_user(optval, &peercred, len))
> +               if (copy_to_sockptr(optval, &peercred, len))
>                         return -EFAULT;
>                 goto lenout;
>         }
> @@ -1753,11 +1756,11 @@ static int sk_getsockopt(struct sock *sk, int level, int optname,
>                 if (len < n * sizeof(gid_t)) {
>                         len = n * sizeof(gid_t);
>                         put_cred(cred);
> -                       return put_user(len, optlen) ? -EFAULT : -ERANGE;
> +                       return copy_to_sockptr(optlen, &len, sizeof(int)) ? -EFAULT : -ERANGE;
>                 }
>                 len = n * sizeof(gid_t);
>
> -               ret = groups_to_user((gid_t __user *)optval, cred->group_info);
> +               ret = groups_to_user(optval, cred->group_info);
>                 put_cred(cred);
>                 if (ret)
>                         return ret;
> @@ -1773,7 +1776,7 @@ static int sk_getsockopt(struct sock *sk, int level, int optname,
>                         return -ENOTCONN;
>                 if (lv < len)
>                         return -EINVAL;
> -               if (copy_to_user(optval, address, len))
> +               if (copy_to_sockptr(optval, address, len))
>                         return -EFAULT;
>                 goto lenout;
>         }
> @@ -1790,7 +1793,7 @@ static int sk_getsockopt(struct sock *sk, int level, int optname,
>                 break;
>
>         case SO_PEERSEC:
> -               return security_socket_getpeersec_stream(sock, optval, optlen, len);
> +               return security_socket_getpeersec_stream(sock, optval.user, optlen.user, len);

I'm assuming there should be something to prevent this being called
from BPF? (haven't read all the patches yet)
Do we want to be a bit more defensive with 'if (!optval.user) return
-EFAULT' or something similar?


>         case SO_MARK:
>                 v.val = sk->sk_mark;
> @@ -1822,7 +1825,7 @@ static int sk_getsockopt(struct sock *sk, int level, int optname,
>                 return sock_getbindtodevice(sk, optval, optlen, len);
>
>         case SO_GET_FILTER:
> -               len = sk_get_filter(sk, (struct sock_filter __user *)optval, len);
> +               len = sk_get_filter(sk, optval, len);
>                 if (len < 0)
>                         return len;
>
> @@ -1870,7 +1873,7 @@ static int sk_getsockopt(struct sock *sk, int level, int optname,
>                 sk_get_meminfo(sk, meminfo);
>
>                 len = min_t(unsigned int, len, sizeof(meminfo));
> -               if (copy_to_user(optval, &meminfo, len))
> +               if (copy_to_sockptr(optval, &meminfo, len))
>                         return -EFAULT;
>
>                 goto lenout;
> @@ -1939,10 +1942,10 @@ static int sk_getsockopt(struct sock *sk, int level, int optname,
>
>         if (len > lv)
>                 len = lv;
> -       if (copy_to_user(optval, &v, len))
> +       if (copy_to_sockptr(optval, &v, len))
>                 return -EFAULT;
>  lenout:
> -       if (put_user(len, optlen))
> +       if (copy_to_sockptr(optlen, &len, sizeof(int)))
>                 return -EFAULT;
>         return 0;
>  }
> @@ -1950,7 +1953,9 @@ static int sk_getsockopt(struct sock *sk, int level, int optname,
>  int sock_getsockopt(struct socket *sock, int level, int optname,
>                     char __user *optval, int __user *optlen)
>  {
> -       return sk_getsockopt(sock->sk, level, optname, optval, optlen);
> +       return sk_getsockopt(sock->sk, level, optname,
> +                            USER_SOCKPTR(optval),
> +                            USER_SOCKPTR(optlen));
>  }
>
>  /*
> --
> 2.30.2
>



[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux