netdev - Re: [PATCH net] netconsole: fix NULL pointer dereference

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20131024102147.GD16787@redhat.com>
Date:	Thu, 24 Oct 2013 12:21:47 +0200
From:	Veaceslav Falico <vfalico@...hat.com>
To:	Nikolay Aleksandrov <nikolay@...hat.com>
Cc:	netdev@...r.kernel.org, davem@...emloft.net
Subject: Re: [PATCH net] netconsole: fix NULL pointer dereference

On Wed, Oct 23, 2013 at 03:04:49PM +0200, Nikolay Aleksandrov wrote:
>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 <nikolay@...hat.com>
>---
>Taking the spinlock seems like the cleanest way to insure there's noone
>running in parallel, but I'm open to suggestions as I'm not satisfied with
>the looks of this. I'll prepare a net-next patchset for netconsole soon to
>clean it up properly, all of these can be easily simplified.

First when I've seen 'spin_lock(); a = 1; spin_unlock()' I've thought
"WTF?", however indeed it will stop us racing with write_msg().

FWIW...

Reviewed-by: Veacelsav Falico <vfalico@...hat.com>

>
> drivers/net/netconsole.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
>diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
>index adeee61..1505dcb 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);
> 	}
>
>-- 
>1.8.1.4
>
>--
>To unsubscribe from this list: send the line "unsubscribe netdev" in
>the body of a message to majordomo@...r.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html