| 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: <20161002031349.GB2635@templeofstupid.com>
Date: Sat, 1 Oct 2016 20:13:49 -0700
From: Krister Johansen <kjlx@...pleofstupid.com>
To: Jamal Hadi Salim <jhs@...atatu.com>
Cc: netdev@...r.kernel.org
Subject: [PATCH net] Panic when tc_lookup_action_n finds a partially
initialized action.
A tc_action_ops structure is visibile as soon as it is placed in the
act_base list. When tcf_regsiter_action adds an item to this list and
drops act_mod_lock, registration is not complete until
register_pernet_subsys() finishes.
If two threads attempt to modify a tc action in a way that triggers a
module load, the thread that wins the race ends up defeferencing a NULL
pointer after tcf_action_init_1() invokes a_o->init(). In the
particular case that this submitter encountered, the panic occurred in
tcf_gact_init() when net_generic() returned a NULL tc_action_net
pointer. The gact_net_id needed to fetch the correct pointer was not
yet set, because the register_pernet_subsys() call was pending in
another thread.
Fixes: ddf97ccdd7cb ("net_sched: add network namespace support for tc actions")
Signed-off-by: Krister Johansen <kjlx@...pleofstupid.com>
---
include/net/act_api.h | 1 +
net/sched/act_api.c | 64 ++++++++++++++++++++++++++++++++++++++++++++-------
2 files changed, 57 insertions(+), 8 deletions(-)
diff --git a/include/net/act_api.h b/include/net/act_api.h
index 82f3c91..9ede72d 100644
--- a/include/net/act_api.h
+++ b/include/net/act_api.h
@@ -107,6 +107,7 @@ struct tc_action_ops {
struct list_head head;
char kind[IFNAMSIZ];
__u32 type; /* TBD to match kind */
+ __u32 pernet_pending;
size_t size;
struct module *owner;
int (*act)(struct sk_buff *, const struct tc_action *,
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index d09d068..3b51808 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -331,6 +331,7 @@ EXPORT_SYMBOL(tcf_hashinfo_destroy);
static LIST_HEAD(act_base);
static DEFINE_RWLOCK(act_mod_lock);
+static DECLARE_WAIT_QUEUE_HEAD(act_mod_wait);
int tcf_register_action(struct tc_action_ops *act,
struct pernet_operations *ops)
@@ -349,15 +350,18 @@ int tcf_register_action(struct tc_action_ops *act,
}
}
list_add_tail(&act->head, &act_base);
+ act->pernet_pending = 1;
write_unlock(&act_mod_lock);
ret = register_pernet_subsys(ops);
- if (ret) {
- tcf_unregister_action(act, ops);
- return ret;
- }
+ write_lock(&act_mod_lock);
+ if (ret)
+ list_del(&act->head);
+ act->pernet_pending = 0;
+ write_unlock(&act_mod_lock);
+ wake_up_all(&act_mod_wait);
- return 0;
+ return ret;
}
EXPORT_SYMBOL(tcf_register_action);
@@ -367,8 +371,6 @@ int tcf_unregister_action(struct tc_action_ops *act,
struct tc_action_ops *a;
int err = -ENOENT;
- unregister_pernet_subsys(ops);
-
write_lock(&act_mod_lock);
list_for_each_entry(a, &act_base, head) {
if (a == act) {
@@ -378,10 +380,49 @@ int tcf_unregister_action(struct tc_action_ops *act,
}
}
write_unlock(&act_mod_lock);
+
+ if (!err)
+ unregister_pernet_subsys(ops);
+
return err;
}
EXPORT_SYMBOL(tcf_unregister_action);
+static int tcf_module_needed(char *kind)
+{
+ struct tc_action_ops *a;
+ int rc = 0;
+ DEFINE_WAIT_FUNC(wait, woken_wake_function);
+
+ if (!kind)
+ return rc;
+
+ add_wait_queue(&act_mod_wait, &wait);
+ read_lock(&act_mod_lock);
+ for (;;) {
+ list_for_each_entry(a, &act_base, head) {
+ if (strcmp(kind, a->kind) == 0)
+ break;
+ }
+
+ if (!a || strcmp(kind, a->kind) != 0) {
+ rc = 1;
+ break;
+ }
+
+ if (a->pernet_pending == 0 || signal_pending(current))
+ break;
+
+ read_unlock(&act_mod_lock);
+ wait_woken(&wait, TASK_INTERRUPTIBLE, MAX_SCHEDULE_TIMEOUT);
+ read_lock(&act_mod_lock);
+ }
+ read_unlock(&act_mod_lock);
+ remove_wait_queue(&act_mod_wait, &wait);
+
+ return rc;
+}
+
/* lookup by name */
static struct tc_action_ops *tc_lookup_action_n(char *kind)
{
@@ -391,11 +432,14 @@ static struct tc_action_ops *tc_lookup_action_n(char *kind)
read_lock(&act_mod_lock);
list_for_each_entry(a, &act_base, head) {
if (strcmp(kind, a->kind) == 0) {
+ if (a->pernet_pending != 0)
+ goto out;
if (try_module_get(a->owner))
res = a;
break;
}
}
+out:
read_unlock(&act_mod_lock);
}
return res;
@@ -410,11 +454,14 @@ static struct tc_action_ops *tc_lookup_action(struct nlattr *kind)
read_lock(&act_mod_lock);
list_for_each_entry(a, &act_base, head) {
if (nla_strcmp(kind, a->kind) == 0) {
+ if (a->pernet_pending != 0)
+ goto out;
if (try_module_get(a->owner))
res = a;
break;
}
}
+out:
read_unlock(&act_mod_lock);
}
return res;
@@ -549,7 +596,8 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
if (a_o == NULL) {
#ifdef CONFIG_MODULES
rtnl_unlock();
- request_module("act_%s", act_name);
+ if (tcf_module_needed(act_name))
+ request_module("act_%s", act_name);
rtnl_lock();
a_o = tc_lookup_action_n(act_name);
--
2.7.4
Powered by blists - more mailing lists