From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.linuxfoundation.org ([140.211.169.12]:47271 "EHLO mail.linuxfoundation.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755023AbcGYVbs (ORCPT ); Mon, 25 Jul 2016 17:31:48 -0400 From: Greg Kroah-Hartman To: linux-kernel@vger.kernel.org Cc: Greg Kroah-Hartman , stable@vger.kernel.org, Vegard Nossum , Al Viro , John Johansen , Paul Moore , Stephen Smalley , Eric Paris , Casey Schaufler , Tyler Hicks , James Morris Subject: [PATCH 4.6 092/203] apparmor: fix oops, validate buffer size in apparmor_setprocattr() Date: Mon, 25 Jul 2016 13:55:07 -0700 Message-Id: <20160725203433.104249131@linuxfoundation.org> In-Reply-To: <20160725203429.221747288@linuxfoundation.org> References: <20160725203429.221747288@linuxfoundation.org> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Sender: stable-owner@vger.kernel.org List-ID: 4.6-stable review patch. If anyone has any objections, please let me know. ------------------ From: Vegard Nossum commit 30a46a4647fd1df9cf52e43bf467f0d9265096ca upstream. When proc_pid_attr_write() was changed to use memdup_user apparmor's (interface violating) assumption that the setprocattr buffer was always a single page was violated. The size test is not strictly speaking needed as proc_pid_attr_write() will reject anything larger, but for the sake of robustness we can keep it in. SMACK and SELinux look safe to me, but somebody else should probably have a look just in case. Based on original patch from Vegard Nossum modified for the case that apparmor provides null termination. Fixes: bb646cdb12e75d82258c2f2e7746d5952d3e321a Reported-by: Vegard Nossum Cc: Al Viro Cc: John Johansen Cc: Paul Moore Cc: Stephen Smalley Cc: Eric Paris Cc: Casey Schaufler Signed-off-by: John Johansen Reviewed-by: Tyler Hicks Signed-off-by: James Morris Signed-off-by: Greg Kroah-Hartman --- security/apparmor/lsm.c | 36 +++++++++++++++++++----------------- 1 file changed, 19 insertions(+), 17 deletions(-) --- a/security/apparmor/lsm.c +++ b/security/apparmor/lsm.c @@ -523,34 +523,34 @@ static int apparmor_setprocattr(struct t { struct common_audit_data sa; struct apparmor_audit_data aad = {0,}; - char *command, *args = value; + char *command, *largs = NULL, *args = value; size_t arg_size; int error; if (size == 0) return -EINVAL; - /* args points to a PAGE_SIZE buffer, AppArmor requires that - * the buffer must be null terminated or have size <= PAGE_SIZE -1 - * so that AppArmor can null terminate them - */ - if (args[size - 1] != '\0') { - if (size == PAGE_SIZE) - return -EINVAL; - args[size] = '\0'; - } - /* task can only write its own attributes */ if (current != task) return -EACCES; - args = value; + /* AppArmor requires that the buffer must be null terminated atm */ + if (args[size - 1] != '\0') { + /* null terminate */ + largs = args = kmalloc(size + 1, GFP_KERNEL); + if (!args) + return -ENOMEM; + memcpy(args, value, size); + args[size] = '\0'; + } + + error = -EINVAL; args = strim(args); command = strsep(&args, " "); if (!args) - return -EINVAL; + goto out; args = skip_spaces(args); if (!*args) - return -EINVAL; + goto out; arg_size = size - (args - (char *) value); if (strcmp(name, "current") == 0) { @@ -576,10 +576,12 @@ static int apparmor_setprocattr(struct t goto fail; } else /* only support the "current" and "exec" process attributes */ - return -EINVAL; + goto fail; if (!error) error = size; +out: + kfree(largs); return error; fail: @@ -588,9 +590,9 @@ fail: aad.profile = aa_current_profile(); aad.op = OP_SETPROCATTR; aad.info = name; - aad.error = -EINVAL; + aad.error = error = -EINVAL; aa_audit_msg(AUDIT_APPARMOR_DENIED, &sa, NULL); - return -EINVAL; + goto out; } static int apparmor_task_setrlimit(struct task_struct *task,