8000 apparmor: fix oops, validate buffer size in apparmor_setprocattr() · bsd-unix/linux@30a46a4 · GitHub
[go: up one dir, main page]
Skip to content

Commit 30a46a4

Browse files
vegardJames Morris
vegard
authored and
James Morris
committed
apparmor: fix oops, validate buffer size in apparmor_setprocattr()
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 <vegard.nossum@oracle.com> modified for the case that apparmor provides null termination. Fixes: bb646cd Reported-by: Vegard Nossum <vegard.nossum@oracle.com> Cc: Al Viro <viro@zeniv.linux.org.uk> Cc: John Johansen <john.johansen@canonical.com> Cc: Paul Moore <paul@paul-moore.com> Cc: Stephen Smalley <sds@tycho.nsa.gov> Cc: Eric Paris <eparis@parisplace.org> Cc: Casey Schaufler <casey@schaufler-ca.com> Cc: stable@kernel.org Signed-off-by: John Johansen <john.johansen@canonical.com> Reviewed-by: Tyler Hicks <tyhicks@canonical.com> Signed-off-by: James Morris <james.l.morris@oracle.com>
1 parent ac904ae commit 30a46a4

File tree

1 file changed

+19
-17
lines changed

1 file changed

+19
-17
lines changed

‎security/apparmor/lsm.c

Lines changed: 19 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -500,34 +500,34 @@ static int apparmor_setprocattr(struct task_struct *task, char *name,
500500
{
501501
struct common_audit_data sa;
502502
struct apparmor_audit_data aad = {0,};
503-
char *command, *args = value;
503+
char *command, *largs = NULL, *args = value;
504504
size_t arg_size;
505505
int error;
506506

507507
if (size == 0)
508508
return -EINVAL;
509-
/* args points to a PAGE_SIZE buffer, AppArmor requires that
510-
* the buffer must be null terminated or have size <= PAGE_SIZE -1
511-
* so that AppArmor can null terminate them
512-
*/
513-
if (args[size - 1] != '\0') {
514-
if (size == PAGE_SIZE)
515-
return -EINVAL;
516-
args[size] = '\0';
517-
}
518-
519509
/* task can only write its own attributes */
520510
if (current != task)
521511
return -EACCES;
522512

523-
args = value;
513+
/* AppArmor requires that the buffer must be null terminated atm */
514+
if (args[size - 1] != '\0') {
515+
/* null terminate */
516+
largs = args = kmalloc(size + 1, GFP_KERNEL);
517+
if (!args)
518+
return -ENOMEM;
519+
memcpy(args, value, size);
520+
args[size] = '\0';
521+
}
522+
523+
error = -EINVAL;
524524
args = strim(args);
525525
command = strsep(&args, " ");
526526
if (!args)
527-
return -EINVAL;
527+
goto out;
528528
args = skip_spaces(args);
529529
if (!*args)
530-
return -EINVAL;
530+
goto out;
531531

532532
arg_size = size - (args - (char *) value);
533533
if (strcmp(name, "current") == 0) {
@@ -553,10 +553,12 @@ static int apparmor_setprocattr(struct task_struct *task, char *name,
553553
goto fail;
554554
} else
555555
/* only support the "current" and "exec" process attributes */
556-
return -EINVAL;
556+
goto fail;
557557

558558
if (!error)
559559
error = size;
560+
out:
561+
kfree(largs);
560562
return error;
561563

562564
fail:
@@ -565,9 +567,9 @@ static int apparmor_setprocattr(struct task_struct *task, char *name,
565567
aad.profile = aa_current_profile();
566568
aad.op = OP_SETPROCATTR;
567569
aad.info = name;
568-
aad.error = -EINVAL;
570+
aad.error = error = -EINVAL;
569571
aa_audit_msg(AUDIT_APPARMOR_DENIED, &sa, NULL);
570-
return -EINVAL;
572+
goto out;
571573
}
572574

573575
static int apparmor_task_setrlimit(struct task_struct *task,

0 commit comments

Comments
 (0)
0