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: <20071229.191443.10947724.davem@davemloft.net>
Date:	Sat, 29 Dec 2007 19:14:43 -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: Fri, 28 Dec 2007 22:48:57 +0100

> lockdep is worried about the different order here:
> 
> #1 (rose_neigh_list_lock){-+..}:
> #3 (ax25_list_lock){-+..}:
> 
> #0 (linkfail_lock){-+..}:
> #1 (rose_neigh_list_lock){-+..}:
> 
> #3 (ax25_list_lock){-+..}:
> #0 (linkfail_lock){-+..}:
> 
> So, ax25_list_lock could be taken before and after linkfail_lock. 
> I don't know if this three-thread clutch is very probable (or
> possible at all), but it seems another bug reported by Bernard
> ("[...] system impossible to reboot with linux-2.6.24-rc5")
> could have similar source - namely ax25_list_lock held by
> ax25_kill_by_device() during ax25_disconnect(). It looks like the
> only place which calls ax25_disconnect() this way, so I guess, it
> isn't necessary.
> 
> This patch is breaking the lock for ax25_disconnect(), with some
> failsafe and debugging added to detect unforeseen problems.
> 
> 
> Reported-and-tested-by: Bernard Pidoux <f6bvp@...e.fr>
> Signed-off-by: Jarek Poplawski <jarkao2@...il.com>

I can't apply this fix, sorry.

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.  And if it triggers it will do the wrong thing, because
by branching back to "again" we can call ax25_disconnect() multiple
times on the same entry which isn't right.

You'll thus need to resolve this locking conflict more properly.
I know it's hard, but your current fix is worse because it adds
a new known bug.
--
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