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 for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <alpine.DEB.2.20.1612262224400.3687@nanos>
Date:   Mon, 26 Dec 2016 22:42:01 +0100 (CET)
From:   Thomas Gleixner <tglx@...utronix.de>
To:     Borislav Petkov <bp@...en8.de>
cc:     Boris Ostrovsky <boris.ostrovsky@...cle.com>,
        Markus Trippelsdorf <markus@...ppelsdorf.de>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Ingo Molnar <mingo@...nel.org>,
        "H. Peter Anvin" <hpa@...or.com>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Subject: Re: [GIT pull] smp/hotplug: Removal of notifiers

On Mon, 26 Dec 2016, Thomas Gleixner wrote:
> On Mon, 26 Dec 2016, Borislav Petkov wrote:
> > On Mon, Dec 26, 2016 at 07:21:44PM +0100, Thomas Gleixner wrote:
> > > Is there anything interesting error message before the BUG hits? I'll try
> > > to reproduce on a AMD box tomorrow.
> > 
> > Hmm, so lemme see if I see it correctly:
> > 
> > threshold_create_bank() does kobject_create_and_add(name, &dev->kobj);
> > and that dev thing is
> > 
> > 	struct device *dev = per_cpu(mce_device, cpu);
> > 
> > BUT(!), those mce_device per-CPU things get initialized in
> > 
> > mce_cpu_online()
> > |-> mce_device_create(cpu);
> > 
> > With a CONFIG_HOTPLUG_CPU=n .config that doesn't happen, right?
> > 
> > Oh, and I see what could've changed that:
> > 
> >   8c0eeac819c8 ("x86/mcheck: Move CPU_ONLINE and CPU_DOWN_PREPARE to hotplug state machine")
> > 
> > And before that, we did call mce_device_create(cpu) in
> > mcheck_init_device() which is a device initcall and not dependent on CPU
> > hotplug.
> > 
> > And frankly, flipping back to the for_each_online_cpu(i) is yucky as
> > hell but I don't see any other/better solution besides pulling up
> > mce_device_create() into mcheck_init_device()...
> 
> The hotplug callbacks are invoked even with HOTPLUG=n. So that's not the
> problem. I can reproduce it. Will post info once I understand it.

So the issue is indeed in that commit. I'm a moron.

But the amd mce code should be made more solid, because exactly that issue
can happen when something goes wrong in mcheck_init_device(). If that
happens then the device pointer is NULL and this code crashes. Adding the
NULL pointer check makes the machine survive despite the wreckage in the
hotplug code.

Fix below.

Thanks,

	tglx

8<---------------------------

arch/x86/kernel/cpu/mcheck/mce_amd.c |    3 +++
 kernel/cpu.c                         |    9 ++++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

--- a/arch/x86/kernel/cpu/mcheck/mce_amd.c
+++ b/arch/x86/kernel/cpu/mcheck/mce_amd.c
@@ -1182,6 +1182,9 @@ static int threshold_create_bank(unsigne
 	const char *name = get_name(bank, NULL);
 	int err = 0;
 
+	if (!dev)
+		return -ENODEV;
+
 	if (is_shared_bank(bank)) {
 		nb = node_to_amd_nb(amd_get_nb_id(cpu));
 
--- a/kernel/cpu.c
+++ b/kernel/cpu.c
@@ -1471,6 +1471,7 @@ int __cpuhp_setup_state(enum cpuhp_state
 			bool multi_instance)
 {
 	int cpu, ret = 0;
+	bool dynstate;
 
 	if (cpuhp_cb_check(state) || !name)
 		return -EINVAL;
@@ -1480,6 +1481,12 @@ int __cpuhp_setup_state(enum cpuhp_state
 	ret = cpuhp_store_callbacks(state, name, startup, teardown,
 				    multi_instance);
 
+	dynstate = state == CPUHP_AP_ONLINE_DYN;
+	if (ret > 0 && dynstate) {
+		state = ret;
+		ret = 0;
+	}
+
 	if (ret || !invoke || !startup)
 		goto out;
 
@@ -1508,7 +1515,7 @@ int __cpuhp_setup_state(enum cpuhp_state
 	 * If the requested state is CPUHP_AP_ONLINE_DYN, return the
 	 * dynamically allocated state in case of success.
 	 */
-	if (!ret && state == CPUHP_AP_ONLINE_DYN)
+	if (!ret && dynstate)
 		return state;
 	return ret;
 }

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ