Re: [PATCH] x86/apic: Initialize TPR to block interrupts 16-31

[Nadav Amit <namit@vmware.com>]

> On Jul 14, 2019, at 8:23 AM, Andy Lutomirski <luto@kernel.org> wrote:
> 
> The APIC, per spec, is fundamentally confused and thinks that
> interrupt vectors 16-31 are valid.  This makes no sense -- the CPU
> reserves vectors 0-31 for exceptions (faults, traps, etc).
> Obviously, no device should actually produce an interrupt with
> vector 16-31, but we can improve robustness by setting the APIC TPR
> class to 1, which will prevent delivery of an interrupt with a
> vector below 32.
> 
> Note: this is *not* intended as a security measure against attackers
> who control malicious hardware.  Any PCI or similar hardware that
> can be controlled by an attacker MUST be behind a functional IOMMU
> that remaps interrupts.  The purpose of this patch is to reduce the
> chance that a certain class of device malfunctions crashes the
> kernel in hard-to-debug ways.
> 
> Cc: Nadav Amit <namit@vmware.com>
> Cc: Stephane Eranian <eranian@google.com>
> Cc: Feng Tang <feng.tang@intel.com>
> Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---
> arch/x86/kernel/apic/apic.c | 7 +++++--
> 1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/kernel/apic/apic.c b/arch/x86/kernel/apic/apic.c
> index 177aa8ef2afa..ff31322f8839 100644
> --- a/arch/x86/kernel/apic/apic.c
> +++ b/arch/x86/kernel/apic/apic.c
> @@ -1531,11 +1531,14 @@ static void setup_local_APIC(void)
> #endif
> 
> 	/*
> -	 * Set Task Priority to 'accept all'. We never change this
> -	 * later on.
> +	 * Set Task Priority to 'accept all except vectors 0-31'.  An APIC
> +	 * vector in the 16-31 range could be delivered if TPR == 0, but we
> +	 * would think it's an exception and terrible things will happen.  We
> +	 * never change this later on.
> 	 */
> 	value = apic_read(APIC_TASKPRI);
> 	value &= ~APIC_TPRI_MASK;
> +	value |= 0x10;
> 	apic_write(APIC_TASKPRI, value);
> 
> 	apic_pending_intr_clear();

It looks fine, and indeed it seems that writes to APIC_TASKPRI and CR8 are
not overwriting this value.

Yet, the fact that if someone overwrites with zero (or does not restore) the
TPR will not produce any warning is not that great. Not that I know what the
right course of action is (adding checks in write_cr8()? but then what about
if APIC_TASKPRI is not restored after some APIC reset?)