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

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 14/07/2019 18:21, Nadav Amit wrote:
>> 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.

Writes to these two registers should overwrite 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?)

TPR is only written during boot and resume.

cr8 is only read and written during suspend/resume, which is redundant
with the TPR save/restore.  Dropping that would drop two PVops, and the
final trace of the kernel using cr8 itself.

All cr8 and TPR accesses in KVM look to be on virtual state, rather than
the real registers (as expected).

~Andrew