Re: [PATCH v12 15/25] LSM: Use lsmcontext in security_secid_to_secctx

From: Stephen Smalley <sds@tycho.nsa.gov>
To: Casey Schaufler <casey@schaufler-ca.com>,
	casey.schaufler@intel.com, jmorris@namei.org,
	linux-security-module@vger.kernel.org, selinux@vger.kernel.org
Cc: keescook@chromium.org, john.johansen@canonical.com,
	penguin-kernel@i-love.sakura.ne.jp, paul@paul-moore.com
Subject: Re: [PATCH v12 15/25] LSM: Use lsmcontext in security_secid_to_secctx
Date: Wed, 18 Dec 2019 14:33:01 -0500	[thread overview]
Message-ID: <dd598a2d-716f-8bf5-fe71-9c46b6d1814b@tycho.nsa.gov> (raw)
In-Reply-To: <20191216223621.5127-16-casey@schaufler-ca.com>

On 12/16/19 5:36 PM, Casey Schaufler wrote:
> Replace the (secctx,seclen) pointer pair with a single
> lsmcontext pointer to allow return of the LSM identifier
> along with the context and context length. This allows
> security_release_secctx() to know how to release the
> context. Callers have been modified to use or save the
> returned data from the new structure.
> 
> Signed-off-by: Casey Schaufler <casey@schaufler-ca.com>
> cc: netdev@vger.kernel.org

Sorry, have to retract my ack.  See below.

> ---
> diff --git a/kernel/audit.c b/kernel/audit.c
> index 3305c4af43a8..224c7b4a1bc0 100644
> --- a/kernel/audit.c
> +++ b/kernel/audit.c
> @@ -1178,9 +1178,8 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
>   	struct audit_buffer	*ab;
>   	u16			msg_type = nlh->nlmsg_type;
>   	struct audit_sig_info   *sig_data;
> -	char			*ctx = NULL;
>   	u32			len;
> -	struct lsmcontext	scaff; /* scaffolding */
> +	struct lsmcontext	context = { };
>   
>   	err = audit_netlink_ok(skb, msg_type);
>   	if (err)
> @@ -1418,25 +1417,22 @@ static int audit_receive_msg(struct sk_buff *skb, struct nlmsghdr *nlh)
>   	case AUDIT_SIGNAL_INFO:
>   		len = 0;
>   		if (lsmblob_is_set(&audit_sig_lsm)) {
> -			err = security_secid_to_secctx(&audit_sig_lsm, &ctx,
> -						       &len);
> +			err = security_secid_to_secctx(&audit_sig_lsm,
> +						       &context);
>   			if (err)
>   				return err;
>   		}
>   		sig_data = kmalloc(sizeof(*sig_data) + len, GFP_KERNEL);

You forgot to update this kmalloc() to use context.len, so you'll end up 
allocating too small a buffer and then writing out of bounds upon the 
memcpy below.

>   		if (!sig_data) {
> -			if (lsmblob_is_set(&audit_sig_lsm)) {
> -				lsmcontext_init(&scaff, ctx, len, 0);
> -				security_release_secctx(&scaff);
> -			}
> +			if (lsmblob_is_set(&audit_sig_lsm))
> +				security_release_secctx(&context);
>   			return -ENOMEM;
>   		}
>   		sig_data->uid = from_kuid(&init_user_ns, audit_sig_uid);
>   		sig_data->pid = audit_sig_pid;
>   		if (lsmblob_is_set(&audit_sig_lsm)) {
> -			memcpy(sig_data->ctx, ctx, len);
> -			lsmcontext_init(&scaff, ctx, len, 0);
> -			security_release_secctx(&scaff);
> +			memcpy(sig_data->ctx, context.context, context.len);
> +			security_release_secctx(&context);
>   		}
>   		audit_send_reply(skb, seq, AUDIT_SIGNAL_INFO, 0, 0,
>   				 sig_data, sizeof(*sig_data) + len);