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
>
