| Message ID | 20191210000733.17979-5-jschoenh@amazon.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | x86/mce: Various fixes and cleanups for MCE handling | expand |
On Tue, Dec 10, 2019 at 01:07:31AM +0100, Jan H. Schönherr wrote: > The function mce_severity() is not required to update its msg argument. > In fact, mce_severity_amd() doesn't. Fix some code paths, that assume > that it is always updated. > > In particular, this avoids returning uninitialized data in > mce_no_way_out(), which may be used later for printing. > > Signed-off-by: Jan H. Schönherr <jschoenh@amazon.de> > --- > arch/x86/kernel/cpu/mce/core.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c > index 6afb9de251f2..b11a74e3fea9 100644 > --- a/arch/x86/kernel/cpu/mce/core.c > +++ b/arch/x86/kernel/cpu/mce/core.c > @@ -809,10 +809,12 @@ static int mce_no_way_out(struct mce *m, char **msg, unsigned long *validp, > if (quirk_no_way_out) > quirk_no_way_out(i, m, regs); > > + tmp = NULL; > m->bank = i; > if (mce_severity(m, mca_cfg.tolerant, &tmp, true) >= MCE_PANIC_SEVERITY) { > mce_read_aux(m, i); > - *msg = tmp; > + if (tmp) > + *msg = tmp; > return 1; > } > } > @@ -1313,6 +1315,7 @@ void do_machine_check(struct pt_regs *regs, long error_code) > * make sure we have the right "msg". > */ > if (worst >= MCE_PANIC_SEVERITY && mca_cfg.tolerant < 3) { > + msg = "Unknown"; > mce_severity(&m, cfg->tolerant, &msg, true); > mce_panic("Local fatal machine check!", &m, msg); > } > -- Can we get rid of all that silliness of dealing with a possibly uninitialized pointer in the callers and simply do at the beginning of mce_panic(): if (!msg) msg = "Unknown"; ?
On 16/12/2019 18.37, Borislav Petkov wrote: > On Tue, Dec 10, 2019 at 01:07:31AM +0100, Jan H. Schönherr wrote: >> diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c >> index 6afb9de251f2..b11a74e3fea9 100644 >> --- a/arch/x86/kernel/cpu/mce/core.c >> +++ b/arch/x86/kernel/cpu/mce/core.c >> @@ -809,10 +809,12 @@ static int mce_no_way_out(struct mce *m, char **msg, unsigned long *validp, >> if (quirk_no_way_out) >> quirk_no_way_out(i, m, regs); >> >> + tmp = NULL; >> m->bank = i; >> if (mce_severity(m, mca_cfg.tolerant, &tmp, true) >= MCE_PANIC_SEVERITY) { >> mce_read_aux(m, i); >> - *msg = tmp; >> + if (tmp) >> + *msg = tmp; >> return 1; >> } >> } >> @@ -1313,6 +1315,7 @@ void do_machine_check(struct pt_regs *regs, long error_code) >> * make sure we have the right "msg". >> */ >> if (worst >= MCE_PANIC_SEVERITY && mca_cfg.tolerant < 3) { >> + msg = "Unknown"; >> mce_severity(&m, cfg->tolerant, &msg, true); >> mce_panic("Local fatal machine check!", &m, msg); >> } >> -- > > Can we get rid of all that silliness of dealing with a possibly > uninitialized pointer in the callers and simply do at the beginning of > mce_panic(): > > if (!msg) > msg = "Unknown"; > > ? > Not quite. mce_panic() already handles NULL as a value for "exp" (not "msg"). We still need to pass NULL or a proper pointer. Not some uninitialized, potentially random data. So, at the very least we need to initialize "tmp" in mce_no_way_out(), if you're looking for a minimal patch. This would turn the (non-existing) description of the "msg" argument of mce_severity() from an assumed: "an implementation may or may not update a provided *msg argument" to: "an implementation must either always update a provided *msg argument, or it must never do so" Regards Jan -- Amazon Development Center Germany GmbH Krausenstr. 38 10117 Berlin Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B Sitz: Berlin Ust-ID: DE 289 237 879
On Thu, Dec 19, 2019 at 06:49:32PM +0100, Jan H. Schönherr wrote: > Not quite. mce_panic() already handles NULL as a value for "exp" (not "msg"). > > We still need to pass NULL or a proper pointer. Not some uninitialized, potentially > random data. > > So, at the very least we need to initialize "tmp" in mce_no_way_out(), if you're looking > for a minimal patch. Yes, sure, this is what I'm thinking of. And yes, I'm not going to cry if we don't print the immensely helpful "Unknown" anymore.</sarcasm> --- diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c index 68dd4b358740..fd23f9f53379 100644 --- a/arch/x86/kernel/cpu/mce/core.c +++ b/arch/x86/kernel/cpu/mce/core.c @@ -798,7 +798,7 @@ EXPORT_SYMBOL_GPL(machine_check_poll); static int mce_no_way_out(struct mce *m, char **msg, unsigned long *validp, struct pt_regs *regs) { - char *tmp; + char *tmp = NULL; int i; for (i = 0; i < this_cpu_read(mce_num_banks); i++) { @@ -1223,8 +1223,8 @@ void do_machine_check(struct pt_regs *regs, long error_code) DECLARE_BITMAP(toclear, MAX_NR_BANKS); struct mca_config *cfg = &mca_cfg; int cpu = smp_processor_id(); - char *msg = "Unknown"; struct mce m, *final; + char *msg = NULL; int worst = 0; /*
diff --git a/arch/x86/kernel/cpu/mce/core.c b/arch/x86/kernel/cpu/mce/core.c index 6afb9de251f2..b11a74e3fea9 100644 --- a/arch/x86/kernel/cpu/mce/core.c +++ b/arch/x86/kernel/cpu/mce/core.c @@ -809,10 +809,12 @@ static int mce_no_way_out(struct mce *m, char **msg, unsigned long *validp, if (quirk_no_way_out) quirk_no_way_out(i, m, regs); + tmp = NULL; m->bank = i; if (mce_severity(m, mca_cfg.tolerant, &tmp, true) >= MCE_PANIC_SEVERITY) { mce_read_aux(m, i); - *msg = tmp; + if (tmp) + *msg = tmp; return 1; } } @@ -1313,6 +1315,7 @@ void do_machine_check(struct pt_regs *regs, long error_code) * make sure we have the right "msg". */ if (worst >= MCE_PANIC_SEVERITY && mca_cfg.tolerant < 3) { + msg = "Unknown"; mce_severity(&m, cfg->tolerant, &msg, true); mce_panic("Local fatal machine check!", &m, msg); }
The function mce_severity() is not required to update its msg argument. In fact, mce_severity_amd() doesn't. Fix some code paths, that assume that it is always updated. In particular, this avoids returning uninitialized data in mce_no_way_out(), which may be used later for printing. Signed-off-by: Jan H. Schönherr <jschoenh@amazon.de> --- arch/x86/kernel/cpu/mce/core.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)