KASAN: use-after-free Read in smk_write_relabel_self

[syzbot]

Hello,

syzbot found the following crash on:

HEAD commit: acf25aa6 Merge tag 'Smack-for-5.8' of git://github.com/csc..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=11cd3ea6100000
kernel config: https://syzkaller.appspot.com/x/.config?x=3d66e6915128ae67
dashboard link: https://syzkaller.appspot.com/bug?extid=e6416dabb497a650da40
compiler: clang version 10.0.0 (https://github.com/llvm/llvm-project/ c2443155a0fb245c8f17f2c1c72b6ea391e86e81)
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=10654fd2100000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=107beea6100000

Bisection is inconclusive: the bug happens on the oldest tested release.

bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=129e16fe100000
final crash: https://syzkaller.appspot.com/x/report.txt?x=119e16fe100000
console output: https://syzkaller.appspot.com/x/log.txt?x=169e16fe100000

IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+e6416d...@syzkaller.appspotmail.com

==================================================================
BUG: KASAN: use-after-free in smk_destroy_label_list security/smack/smackfs.c:1975 [inline]
BUG: KASAN: use-after-free in smk_write_relabel_self+0x2f6/0x480 security/smack/smackfs.c:2748
Read of size 8 at addr ffff88809184bec0 by task syz-executor032/6852

CPU: 0 PID: 6852 Comm: syz-executor032 Not tainted 5.7.0-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x1e9/0x30e lib/dump_stack.c:118
print_address_description+0x66/0x5a0 mm/kasan/report.c:383
__kasan_report mm/kasan/report.c:513 [inline]
kasan_report+0x132/0x1d0 mm/kasan/report.c:530
smk_destroy_label_list security/smack/smackfs.c:1975 [inline]
smk_write_relabel_self+0x2f6/0x480 security/smack/smackfs.c:2748
__vfs_write+0x9c/0x6e0 fs/read_write.c:495
__kernel_write+0x120/0x350 fs/read_write.c:516
write_pipe_buf+0xf9/0x150 fs/splice.c:799
splice_from_pipe_feed fs/splice.c:502 [inline]
__splice_from_pipe+0x351/0x8b0 fs/splice.c:626
splice_from_pipe fs/splice.c:661 [inline]
default_file_splice_write fs/splice.c:811 [inline]
do_splice_from fs/splice.c:847 [inline]
direct_splice_actor+0x1eb/0x2a0 fs/splice.c:1016
splice_direct_to_actor+0x4a2/0xb60 fs/splice.c:971
do_splice_direct+0x201/0x340 fs/splice.c:1059
do_sendfile+0x809/0xfe0 fs/read_write.c:1521
__do_sys_sendfile64 fs/read_write.c:1582 [inline]
__se_sys_sendfile64 fs/read_write.c:1568 [inline]
__x64_sys_sendfile64+0x164/0x1a0 fs/read_write.c:1568
do_syscall_64+0xf3/0x1b0 arch/x86/entry/common.c:295
entry_SYSCALL_64_after_hwframe+0x49/0xb3
RIP: 0033:0x446a29
Code: e8 bc b4 02 00 48 83 c4 18 c3 0f 1f 80 00 00 00 00 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 ab 08 fc ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007f662054fdb8 EFLAGS: 00000246 ORIG_RAX: 0000000000000028
RAX: ffffffffffffffda RBX: 00000000006dbc88 RCX: 0000000000446a29
RDX: 0000000000000000 RSI: 0000000000000005 RDI: 0000000000000006
RBP: 00000000006dbc80 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000100000064 R11: 0000000000000246 R12: 00000000006dbc8c
R13: 00007fffa294e1ef R14: 00007f66205509c0 R15: 0000000000000001

Allocated by task 6850:
save_stack mm/kasan/common.c:48 [inline]
set_track mm/kasan/common.c:56 [inline]
__kasan_kmalloc+0x103/0x140 mm/kasan/common.c:494
kmem_cache_alloc_trace+0x234/0x300 mm/slab.c:3551
kmalloc include/linux/slab.h:555 [inline]
kzalloc include/linux/slab.h:669 [inline]
smk_parse_label_list+0xff/0x280 security/smack/smackfs.c:1955
smk_write_relabel_self+0x190/0x480 security/smack/smackfs.c:2744
__vfs_write+0x9c/0x6e0 fs/read_write.c:495
__kernel_write+0x120/0x350 fs/read_write.c:516
write_pipe_buf+0xf9/0x150 fs/splice.c:799
splice_from_pipe_feed fs/splice.c:502 [inline]
__splice_from_pipe+0x351/0x8b0 fs/splice.c:626
splice_from_pipe fs/splice.c:661 [inline]
default_file_splice_write fs/splice.c:811 [inline]
do_splice_from fs/splice.c:847 [inline]
direct_splice_actor+0x1eb/0x2a0 fs/splice.c:1016
splice_direct_to_actor+0x4a2/0xb60 fs/splice.c:971
do_splice_direct+0x201/0x340 fs/splice.c:1059
do_sendfile+0x809/0xfe0 fs/read_write.c:1521
__do_sys_sendfile64 fs/read_write.c:1582 [inline]
__se_sys_sendfile64 fs/read_write.c:1568 [inline]
__x64_sys_sendfile64+0x164/0x1a0 fs/read_write.c:1568
do_syscall_64+0xf3/0x1b0 arch/x86/entry/common.c:295
entry_SYSCALL_64_after_hwframe+0x49/0xb3

Freed by task 6850:
save_stack mm/kasan/common.c:48 [inline]
set_track mm/kasan/common.c:56 [inline]
kasan_set_free_info mm/kasan/common.c:316 [inline]
__kasan_slab_free+0x114/0x170 mm/kasan/common.c:455
__cache_free mm/slab.c:3426 [inline]
kfree+0x10a/0x220 mm/slab.c:3757
smk_destroy_label_list security/smack/smackfs.c:1976 [inline]
smk_write_relabel_self+0x302/0x480 security/smack/smackfs.c:2748
__vfs_write+0x9c/0x6e0 fs/read_write.c:495
__kernel_write+0x120/0x350 fs/read_write.c:516
write_pipe_buf+0xf9/0x150 fs/splice.c:799
splice_from_pipe_feed fs/splice.c:502 [inline]
__splice_from_pipe+0x351/0x8b0 fs/splice.c:626
splice_from_pipe fs/splice.c:661 [inline]
default_file_splice_write fs/splice.c:811 [inline]
do_splice_from fs/splice.c:847 [inline]
direct_splice_actor+0x1eb/0x2a0 fs/splice.c:1016
splice_direct_to_actor+0x4a2/0xb60 fs/splice.c:971
do_splice_direct+0x201/0x340 fs/splice.c:1059
do_sendfile+0x809/0xfe0 fs/read_write.c:1521
__do_sys_sendfile64 fs/read_write.c:1582 [inline]
__se_sys_sendfile64 fs/read_write.c:1568 [inline]
__x64_sys_sendfile64+0x164/0x1a0 fs/read_write.c:1568
do_syscall_64+0xf3/0x1b0 arch/x86/entry/common.c:295
entry_SYSCALL_64_after_hwframe+0x49/0xb3

The buggy address belongs to the object at ffff88809184bec0
which belongs to the cache kmalloc-32 of size 32
The buggy address is located 0 bytes inside of
32-byte region [ffff88809184bec0, ffff88809184bee0)
The buggy address belongs to the page:
page:ffffea00024612c0 refcount:1 mapcount:0 mapping:0000000000000000 index:0xffff88809184bfc1
flags: 0xfffe0000000200(slab)
raw: 00fffe0000000200 ffffea00029a8788 ffffea0002a351c8 ffff8880aa4001c0
raw: ffff88809184bfc1 ffff88809184b000 000000010000003f 0000000000000000
page dumped because: kasan: bad access detected

Memory state around the buggy address:
ffff88809184bd80: 00 00 00 fc fc fc fc fc fb fb fb fb fc fc fc fc
ffff88809184be00: 00 fc fc fc fc fc fc fc fb fb fb fb fc fc fc fc
>ffff88809184be80: fb fb fb fb fc fc fc fc fb fb fb fb fc fc fc fc
^
ffff88809184bf00: fb fb fb fb fc fc fc fc 00 00 00 fc fc fc fc fc
ffff88809184bf80: fb fb fb fb fc fc fc fc fc fc fc fc fc fc fc fc
==================================================================


---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzk...@googlegroups.com.

syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches

[Eric Biggers]

From: Eric Biggers <ebig...@google.com>

smk_write_relabel_self() frees memory from the task's credentials with
no locking, which can easily cause a use-after-free because multiple
tasks can share the same credentials structure.

Fix this by using prepare_creds() and commit_creds() to correctly modify
the task's credentials.

Reproducer for "BUG: KASAN: use-after-free in smk_write_relabel_self":

#include <fcntl.h>
#include <pthread.h>
#include <unistd.h>

static void *thrproc(void *arg)
{
int fd = open("/sys/fs/smackfs/relabel-self", O_WRONLY);
for (;;) write(fd, "foo", 3);
}

int main()
{
pthread_t t;
pthread_create(&t, NULL, thrproc, NULL);
thrproc(NULL);
}

Reported-by: syzbot+e6416d...@syzkaller.appspotmail.com
Fixes: 38416e53936e ("Smack: limited capability for changing process label")
Cc: <sta...@vger.kernel.org> # v4.4+
Signed-off-by: Eric Biggers <ebig...@google.com>
---
security/smack/smackfs.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/security/smack/smackfs.c b/security/smack/smackfs.c
index c21b656b3263..840a192e9337 100644
--- a/security/smack/smackfs.c
+++ b/security/smack/smackfs.c
@@ -2720,7 +2720,6 @@ static int smk_open_relabel_self(struct inode *inode, struct file *file)
static ssize_t smk_write_relabel_self(struct file *file, const char __user *buf,
size_t count, loff_t *ppos)
{
- struct task_smack *tsp = smack_cred(current_cred());
char *data;
int rc;
LIST_HEAD(list_tmp);
@@ -2745,11 +2744,21 @@ static ssize_t smk_write_relabel_self(struct file *file, const char __user *buf,
kfree(data);

if (!rc || (rc == -EINVAL && list_empty(&list_tmp))) {
+ struct cred *new;
+ struct task_smack *tsp;
+
+ new = prepare_creds();
+ if (!new) {
+ rc = -ENOMEM;
+ goto out;
+ }
+ tsp = smack_cred(new);
smk_destroy_label_list(&tsp->smk_relabel);
list_splice(&list_tmp, &tsp->smk_relabel);
+ commit_creds(new);
return count;
}
-
+out:
smk_destroy_label_list(&list_tmp);
return rc;
}
--
2.27.0

[Sasha Levin]

Hi

[This is an automated email]

This commit has been processed because it contains a "Fixes:" tag
fixing commit: 38416e53936e ("Smack: limited capability for changing process label").

The bot has tested the following trees: v5.7.8, v5.4.51, v4.19.132, v4.14.188, v4.9.230, v4.4.230.

v5.7.8: Build OK!
v5.4.51: Build OK!
v4.19.132: Failed to apply! Possible dependencies:
b17103a8b8ae9 ("Smack: Abstract use of cred security blob")

v4.14.188: Failed to apply! Possible dependencies:
03450e271a160 ("fs: add ksys_fchmod() and do_fchmodat() helpers and ksys_chmod() wrapper; remove in-kernel calls to syscall")
312db1aa1dc7b ("fs: add ksys_mount() helper; remove in-kernel calls to sys_mount()")
3a18ef5c1b393 ("fs: add ksys_umount() helper; remove in-kernel call to sys_umount()")
447016e968196 ("fs: add ksys_chdir() helper; remove in-kernel calls to sys_chdir()")
819671ff849b0 ("syscalls: define and explain goal to not call syscalls in the kernel")
9481769208b5e ("->file_open(): lose cred argument")
a16fe33ab5572 ("fs: add ksys_chroot() helper; remove-in kernel calls to sys_chroot()")
ae2bb293a3e8a ("get rid of cred argument of vfs_open() and do_dentry_open()")
af04fadcaa932 ("Revert "fs: fold open_check_o_direct into do_dentry_open"")
b17103a8b8ae9 ("Smack: Abstract use of cred security blob")
c7248321a3d42 ("fs: add ksys_dup{,3}() helper; remove in-kernel calls to sys_dup{,3}()")
d19dfe58b7ecb ("Smack: Privilege check on key operations")
dcb569cf6ac99 ("Smack: ptrace capability use fixes")
e3f20ae21079e ("security_file_open(): lose cred argument")
e7a3e8b2edf54 ("fs: add ksys_write() helper; remove in-kernel calls to sys_write()")

v4.9.230: Failed to apply! Possible dependencies:
078c73c63fb28 ("apparmor: add profile and ns params to aa_may_manage_policy()")
121d4a91e3c12 ("apparmor: rename sid to secid")
12557dcba21b0 ("apparmor: move lib definitions into separate lib include")
2bd8dbbf22fe9 ("apparmor: add ns being viewed as a param to policy_view_capable()")
5ac8c355ae001 ("apparmor: allow introspecting the loaded policy pre internal transform")
637f688dc3dc3 ("apparmor: switch from profiles to using labels on contexts")
73688d1ed0b8f ("apparmor: refactor prepare_ns() and make usable from different views")
9481769208b5e ("->file_open(): lose cred argument")
98849dff90e27 ("apparmor: rename namespace to ns to improve code line lengths")
9a2d40c12d00e ("apparmor: add strn version of aa_find_ns")
a6f233003b1af ("apparmor: allow specifying the profile doing the management")
b17103a8b8ae9 ("Smack: Abstract use of cred security blob")
cff281f6861e7 ("apparmor: split apparmor policy namespaces code into its own file")
d19dfe58b7ecb ("Smack: Privilege check on key operations")
dcb569cf6ac99 ("Smack: ptrace capability use fixes")
f28e783ff668c ("Smack: Use cap_capable in privilege check")
fd2a80438d736 ("apparmor: add ns being viewed as a param to policy_admin_capable()")
fe6bb31f590c9 ("apparmor: split out shared policy_XXX fns to lib")

v4.4.230: Failed to apply! Possible dependencies:
078c73c63fb28 ("apparmor: add profile and ns params to aa_may_manage_policy()")
121d4a91e3c12 ("apparmor: rename sid to secid")
12557dcba21b0 ("apparmor: move lib definitions into separate lib include")
2bd8dbbf22fe9 ("apparmor: add ns being viewed as a param to policy_view_capable()")
5ac8c355ae001 ("apparmor: allow introspecting the loaded policy pre internal transform")
637f688dc3dc3 ("apparmor: switch from profiles to using labels on contexts")
73688d1ed0b8f ("apparmor: refactor prepare_ns() and make usable from different views")
79be093500791 ("Smack: File receive for sockets")
9481769208b5e ("->file_open(): lose cred argument")
98849dff90e27 ("apparmor: rename namespace to ns to improve code line lengths")
9a2d40c12d00e ("apparmor: add strn version of aa_find_ns")
a6f233003b1af ("apparmor: allow specifying the profile doing the management")
b17103a8b8ae9 ("Smack: Abstract use of cred security blob")
c60b906673eeb ("Smack: Signal delivery as an append operation")
cff281f6861e7 ("apparmor: split apparmor policy namespaces code into its own file")
d19dfe58b7ecb ("Smack: Privilege check on key operations")
dcb569cf6ac99 ("Smack: ptrace capability use fixes")
f28e783ff668c ("Smack: Use cap_capable in privilege check")
fd2a80438d736 ("apparmor: add ns being viewed as a param to policy_admin_capable()")
fe6bb31f590c9 ("apparmor: split out shared policy_XXX fns to lib")


NOTE: The patch will not be queued to stable trees until it is upstream.

How should we proceed with this patch?

--
Thanks
Sasha

[Eric Biggers]

On Wed, Jul 08, 2020 at 01:15:20PM -0700, Eric Biggers wrote:
> From: Eric Biggers <ebig...@google.com>
>
> smk_write_relabel_self() frees memory from the task's credentials with
> no locking, which can easily cause a use-after-free because multiple
> tasks can share the same credentials structure.
>
> Fix this by using prepare_creds() and commit_creds() to correctly modify
> the task's credentials.
>
> Reproducer for "BUG: KASAN: use-after-free in smk_write_relabel_self":
>
> #include <fcntl.h>
> #include <pthread.h>
> #include <unistd.h>
>
> static void *thrproc(void *arg)
> {
> int fd = open("/sys/fs/smackfs/relabel-self", O_WRONLY);
> for (;;) write(fd, "foo", 3);
> }
>
> int main()
> {
> pthread_t t;
> pthread_create(&t, NULL, thrproc, NULL);
> thrproc(NULL);
> }
>
> Reported-by: syzbot+e6416d...@syzkaller.appspotmail.com
> Fixes: 38416e53936e ("Smack: limited capability for changing process label")
> Cc: <sta...@vger.kernel.org> # v4.4+
> Signed-off-by: Eric Biggers <ebig...@google.com>

Ping.

[Casey Schaufler]

I have queued your patch and will be pushing it for next shortly.