| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <636563a2-8f73-4f08-b882-57b68fe7d0fc@I-love.SAKURA.ne.jp>
Date: Fri, 20 Dec 2024 20:56:16 +0900
From: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
To: syzbot <syzbot+bf6351831bc4f9148d98@...kaller.appspotmail.com>,
linux-kernel@...r.kernel.org, syzkaller-bugs@...glegroups.com
Subject: Re: [syzbot] [tomoyo?] general protection fault in tomoyo_gc_thread
tomoyo_try_to_gc() is called only when the ->is_deleted flag is set.
The ->is_deleted flag is set when a line in
delete $a_line_to_delete
format is written via the /sys/kernel/security/tomoyo/ interface.
But since I can't find the "delete " string within the syz reproducer,
tomoyo_try_to_gc() should not be called in the first place.
The syz reproducer is repeatedly opening /sys/kernel/security/tomoyo/manager
which in turn repeatedly triggers the gc thread. But unless the ->is_deleted
flag is set, gc has nothing to remove from the lists, and list corruption
cannot happen because list manipulation is serialized by tomoyo_policy_lock
mutex.
Therefore, currently I can't imagine that this report and "general protection
fault in tomoyo_check_acl (4)" and "BUG: corrupted list in tomoyo_try_to_gc"
are TOMOYO's bug; these look like a collateral victim caused by memory
corruption in somewhere else.
Anyway, let's try with lockdep annotation added.
#syz test
diff --git a/security/tomoyo/common.c b/security/tomoyo/common.c
index 972664962e8f..25273d35e1b1 100644
--- a/security/tomoyo/common.c
+++ b/security/tomoyo/common.c
@@ -348,6 +348,7 @@ void tomoyo_init_policy_namespace(struct tomoyo_policy_namespace *ns)
INIT_LIST_HEAD(&ns->policy_list[idx]);
ns->profile_version = 20150505;
tomoyo_namespace_enabled = !list_empty(&tomoyo_namespace_list);
+ lockdep_assert_held(&tomoyo_policy_lock);
list_add_tail_rcu(&ns->namespace_list, &tomoyo_namespace_list);
}
diff --git a/security/tomoyo/condition.c b/security/tomoyo/condition.c
index f8bcc083bb0d..f9e78deb51b2 100644
--- a/security/tomoyo/condition.c
+++ b/security/tomoyo/condition.c
@@ -427,6 +427,7 @@ static struct tomoyo_condition *tomoyo_commit_condition
if (!found) {
if (tomoyo_memory_ok(entry)) {
atomic_set(&entry->head.users, 1);
+ lockdep_assert_held(&tomoyo_policy_lock);
list_add(&entry->head.list, &tomoyo_condition_list);
} else {
found = true;
diff --git a/security/tomoyo/domain.c b/security/tomoyo/domain.c
index 3a7b0874cf44..b90728a6edc9 100644
--- a/security/tomoyo/domain.c
+++ b/security/tomoyo/domain.c
@@ -54,6 +54,7 @@ int tomoyo_update_policy(struct tomoyo_acl_head *new_entry, const int size,
if (error && !param->is_delete) {
entry = tomoyo_commit_ok(new_entry, size);
if (entry) {
+ lockdep_assert_held(&tomoyo_policy_lock);
list_add_tail_rcu(&entry->list, list);
error = 0;
}
@@ -138,6 +139,7 @@ int tomoyo_update_domain(struct tomoyo_acl_info *new_entry, const int size,
if (error && !is_delete) {
entry = tomoyo_commit_ok(new_entry, size);
if (entry) {
+ lockdep_assert_held(&tomoyo_policy_lock);
list_add_tail_rcu(&entry->list, list);
error = 0;
}
@@ -576,6 +578,7 @@ struct tomoyo_domain_info *tomoyo_assign_domain(const char *domainname,
entry = tomoyo_commit_ok(&e, sizeof(e));
if (entry) {
INIT_LIST_HEAD(&entry->acl_info_list);
+ lockdep_assert_held(&tomoyo_policy_lock);
list_add_tail_rcu(&entry->list, &tomoyo_domain_list);
created = true;
}
diff --git a/security/tomoyo/gc.c b/security/tomoyo/gc.c
index 026e29ea3796..ef3ed4fc71ca 100644
--- a/security/tomoyo/gc.c
+++ b/security/tomoyo/gc.c
@@ -29,6 +29,8 @@ static LIST_HEAD(tomoyo_io_buffer_list);
/* Lock for protecting tomoyo_io_buffer_list. */
static DEFINE_SPINLOCK(tomoyo_io_buffer_list_lock);
+static DEFINE_MUTEX(tomoyo_gc_mutex);
+
/**
* tomoyo_struct_used_by_io_buffer - Check whether the list element is used by /sys/kernel/security/tomoyo/ users or not.
*
@@ -392,8 +394,11 @@ static void tomoyo_try_to_gc(const enum tomoyo_policy_id type,
* tomoyo_domain_list). Also, synchronize_srcu() guarantees that the
* list element became no longer referenced by syscall users.
*/
+ struct list_head *head = element->prev;
+
__list_del_entry(element);
mutex_unlock(&tomoyo_policy_lock);
+ lockdep_assert_held(&tomoyo_gc_mutex);
synchronize_srcu(&tomoyo_ss);
/*
* However, there are two users which may still be using the list
@@ -470,7 +475,10 @@ static void tomoyo_try_to_gc(const enum tomoyo_policy_id type,
* exclusively executed by tomoyo_gc_mutex mutex.
* are true.
*/
+ lockdep_assert_held(&tomoyo_gc_mutex);
mutex_lock(&tomoyo_policy_lock);
+ printk("Reinjecting list=%px type=%d\n", element, type);
+ BUG_ON(element->prev != head);
list_add_rcu(element, element->prev);
}
@@ -612,7 +620,6 @@ static void tomoyo_collect_entry(void)
static int tomoyo_gc_thread(void *unused)
{
/* Garbage collector thread is exclusive. */
- static DEFINE_MUTEX(tomoyo_gc_mutex);
if (!mutex_trylock(&tomoyo_gc_mutex))
goto out;
diff --git a/security/tomoyo/memory.c b/security/tomoyo/memory.c
index 1b570bde7a3b..e70c33576349 100644
--- a/security/tomoyo/memory.c
+++ b/security/tomoyo/memory.c
@@ -123,6 +123,7 @@ struct tomoyo_group *tomoyo_get_group(struct tomoyo_acl_param *param,
if (entry) {
INIT_LIST_HEAD(&entry->member_list);
atomic_set(&entry->head.users, 1);
+ lockdep_assert_held(&tomoyo_policy_lock);
list_add_tail_rcu(&entry->head.list, list);
group = entry;
found = true;
@@ -176,6 +177,7 @@ const struct tomoyo_path_info *tomoyo_get_name(const char *name)
memmove((char *) ptr->entry.name, name, len);
atomic_set(&ptr->head.users, 1);
tomoyo_fill_path_info(&ptr->entry);
+ lockdep_assert_held(&tomoyo_policy_lock);
list_add_tail(&ptr->head.list, head);
} else {
kfree(ptr);
@@ -199,7 +201,9 @@ void __init tomoyo_mm_init(void)
for (idx = 0; idx < TOMOYO_MAX_HASH; idx++)
INIT_LIST_HEAD(&tomoyo_name_list[idx]);
tomoyo_kernel_namespace.name = "<kernel>";
+ mutex_lock(&tomoyo_policy_lock);
tomoyo_init_policy_namespace(&tomoyo_kernel_namespace);
+ mutex_unlock(&tomoyo_policy_lock);
tomoyo_kernel_domain.ns = &tomoyo_kernel_namespace;
INIT_LIST_HEAD(&tomoyo_kernel_domain.acl_info_list);
tomoyo_kernel_domain.domainname = tomoyo_get_name("<kernel>");
Powered by blists - more mailing lists