linux-kernel - Re: [PATCH 02/31] x86: MCE: Improve mce_get

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]

Date:	Wed, 27 May 2009 13:29:16 +0900
From:	Hidetoshi Seto <seto.hidetoshi@...fujitsu.com>
To:	Andi Kleen <andi@...stfloor.org>
CC:	linux-kernel@...r.kernel.org, hpa@...or.com, x86@...nel.org,
	Huang Ying <ying.huang@...el.com>,
	Andi Kleen <ak@...ux.intel.com>
Subject: Re: [PATCH 02/31] x86: MCE: Improve mce_get_rip v3

Andi Kleen wrote:
> From: Huang Ying <ying.huang@...el.com>
> 
> Assume RIP is valid when either EIPV or RIPV are set.

Bad description.
If RIP means "restart IP" that is valid only if RIPV is set,
this sentence doesn't make sense completely.

Followings will be better:
"Assume IP stored on the stack indicates the address of instruction
 at the time of the MCE when either EIPV or RIPV are set."

> This influences
> whether the machine check exception handler decides to return or panic.

I suppose you are pointing logics in:

                mce_get_rip(&m, regs);
 :
                        panicm = m;
 :
                /*
                 * If the EIPV bit is set, it means the saved IP is the
                 * instruction which caused the MCE.
                 */
                if (m.mcgstatus & MCG_STATUS_EIPV)
                        user_space = panicm.ip && (panicm.cs & 3);

                /*
                 * If we know that the error was in user space, send a
                 * SIGBUS.  Otherwise, panic if tolerance is low.
                 *
                 * force_sig() takes an awful lot of locks and has a slight
                 * risk of deadlocking.
                 */
                if (user_space) {
                        force_sig(SIGBUS, current);
                } else if (panic_on_oops || tolerant < 2) {
                        mce_panic("Uncorrected machine check",
                                &panicm, mcestart);
                }

So EIPV without RIPV will be no ip and will result in panic,
while expected result is SIGBUS. 

> This fixes a test case in the mce-test suite and is more compliant
> to the specification.

It would be better to describe about the test case that need this fix.

> This currently only makes a difference in a artificial testing
> scenario with the mce-test test suite.
> 
> Also in addition do not force the RIP to be valid with the exact
> register MSRs.

I think the forced one is EIP:
> -		m->mcgstatus |= MCG_STATUS_EIPV;

And please note that it keep use CS on stack even if MSR is available.

I made an alternative patch for this, with no functional change.
Please consider replacing.


Thanks,
H.Seto

> [AK: combination of patches from Huang Ying and Hidetoshi Seto, with
> new description by me]
> Signed-off-by: Huang Ying <ying.huang@...el.com>
> Signed-off-by: Andi Kleen <ak@...ux.intel.com>
> Signed-off-by: Hidetoshi Seto <seto.hidetoshi@...fujitsu.com>
> ---
>  arch/x86/kernel/cpu/mcheck/mce.c |   13 +++++++------
>  1 files changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> index 249e3cf..3f158d7 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> @@ -247,21 +247,22 @@ int mce_available(struct cpuinfo_x86 *c)
>  	return cpu_has(c, X86_FEATURE_MCE) && cpu_has(c, X86_FEATURE_MCA);
>  }
>  
> +/*
> + * Get the address of the instruction at the time of the machine check
> + * error.
> + */
>  static inline void mce_get_rip(struct mce *m, struct pt_regs *regs)
>  {
> -	if (regs && (m->mcgstatus & MCG_STATUS_RIPV)) {
> +
> +	if (regs && (m->mcgstatus & (MCG_STATUS_RIPV|MCG_STATUS_EIPV))) {
>  		m->ip = regs->ip;
>  		m->cs = regs->cs;
>  	} else {
>  		m->ip = 0;
>  		m->cs = 0;
>  	}
> -	if (rip_msr) {
> -		/* Assume the RIP in the MSR is exact. Is this true? */
> -		m->mcgstatus |= MCG_STATUS_EIPV;
> +	if (rip_msr)
>  		m->ip = mce_rdmsrl(rip_msr);
> -		m->cs = 0;
> -	}
>  }
>  
>  /*

--
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/