From: Eric Biggers ebiggers@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+e6416dabb497a650da40@syzkaller.appspotmail.com Fixes: 38416e53936e ("Smack: limited capability for changing process label") Cc: stable@vger.kernel.org # v4.4+ Signed-off-by: Eric Biggers ebiggers@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; }
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?
On Wed, Jul 08, 2020 at 01:15:20PM -0700, Eric Biggers wrote:
From: Eric Biggers ebiggers@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+e6416dabb497a650da40@syzkaller.appspotmail.com Fixes: 38416e53936e ("Smack: limited capability for changing process label") Cc: stable@vger.kernel.org # v4.4+ Signed-off-by: Eric Biggers ebiggers@google.com
Ping.
On 7/20/2020 5:38 PM, Eric Biggers wrote:
On Wed, Jul 08, 2020 at 01:15:20PM -0700, Eric Biggers wrote:
From: Eric Biggers ebiggers@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+e6416dabb497a650da40@syzkaller.appspotmail.com Fixes: 38416e53936e ("Smack: limited capability for changing process label") Cc: stable@vger.kernel.org # v4.4+ Signed-off-by: Eric Biggers ebiggers@google.com
Ping.
I have queued your patch and will be pushing it for next shortly.
linux-stable-mirror@lists.linaro.org