On 05/10/2018 08:55 PM, Casey Schaufler wrote:
> From: Casey Schaufler <ca...@schaufler-ca.com>
> Date: Thu, 10 May 2018 15:54:25 -0700
> Subject: [PATCH 20/23] LSM: Move common usercopy into
> security_getpeersec_stream
>
> The modules implementing hook for getpeersec_stream
> don't need to be duplicating the copy-to-user checks.
> Moving the user copy part into the infrastructure makes
> the security module code simpler and reduces the places
> where user copy code may go awry.
>
> Signed-off-by: Casey Schaufler <ca...@schaufler-ca.com>
> ---
> include/linux/lsm_hooks.h | 10 ++++------
> include/linux/security.h | 6 ++++--
> security/apparmor/lsm.c | 28 ++++++++++------------------
> security/security.c | 17 +++++++++++++++--
> security/selinux/hooks.c | 22 +++++++---------------
> security/smack/smack_lsm.c | 19 ++++++++-----------
> 6 files changed, 48 insertions(+), 54 deletions(-)
>
> diff --git a/include/linux/lsm_hooks.h b/include/linux/lsm_hooks.h
> index 81504623afb4..84bc9ec01931 100644
> --- a/include/linux/lsm_hooks.h
> +++ b/include/linux/lsm_hooks.h
> @@ -841,9 +841,8 @@
> * SO_GETPEERSEC. For tcp sockets this can be meaningful if the
> * socket is associated with an ipsec SA.
> * @sock is the local socket.
> - * @optval userspace memory where the security state is to be copied.
> - * @optlen userspace int where the module should copy the actual length
> - * of the security state.
> + * @optval the security state.
> + * @optlen the actual length of the security state.
> * @len as input is the maximum length to copy to userspace provided
> * by the caller.
> * Return 0 if all is well, otherwise, typical getsockopt return
> @@ -1674,9 +1673,8 @@ union security_list_options {
> int (*socket_setsockopt)(struct socket *sock, int level, int optname);
> int (*socket_shutdown)(struct socket *sock, int how);
> int (*socket_sock_rcv_skb)(struct sock *sk, struct sk_buff *skb);
> - int (*socket_getpeersec_stream)(struct socket *sock,
> - char __user *optval,
> - int __user *optlen, unsigned int len);
> + int (*socket_getpeersec_stream)(struct socket *sock, char **optval,
> + int *optlen, unsigned int len);
> int (*socket_getpeersec_dgram)(struct socket *sock,
> struct sk_buff *skb,
> struct secids *secid);
> diff --git a/include/linux/security.h b/include/linux/security.h
> index ab70064c283f..712d138e0148 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -1369,8 +1369,10 @@ static inline int security_sock_rcv_skb(struct sock
> *sk,
> return 0;
> }
>
> -static inline int security_socket_getpeersec_stream(struct socket *sock,
> char __user *optval,
> - int __user *optlen,
> unsigned len)
> +static inline int security_socket_getpeersec_stream(struct socket *sock,
> + char __user *optval,
> + int __user *optlen,
> + unsigned int len)
> {
> return -ENOPROTOOPT;
> }
> diff --git a/security/apparmor/lsm.c b/security/apparmor/lsm.c
> index 90453dbb4fac..7444cfa689b3 100644
> --- a/security/apparmor/lsm.c
> +++ b/security/apparmor/lsm.c
> @@ -1017,10 +1017,8 @@ static struct aa_label *sk_peer_label(struct sock *sk)
> *
> * Note: for tcp only valid if using ipsec or cipso on lan
> */
> -static int apparmor_socket_getpeersec_stream(struct socket *sock,
> - char __user *optval,
> - int __user *optlen,
> - unsigned int len)
> +static int apparmor_socket_getpeersec_stream(struct socket *sock, char
> **optval,
> + int *optlen, unsigned int len)
> {
> char *name;
> int slen, error = 0;
> @@ -1037,22 +1035,16 @@ static int apparmor_socket_getpeersec_stream(struct
> socket *sock,
> FLAG_SHOW_MODE | FLAG_VIEW_SUBNS |
> FLAG_HIDDEN_UNCONFINED, GFP_KERNEL);
> /* don't include terminating \0 in slen, it breaks some apps */
> - if (slen < 0) {
> + if (slen < 0)
> error = -ENOMEM;
> - } else {
> - if (slen > len) {
> - error = -ERANGE;
> - } else if (copy_to_user(optval, name, slen)) {
> - error = -EFAULT;
> - goto out;
> - }
> - if (put_user(slen, optlen))
> - error = -EFAULT;
> -out:
> - kfree(name);
> -
> + else if (slen > len)
> + error = -ERANGE;
> + else {
> + *optlen = slen;
> + *optval = name;
> }
> -
> + if (error)
> + kfree(name);
> done:
> end_current_label_crit_section(label);
>
> diff --git a/security/security.c b/security/security.c
> index cbe1a497ec5a..6144ff52d862 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -1924,8 +1924,21 @@ EXPORT_SYMBOL(security_sock_rcv_skb);
> int security_socket_getpeersec_stream(struct socket *sock, char __user
> *optval,
> int __user *optlen, unsigned len)
> {
> - return call_int_hook(socket_getpeersec_stream, -ENOPROTOOPT, sock,
> - optval, optlen, len);
> + char *tval = NULL;
> + u32 tlen;
> + int rc;
> +
> + rc = call_int_hook(socket_getpeersec_stream, -ENOPROTOOPT, sock,
> + &tval, &tlen, len);
> + if (rc == 0) {
> + tlen = strlen(tval) + 1;
Why are you recomputing tlen here from what the module provided, and further
presuming it must be nul-terminated?
> + if (put_user(tlen, optlen))
> + rc = -EFAULT;
> + else if (copy_to_user(optval, tval, tlen))
> + rc = -EFAULT;
> + kfree(tval);
> + }
> + return rc;
> }
>
> int security_socket_getpeersec_dgram(struct socket *sock, struct sk_buff
> *skb,
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 81f104d9e85e..9520341daa78 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -4955,10 +4955,8 @@ static int selinux_socket_sock_rcv_skb(struct sock
> *sk, struct sk_buff *skb)
> return err;
> }
>
> -static int selinux_socket_getpeersec_stream(struct socket *sock,
> - __user char *optval,
> - __user int *optlen,
> - unsigned int len)
> +static int selinux_socket_getpeersec_stream(struct socket *sock, char
> **optval,
> + int *optlen, unsigned int len)
> {
> int err = 0;
> char *scontext;
> @@ -4979,18 +4977,12 @@ static int selinux_socket_getpeersec_stream(struct
> socket *sock,
> return err;
>
> if (scontext_len > len) {
> - err = -ERANGE;
> - goto out_len;
> + kfree(scontext);
> + return -ERANGE;
> }
> -
> - if (copy_to_user(optval, scontext, scontext_len))
> - err = -EFAULT;
> -
> -out_len:
> - if (put_user(scontext_len, optlen))
> - err = -EFAULT;
> - kfree(scontext);
> - return err;
> + *optval = scontext;
> + *optlen = scontext_len;
> + return 0;
> }
>
> static int selinux_socket_getpeersec_dgram(struct socket *sock, struct
> sk_buff *skb, struct secids *secid)
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 660a55ee8a57..12b00aac0c94 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -3878,14 +3878,12 @@ static int smack_socket_sock_rcv_skb(struct sock *sk,
> struct sk_buff *skb)
> *
> * returns zero on success, an error code otherwise
> */
> -static int smack_socket_getpeersec_stream(struct socket *sock,
> - char __user *optval,
> - int __user *optlen, unsigned len)
> +static int smack_socket_getpeersec_stream(struct socket *sock, char **optval,
> + int *optlen, unsigned int len)
> {
> struct socket_smack *ssp;
> char *rcp = "";
> int slen = 1;
> - int rc = 0;
>
> ssp = smack_sock(sock->sk);
> if (ssp->smk_packet != NULL) {
> @@ -3894,14 +3892,13 @@ static int smack_socket_getpeersec_stream(struct
> socket *sock,
> }
>
> if (slen > len)
> - rc = -ERANGE;
> - else if (copy_to_user(optval, rcp, slen) != 0)
> - rc = -EFAULT;
> -
> - if (put_user(slen, optlen) != 0)
> - rc = -EFAULT;
> + return -ERANGE;
>
> - return rc;
> + *optval = kstrdup(rcp, GFP_ATOMIC);
> + if (*optval == NULL)
> + return -ENOMEM;
> + *optlen = slen;
> + return 0;
> }
>
>
>
