linux-kernel - Re: [PATCH] x86/mce: Initialize "bank" when we find a fatal error in mce_no_way

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: <20190201183616.GA24278@agluck-desk>
Date:   Fri, 1 Feb 2019 10:36:17 -0800
From:   "Luck, Tony" <tony.luck@...el.com>
To:     Borislav Petkov <bp@...en8.de>
Cc:     x86@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] x86/mce: Initialize "bank" when we find a fatal error in
 mce_no_way_out()

On Fri, Feb 01, 2019 at 10:55:53AM +0100, Borislav Petkov wrote:
> On Thu, Jan 31, 2019 at 04:33:41PM -0800, Tony Luck wrote:
> >  		if (mce_severity(m, mca_cfg.tolerant, &tmp, true) >= MCE_PANIC_SEVERITY) {
> > +			m->bank = i;
> 
> So conceptually this write belongs in...
> 
> >  			mce_read_aux(m, i);

Really? That function starts with the comment

	/*
	 * Read ADDR and MISC registers.
	 */

and the name is pretty descriptive that it is "reading auxiliary" information.

> 
> ... this function, i.e., in mce_read_aux() because it gets the bank
> number passed in already. And our calling pattern when populating struct
> mce is:
> 
> 	mce_gather_info()
> 	mce_read_aux()

Only two exising uses:

1) in machine_check_poll()

	mce_gather_info(&m, NULL);
	for (each bank) {
		m.bank = i;
		mce_read_aux(&m, i);
		...
	}

2) in __mc_scan_banks

	// mce_gather_info() already done in do_machine_check
	for (each bank) {
		m.bank = i;
		mce_read_aux(m, i);
		...
	}
> 
> so it'll be more robust if we moved it there.

It would be redundant to move it there for both
existing uses.

> Also, that argument "i" of mce_read_aux() is not very telling and it
> should be "bank" but that would complicate the stable backporting so if
> you feel like it, you could do a second, cleanup patch ontop to fix that
> too.

Agreed that "bank" is clearer than "i". But also agreed that should
be a separate patch for another day.

-Tony