LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Casey Schaufler <casey@schaufler-ca.com>
To: Stephen Smalley <sds@tycho.nsa.gov>,
	LSM <linux-security-module@vger.kernel.org>,
	LKLM <linux-kernel@vger.kernel.org>,
	Paul Moore <paul@paul-moore.com>,
	SE Linux <selinux@tycho.nsa.gov>,
	"SMACK-discuss@lists.01.org" <SMACK-discuss@lists.01.org>,
	John Johansen <john.johansen@canonical.com>,
	Kees Cook <keescook@chromium.org>,
	Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>,
	James Morris <jmorris@namei.org>
Subject: Re: [PATCH 20/23] LSM: Move common usercopy into
Date: Mon, 14 May 2018 11:55:41 -0700	[thread overview]
Message-ID: <0c6ec4e0-1562-8f30-09e9-9de0109b2cbc@schaufler-ca.com> (raw)
In-Reply-To: <992325e2-0f53-0b3d-ddd2-f4f8ba7ccf89@tycho.nsa.gov>

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 <casey@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 <casey@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?
> 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.

  reply	other threads:[~2018-05-14 18:55 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-11  0:30 [PATCH 00/23] LSM: Full security module stacking Casey Schaufler
2018-05-11  0:52 ` [PATCH 01/23] procfs: add smack subdir to attrs Casey Schaufler
2018-05-11  0:52 ` [PATCH 02/23] Smack: Abstract use of cred security blob Casey Schaufler
2018-05-11  0:52 ` [PATCH 03/23] SELinux: " Casey Schaufler
2018-05-11  0:52 ` [PATCH 04/23] LSM: Infrastructure management of the cred security Casey Schaufler
2018-05-11  0:52 ` [PATCH 05/23] SELinux: Abstract use of file security blob Casey Schaufler
2018-05-11  0:53 ` [PATCH 06/23] LSM: Infrastructure management of the file security Casey Schaufler
2018-05-11  0:53 ` [PATCH 07/23] LSM: Infrastructure management of the task security Casey Schaufler
2018-05-11  0:53 ` [PATCH 08/23] SELinux: Abstract use of inode security blob Casey Schaufler
2018-05-11  0:53 ` [PATCH 09/23] Smack: " Casey Schaufler
2018-05-11  0:53 ` [PATCH 10/23] LSM: Infrastructure management of the inode security Casey Schaufler
2018-05-14 15:04   ` Stephen Smalley
2018-05-14 16:32     ` Casey Schaufler
2018-05-11  0:54 ` [PATCH 11/23] LSM: Infrastructure management of the superblock Casey Schaufler
2018-05-11  0:54 ` [PATCH 12/23] LSM: Infrastructure management of the sock security Casey Schaufler
2018-05-11  0:54 ` [PATCH 13/23] LSM: Infrastructure management of the ipc security blob Casey Schaufler
2018-05-11  0:54 ` [PATCH 14/23] LSM: Infrastructure management of the key " Casey Schaufler
2018-05-11  0:55 ` [PATCH 15/23] LSM: Mark security blob allocation failures as unlikely Casey Schaufler
2018-05-11  0:55 ` [PATCH 16/23] LSM: Sharing of security blobs Casey Schaufler
2018-05-11  0:55 ` [PATCH 17/23] LSM: Allow mount options from multiple security modules Casey Schaufler
2018-05-11  0:55 ` [PATCH 18/23] LSM: Use multiple secids in security module interfaces Casey Schaufler
2018-05-11  0:55 ` [PATCH 19/23] LSM: Use multiple secids in LSM interfaces Casey Schaufler
2018-05-11  0:55 ` [PATCH 20/23] LSM: Move common usercopy into Casey Schaufler
2018-05-14 15:12   ` Stephen Smalley
2018-05-14 16:53     ` Stephen Smalley
2018-05-14 18:55       ` Casey Schaufler [this message]
2018-05-11  0:56 ` [PATCH 21/23] LSM: Multiple concurrent major security modules Casey Schaufler
2018-05-11  0:56 ` [PATCH 22/23] LSM: Fix setting of the IMA data in inode init Casey Schaufler
2018-05-11  0:56 ` [PATCH 23/23] Netfilter: Add a selection for Smack Casey Schaufler
2018-05-11  0:58 ` [PATCH 00/23] LSM: Full security module stacking Casey Schaufler
2018-05-11 20:25 ` [PATCH 24/23] LSM: Functions for dealing with struct secids Casey Schaufler

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=0c6ec4e0-1562-8f30-09e9-9de0109b2cbc@schaufler-ca.com \
    --to=casey@schaufler-ca.com \
    --cc=SMACK-discuss@lists.01.org \
    --cc=jmorris@namei.org \
    --cc=john.johansen@canonical.com \
    --cc=keescook@chromium.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=paul@paul-moore.com \
    --cc=penguin-kernel@i-love.sakura.ne.jp \
    --cc=sds@tycho.nsa.gov \
    --cc=selinux@tycho.nsa.gov \
    --subject='Re: [PATCH 20/23] LSM: Move common usercopy into' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).