linux-kernel - [PATCH 1/4] x86/entry/64: Fix irqflag tracing wrt context tracking

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: <f3b6b3003f7a2d3304763394160dc7e32ff70d6c.1446849780.git.luto@kernel.org>
Date:	Fri,  6 Nov 2015 15:12:43 -0800
From:	Andy Lutomirski <luto@...nel.org>
To:	x86@...nel.org, linux-kernel@...r.kernel.org
Cc:	Brian Gerst <brgerst@...il.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Borislav Petkov <bp@...en8.de>,
	Frédéric Weisbecker <fweisbec@...il.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Andy Lutomirski <luto@...nel.org>
Subject: [PATCH 1/4] x86/entry/64: Fix irqflag tracing wrt context tracking

Paolo pointed out that enter_from_user_mode could be called while
irqflags were traced as though IRQs were on.

In principle, this could confuse lockdep.  It doesn't cause any
problems that I've seen in any configuration, but if I build with
CONFIG_DEBUG_LOCKDEP=y, enable a nohz_full CPU, and add code like:

	if (irqs_disabled()) {
		spin_lock(&something);
		spin_unlock(&something);
	}

to the top of enter_from_user_mode, then lockdep will complain
without this fix.  It seems that lockdep's irqflags sanity checks
are too weak to detect this bug without forcing the issue.

This patch adds one byte to normal kernels, and it's IMO a bit ugly.
I haven't spotted a better way to do this yet, though.  The issue is
that we can't do TRACE_IRQS_OFF until after SWAPGS (if needed), but
we're also supposed to do it before calling C code.

An alternative approach would be to call trace_hardirqs_off in
enter_from_user_mode.  That would be less code and would not bloat
normal kernels at all, but it would be harder to see how the code
worked.

Signed-off-by: Andy Lutomirski <luto@...nel.org>
---
 arch/x86/entry/entry_64.S | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/arch/x86/entry/entry_64.S b/arch/x86/entry/entry_64.S
index 53616ca03244..f585df24ab3d 100644
--- a/arch/x86/entry/entry_64.S
+++ b/arch/x86/entry/entry_64.S
@@ -509,6 +509,14 @@ END(irq_entries_start)
 	 * tracking that we're in kernel mode.
 	 */
 	SWAPGS
+
+	/*
+	 * IRQs are off.  NB: this trace call is duplicated.  That's
+	 * okay -- it's idempotent and it's irrelevant for performance as
+	 * it's a no-op unless CONFIG_DEBUG_LOCKDEP=y.
+	 */
+	TRACE_IRQS_OFF
+
 #ifdef CONFIG_CONTEXT_TRACKING
 	call enter_from_user_mode
 #endif
@@ -1049,12 +1057,13 @@ ENTRY(error_entry)
 	SWAPGS

 .Lerror_entry_from_usermode_after_swapgs:
+	TRACE_IRQS_OFF
 #ifdef CONFIG_CONTEXT_TRACKING
 	call enter_from_user_mode
 #endif
+	ret

 .Lerror_entry_done:
-
 	TRACE_IRQS_OFF
 	ret

-- 
2.4.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/