Commit 6c2fc091 authored by Andrey Ryabinin's avatar Andrey Ryabinin Committed by Ben Hutchings

smack: fix possible use after frees in task_security() callers

commit 6d1cff2a upstream.

We hit use after free on dereferncing pointer to task_smack struct in
smk_of_task() called from smack_task_to_inode().

task_security() macro uses task_cred_xxx() to get pointer to the task_smack.
task_cred_xxx() could be used only for non-pointer members of task's
credentials. It cannot be used for pointer members since what they point
to may disapper after dropping RCU read lock.

Mainly task_security() used this way:
	smk_of_task(task_security(p))

Intead of this introduce function smk_of_task_struct() which
takes task_struct as argument and returns pointer to smk_known struct
and do this under RCU read lock.
Bogus task_security() macro is not used anymore, so remove it.

KASan's report for this:

	AddressSanitizer: use after free in smack_task_to_inode+0x50/0x70 at addr c4635600
	=============================================================================
	BUG kmalloc-64 (Tainted: PO): kasan error
	-----------------------------------------------------------------------------

	Disabling lock debugging due to kernel taint
	INFO: Allocated in new_task_smack+0x44/0xd8 age=39 cpu=0 pid=1866
		kmem_cache_alloc_trace+0x88/0x1bc
		new_task_smack+0x44/0xd8
		smack_cred_prepare+0x48/0x21c
		security_prepare_creds+0x44/0x4c
		prepare_creds+0xdc/0x110
		smack_setprocattr+0x104/0x150
		security_setprocattr+0x4c/0x54
		proc_pid_attr_write+0x12c/0x194
		vfs_write+0x1b0/0x370
		SyS_write+0x5c/0x94
		ret_fast_syscall+0x0/0x48
	INFO: Freed in smack_cred_free+0xc4/0xd0 age=27 cpu=0 pid=1564
		kfree+0x270/0x290
		smack_cred_free+0xc4/0xd0
		security_cred_free+0x34/0x3c
		put_cred_rcu+0x58/0xcc
		rcu_process_callbacks+0x738/0x998
		__do_softirq+0x264/0x4cc
		do_softirq+0x94/0xf4
		irq_exit+0xbc/0x120
		handle_IRQ+0x104/0x134
		gic_handle_irq+0x70/0xac
		__irq_svc+0x44/0x78
		_raw_spin_unlock+0x18/0x48
		sync_inodes_sb+0x17c/0x1d8
		sync_filesystem+0xac/0xfc
		vdfs_file_fsync+0x90/0xc0
		vfs_fsync_range+0x74/0x7c
	INFO: Slab 0xd3b23f50 objects=32 used=31 fp=0xc4635600 flags=0x4080
	INFO: Object 0xc4635600 @offset=5632 fp=0x  (null)

	Bytes b4 c46355f0: 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a  ZZZZZZZZZZZZZZZZ
	Object c4635600: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
	Object c4635610: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
	Object c4635620: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b  kkkkkkkkkkkkkkkk
	Object c4635630: 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b 6b a5  kkkkkkkkkkkkkkk.
	Redzone c4635640: bb bb bb bb                                      ....
	Padding c46356e8: 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a 5a  ZZZZZZZZZZZZZZZZ
	Padding c46356f8: 5a 5a 5a 5a 5a 5a 5a 5a                          ZZZZZZZZ
	CPU: 5 PID: 834 Comm: launchpad_prelo Tainted: PBO 3.10.30 #1
	Backtrace:
	[<c00233a4>] (dump_backtrace+0x0/0x158) from [<c0023dec>] (show_stack+0x20/0x24)
	 r7:c4634010 r6:d3b23f50 r5:c4635600 r4:d1002140
	[<c0023dcc>] (show_stack+0x0/0x24) from [<c06d6d7c>] (dump_stack+0x20/0x28)
	[<c06d6d5c>] (dump_stack+0x0/0x28) from [<c01c1d50>] (print_trailer+0x124/0x144)
	[<c01c1c2c>] (print_trailer+0x0/0x144) from [<c01c1e88>] (object_err+0x3c/0x44)
	 r7:c4635600 r6:d1002140 r5:d3b23f50 r4:c4635600
	[<c01c1e4c>] (object_err+0x0/0x44) from [<c01cac18>] (kasan_report_error+0x2b8/0x538)
	 r6:d1002140 r5:d3b23f50 r4:c6429cf8 r3:c09e1aa7
	[<c01ca960>] (kasan_report_error+0x0/0x538) from [<c01c9430>] (__asan_load4+0xd4/0xf8)
	[<c01c935c>] (__asan_load4+0x0/0xf8) from [<c031e168>] (smack_task_to_inode+0x50/0x70)
	 r5:c4635600 r4:ca9da000
	[<c031e118>] (smack_task_to_inode+0x0/0x70) from [<c031af64>] (security_task_to_inode+0x3c/0x44)
	 r5:cca25e80 r4:c0ba9780
	[<c031af28>] (security_task_to_inode+0x0/0x44) from [<c023d614>] (pid_revalidate+0x124/0x178)
	 r6:00000000 r5:cca25e80 r4:cbabe3c0 r3:00008124
	[<c023d4f0>] (pid_revalidate+0x0/0x178) from [<c01db98c>] (lookup_fast+0x35c/0x43y4)
	 r9:c6429efc r8:00000101 r7:c079d940 r6:c6429e90 r5:c6429ed8 r4:c83c4148
	[<c01db630>] (lookup_fast+0x0/0x434) from [<c01deec8>] (do_last.isra.24+0x1c0/0x1108)
	[<c01ded08>] (do_last.isra.24+0x0/0x1108) from [<c01dff04>] (path_openat.isra.25+0xf4/0x648)
	[<c01dfe10>] (path_openat.isra.25+0x0/0x648) from [<c01e1458>] (do_filp_open+0x3c/0x88)
	[<c01e141c>] (do_filp_open+0x0/0x88) from [<c01ccb28>] (do_sys_open+0xf0/0x198)
	 r7:00000001 r6:c0ea2180 r5:0000000b r4:00000000
	[<c01cca38>] (do_sys_open+0x0/0x198) from [<c01ccc00>] (SyS_open+0x30/0x34)
	[<c01ccbd0>] (SyS_open+0x0/0x34) from [<c001db80>] (ret_fast_syscall+0x0/0x48)
	Read of size 4 by thread T834:
	Memory state around the buggy address:
	 c4635380: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
	 c4635400: 00 00 00 00 00 00 00 00 fc fc fc fc fc fc fc fc
	 c4635480: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
	 c4635500: 00 00 00 00 00 fc fc fc fc fc fc fc fc fc fc fc
	 c4635580: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
	>c4635600: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
	           ^
	 c4635680: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
	 c4635700: 00 00 00 00 04 fc fc fc fc fc fc fc fc fc fc fc
	 c4635780: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
	 c4635800: 00 00 00 00 00 00 04 fc fc fc fc fc fc fc fc fc
	 c4635880: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
	==================================================================
Signed-off-by: default avatarAndrey Ryabinin <a.ryabinin@samsung.com>
[bwh: Backported to 3.2:
 - smk_of_task() and similar functions return char * not struct smack_known *
 - The callers of task_security() are quite different, but most can be changed
   to use smk_of_task_struct() just as in the upstream version
 - Use open-coded RCU locking in the one place using smk_of_forked() instead
   of smk_of_task()]
Signed-off-by: default avatarBen Hutchings <ben@decadent.org.uk>
parent 1b0539bb
...@@ -272,6 +272,16 @@ static inline char *smk_of_task(const struct task_smack *tsp) ...@@ -272,6 +272,16 @@ static inline char *smk_of_task(const struct task_smack *tsp)
return tsp->smk_task; return tsp->smk_task;
} }
static inline char *smk_of_task_struct(const struct task_struct *t)
{
char *skp;
rcu_read_lock();
skp = smk_of_task(__task_cred(t)->security);
rcu_read_unlock();
return skp;
}
/* /*
* Present a pointer to the forked smack label in an task blob. * Present a pointer to the forked smack label in an task blob.
*/ */
......
...@@ -38,8 +38,6 @@ ...@@ -38,8 +38,6 @@
#include <linux/personality.h> #include <linux/personality.h>
#include "smack.h" #include "smack.h"
#define task_security(task) (task_cred_xxx((task), security))
#define TRANS_TRUE "TRUE" #define TRANS_TRUE "TRUE"
#define TRANS_TRUE_SIZE 4 #define TRANS_TRUE_SIZE 4
...@@ -161,7 +159,7 @@ static int smack_ptrace_access_check(struct task_struct *ctp, unsigned int mode) ...@@ -161,7 +159,7 @@ static int smack_ptrace_access_check(struct task_struct *ctp, unsigned int mode)
if (rc != 0) if (rc != 0)
return rc; return rc;
tsp = smk_of_task(task_security(ctp)); tsp = smk_of_task_struct(ctp);
smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_TASK); smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_TASK);
smk_ad_setfield_u_tsk(&ad, ctp); smk_ad_setfield_u_tsk(&ad, ctp);
...@@ -187,7 +185,7 @@ static int smack_ptrace_traceme(struct task_struct *ptp) ...@@ -187,7 +185,7 @@ static int smack_ptrace_traceme(struct task_struct *ptp)
if (rc != 0) if (rc != 0)
return rc; return rc;
tsp = smk_of_task(task_security(ptp)); tsp = smk_of_task_struct(ptp);
smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_TASK); smk_ad_init(&ad, __func__, LSM_AUDIT_DATA_TASK);
smk_ad_setfield_u_tsk(&ad, ptp); smk_ad_setfield_u_tsk(&ad, ptp);
...@@ -1515,7 +1513,7 @@ static int smk_curacc_on_task(struct task_struct *p, int access, ...@@ -1515,7 +1513,7 @@ static int smk_curacc_on_task(struct task_struct *p, int access,
smk_ad_init(&ad, caller, LSM_AUDIT_DATA_TASK); smk_ad_init(&ad, caller, LSM_AUDIT_DATA_TASK);
smk_ad_setfield_u_tsk(&ad, p); smk_ad_setfield_u_tsk(&ad, p);
return smk_curacc(smk_of_task(task_security(p)), access, &ad); return smk_curacc(smk_of_task_struct(p), access, &ad);
} }
/** /**
...@@ -1561,7 +1559,7 @@ static int smack_task_getsid(struct task_struct *p) ...@@ -1561,7 +1559,7 @@ static int smack_task_getsid(struct task_struct *p)
*/ */
static void smack_task_getsecid(struct task_struct *p, u32 *secid) static void smack_task_getsecid(struct task_struct *p, u32 *secid)
{ {
*secid = smack_to_secid(smk_of_task(task_security(p))); *secid = smack_to_secid(smk_of_task_struct(p));
} }
/** /**
...@@ -1673,7 +1671,7 @@ static int smack_task_kill(struct task_struct *p, struct siginfo *info, ...@@ -1673,7 +1671,7 @@ static int smack_task_kill(struct task_struct *p, struct siginfo *info,
* can write the receiver. * can write the receiver.
*/ */
if (secid == 0) if (secid == 0)
return smk_curacc(smk_of_task(task_security(p)), MAY_WRITE, return smk_curacc(smk_of_task_struct(p), MAY_WRITE,
&ad); &ad);
/* /*
* If the secid isn't 0 we're dealing with some USB IO * If the secid isn't 0 we're dealing with some USB IO
...@@ -1681,7 +1679,7 @@ static int smack_task_kill(struct task_struct *p, struct siginfo *info, ...@@ -1681,7 +1679,7 @@ static int smack_task_kill(struct task_struct *p, struct siginfo *info,
* we can't take privilege into account. * we can't take privilege into account.
*/ */
return smk_access(smack_from_secid(secid), return smk_access(smack_from_secid(secid),
smk_of_task(task_security(p)), MAY_WRITE, &ad); smk_of_task_struct(p), MAY_WRITE, &ad);
} }
/** /**
...@@ -1694,9 +1692,13 @@ static int smack_task_wait(struct task_struct *p) ...@@ -1694,9 +1692,13 @@ static int smack_task_wait(struct task_struct *p)
{ {
struct smk_audit_info ad; struct smk_audit_info ad;
char *sp = smk_of_current(); char *sp = smk_of_current();
char *tsp = smk_of_forked(task_security(p)); char *tsp;
int rc; int rc;
rcu_read_lock();
tsp = smk_of_forked(__task_cred(p)->security);
rcu_read_unlock();
/* we don't log here, we can be overriden */ /* we don't log here, we can be overriden */
rc = smk_access(tsp, sp, MAY_WRITE, NULL); rc = smk_access(tsp, sp, MAY_WRITE, NULL);
if (rc == 0) if (rc == 0)
...@@ -1733,7 +1735,7 @@ static int smack_task_wait(struct task_struct *p) ...@@ -1733,7 +1735,7 @@ static int smack_task_wait(struct task_struct *p)
static void smack_task_to_inode(struct task_struct *p, struct inode *inode) static void smack_task_to_inode(struct task_struct *p, struct inode *inode)
{ {
struct inode_smack *isp = inode->i_security; struct inode_smack *isp = inode->i_security;
isp->smk_inode = smk_of_task(task_security(p)); isp->smk_inode = smk_of_task_struct(p);
} }
/* /*
...@@ -2714,7 +2716,7 @@ static int smack_getprocattr(struct task_struct *p, char *name, char **value) ...@@ -2714,7 +2716,7 @@ static int smack_getprocattr(struct task_struct *p, char *name, char **value)
if (strcmp(name, "current") != 0) if (strcmp(name, "current") != 0)
return -EINVAL; return -EINVAL;
cp = kstrdup(smk_of_task(task_security(p)), GFP_KERNEL); cp = kstrdup(smk_of_task_struct(p), GFP_KERNEL);
if (cp == NULL) if (cp == NULL)
return -ENOMEM; return -ENOMEM;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment