netdev - Re: [PATCH][ROSE][AX25] af_ax25: possible circular locking

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: <20080110.212242.42433023.davem@davemloft.net>
Date:	Thu, 10 Jan 2008 21:22:42 -0800 (PST)
From:	David Miller <davem@...emloft.net>
To:	jarkao2@...il.com
Cc:	f6bvp@...e.fr, ralf@...ux-mips.org, adobriyan@...il.com,
	netdev@...r.kernel.org
Subject: Re: [PATCH][ROSE][AX25] af_ax25: possible circular locking

From: Jarek Poplawski <jarkao2@...il.com>
Date: Sun, 30 Dec 2007 15:13:23 +0100

> On Sat, Dec 29, 2007 at 07:14:43PM -0800, David Miller wrote:
> ...
> > You can't just drop this linked list lock and expect it to stay
> > consistent like that.
> > 
> > Once you drop it, any thread of control can get in there and delete
> > entries from the list.
> > 
> > Since we know it can happen, using a WARN_ON_ONCE(1) is not
> > appropriate.
> 
> The problem is 'we' don't know if it can happen... In the first
> message with this patch I've tried to get this information, and
> now it seems you are the only one with this knowledge, but of
> course this is more than enough for me to agree with your decision
> to dump this patch.

I've removed the warning and made the branch back to 'again'
unconditional as I think this is the safest version of the
change.

I'll push this upstream, thanks for fixing this Jarek.

diff --git a/net/ax25/af_ax25.c b/net/ax25/af_ax25.c
index ecb14ee..b4725ff 100644
--- a/net/ax25/af_ax25.c
+++ b/net/ax25/af_ax25.c
@@ -87,10 +87,22 @@ static void ax25_kill_by_device(struct net_device *dev)
 		return;
 
 	spin_lock_bh(&ax25_list_lock);
+again:
 	ax25_for_each(s, node, &ax25_list) {
 		if (s->ax25_dev == ax25_dev) {
 			s->ax25_dev = NULL;
+			spin_unlock_bh(&ax25_list_lock);
 			ax25_disconnect(s, ENETUNREACH);
+			spin_lock_bh(&ax25_list_lock);
+
+			/* The entry could have been deleted from the
+			 * list meanwhile and thus the next pointer is
+			 * no longer valid.  Play it safe and restart
+			 * the scan.  Forward progress is ensured
+			 * because we set s->ax25_dev to NULL and we
+			 * are never passed a NULL 'dev' argument.
+			 */
+			goto again;
 		}
 	}
 	spin_unlock_bh(&ax25_list_lock);
-- 
1.5.4.rc2.84.gf85fd

--
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