On 5/14/2018 9:53 AM, Stephen Smalley wrote: > On 05/14/2018 11:12 AM, Stephen Smalley wrote: >> On 05/10/2018 08:55 PM, Casey Schaufler wrote: >>> From: Casey Schaufler <[email protected]> >>> 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 <[email protected]> >>> --- >>> 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? > Also, at least for SELinux, we copy out the length even when returning > ERANGE, and libselinux uses that to realloc the buffer and try again.
All points acked and will be repaired. Thanks.
