From 45e526e80e6fdc796d3bc05716d5c930a427df4d Mon Sep 17 00:00:00 2001 From: Nikolay Aleksandrov Date: Wed, 23 Oct 2013 15:04:49 +0200 Subject: netconsole: fix NULL pointer dereference We need to disable the netconsole (enabled = 0) before setting nt->np.dev to NULL because otherwise we might still have users after the netpoll_cleanup() since nt->enabled is set afterwards and we can have a message which will result in a NULL pointer dereference. It is very easy to hit dereferences all over the netpoll_send_udp function by running the following two loops in parallel: while [ 1 ]; do echo 1 > enabled; echo 0 > enabled; done; while [ 1 ]; do echo 00:11:22:33:44:55 > remote_mac; done; (the second loop is to generate messages, it can be done by anything) We're safe to set nt->np.dev = NULL and nt->enabled = 0 with the spinlock since it's required in the write_msg() function. Signed-off-by: Nikolay Aleksandrov Reviewed-by: Veacelsav Falico Signed-off-by: David S. Miller --- drivers/net/netconsole.c | 8 ++++++++ 1 file changed, 8 insertions(+) (limited to 'drivers/net/netconsole.c') diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c index adeee615dd19..1505dcb950fa 100644 --- a/drivers/net/netconsole.c +++ b/drivers/net/netconsole.c @@ -310,6 +310,7 @@ static ssize_t store_enabled(struct netconsole_target *nt, const char *buf, size_t count) { + unsigned long flags; int enabled; int err; @@ -342,6 +343,13 @@ static ssize_t store_enabled(struct netconsole_target *nt, printk(KERN_INFO "netconsole: network logging started\n"); } else { /* 0 */ + /* We need to disable the netconsole before cleaning it up + * otherwise we might end up in write_msg() with + * nt->np.dev == NULL and nt->enabled == 1 + */ + spin_lock_irqsave(&target_list_lock, flags); + nt->enabled = 0; + spin_unlock_irqrestore(&target_list_lock, flags); netpoll_cleanup(&nt->np); } -- cgit From c7c6effdeffcafd792b9f880ad52e48689eea4ad Mon Sep 17 00:00:00 2001 From: Nikolay Aleksandrov Date: Thu, 24 Oct 2013 12:09:24 +0200 Subject: netconsole: fix multiple race conditions In every netconsole option that can be set through configfs there's a race when checking for nt->enabled since it can be modified at the same time. Probably the most damage can be done by store_enabled when racing with another instance of itself. Fix all the races with one stone by moving the mutex lock around the ->store call for all options. Signed-off-by: Nikolay Aleksandrov Signed-off-by: David S. Miller --- drivers/net/netconsole.c | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) (limited to 'drivers/net/netconsole.c') diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c index 1505dcb950fa..c9a15925a1f7 100644 --- a/drivers/net/netconsole.c +++ b/drivers/net/netconsole.c @@ -325,9 +325,7 @@ static ssize_t store_enabled(struct netconsole_target *nt, return -EINVAL; } - mutex_lock(&nt->mutex); if (enabled) { /* 1 */ - /* * Skip netpoll_parse_options() -- all the attributes are * already configured via configfs. Just print them out. @@ -335,13 +333,10 @@ static ssize_t store_enabled(struct netconsole_target *nt, netpoll_print_options(&nt->np); err = netpoll_setup(&nt->np); - if (err) { - mutex_unlock(&nt->mutex); + if (err) return err; - } printk(KERN_INFO "netconsole: network logging started\n"); - } else { /* 0 */ /* We need to disable the netconsole before cleaning it up * otherwise we might end up in write_msg() with @@ -354,7 +349,6 @@ static ssize_t store_enabled(struct netconsole_target *nt, } nt->enabled = enabled; - mutex_unlock(&nt->mutex); return strnlen(buf, count); } @@ -571,8 +565,10 @@ static ssize_t netconsole_target_attr_store(struct config_item *item, struct netconsole_target_attr *na = container_of(attr, struct netconsole_target_attr, attr); + mutex_lock(&nt->mutex); if (na->store) ret = na->store(nt, buf, count); + mutex_unlock(&nt->mutex); return ret; } -- cgit