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

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

[Andy Lutomirski]

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();
-- 
2.21.0

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

[Nadav Amit]

> 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?)

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

[Andrew Cooper]

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

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

[tip-bot for Andy Lutomirski]

Commit-ID:  229b969b3d38bc28bcd55841ee7ca9a9afb922f3
Gitweb:     https://git.kernel.org/tip/229b969b3d38bc28bcd55841ee7ca9a9afb922f3
Author:     Andy Lutomirski <luto@kernel.org>
AuthorDate: Sun, 14 Jul 2019 08:23:14 -0700
Committer:  Thomas Gleixner <tglx@linutronix.de>
CommitDate: Mon, 22 Jul 2019 10:12:32 +0200

x86/apic: Initialize TPR to block interrupts 16-31

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 robustness can be
improved 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 change is to reduce the chance that a
certain class of device malfunctions crashes the kernel in hard-to-debug
ways.

Suggested-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Link: https://lkml.kernel.org/r/dc04a9f8b234d7b0956a8d2560b8945bcd9c4bf7.1563117760.git.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 f5291362da1a..84032bf81476 100644
--- a/arch/x86/kernel/apic/apic.c
+++ b/arch/x86/kernel/apic/apic.c
@@ -1561,11 +1561,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();