linux-kernel - Re: [PATCH 4/6] x86/irq: Process nmi sources in NMI handler

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: <0bac819f-fdbe-4de2-8a5f-30ded87bb036@zytor.com>
Date: Wed, 29 May 2024 13:47:09 -0700
From: "H. Peter Anvin" <hpa@...or.com>
To: Jacob Pan <jacob.jun.pan@...ux.intel.com>, X86 Kernel <x86@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Dave Hansen <dave.hansen@...el.com>, Ingo Molnar <mingo@...hat.com>,
        Borislav Petkov <bp@...en8.de>, linux-perf-users@...r.kernel.org,
        Peter Zijlstra <peterz@...radead.org>
Cc: Andi Kleen <andi.kleen@...el.com>, Xin Li <xin3.li@...el.com>
Subject: Re: [PATCH 4/6] x86/irq: Process nmi sources in NMI handler

On 5/29/24 13:33, Jacob Pan wrote:
> +
> +	/*
> +	 * Per NMI source specification, there is no guarantee that a valid
> +	 * NMI vector is always delivered, even when the source specified
> +	 * one. It is software's responsibility to check all available NMI
> +	 * sources when bit 0 is set in the NMI source bitmap. i.e. we have
> +	 * to call every handler as if we have no NMI source.
> +	 * On the other hand, if we do get non-zero vectors, we know exactly
> +	 * what the sources are. So we only call the handlers with the bit set.
> +	 */
> +	if (source_bitmask & BIT(NMI_SOURCE_VEC_UNKNOWN)) {
> +		pr_warn_ratelimited("NMI received with unknown source\n");
> +		return 0;
> +	}
> +

Note: if bit 0 is set, you can process any other bits first (on the 
general assumption that if you bother with NMI source then those events 
are performance sensitive), and you could even exclude them from the 
poll. This is an optimization, and what you have here is correct from a 
functional point of view.

> +	source_bitmask = fred_event_data(regs);
> +	if (!source_bitmask) {
> +		pr_warn_ratelimited("NMI received without source information!\n");
> +		return 0;
> +	}

If the event data word is 0, it probably should be treated as a 
*permanent* failure, as it is a Should Not Happen[TM] situation, and 
means there is an implementation (or, perhaps more likely, 
virtualization!) bug, and as such it may not be safe to trust the NMI 
source information in the future.

> +	if (!cpu_feature_enabled(X86_FEATURE_NMI_SOURCE) || type != NMI_LOCAL)
> +		return 0;

I'm not sure I understand why you are requiring type to be NMI_LOCAL here?

	-hpa