Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Nadav Amit <namit@vmware.com>
To:     Andy Lutomirski <luto@kernel.org>
CC:     LKML <linux-kernel@vger.kernel.org>,
        "x86@kernel.org" <x86@kernel.org>, Borislav Petkov <bp@alien8.de>,
        Peter Zijlstra <peterz@infradead.org>,
        Stephane Eranian <eranian@google.com>,
        Feng Tang <feng.tang@intel.com>,
        Andrew Cooper <andrew.cooper3@citrix.com>
Subject: Re: [PATCH] x86/apic: Initialize TPR to block interrupts 16-31
Thread-Topic: [PATCH] x86/apic: Initialize TPR to block interrupts 16-31
Thread-Index: AQHVOliSQEatmYzQkESWBh33bdcvFqbKXGiA
Date:   Sun, 14 Jul 2019 17:21:19 +0000
Message-ID: <D7C365E9-0DAD-46B9-95C9-B3475879F813@vmware.com>
References: <dc04a9f8b234d7b0956a8d2560b8945bcd9c4bf7.1563117760.git.luto@kernel.org>
In-Reply-To: <dc04a9f8b234d7b0956a8d2560b8945bcd9c4bf7.1563117760.git.luto@kernel.org>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: vmware.com does not designate
 permitted sender hosts)
Content-Type: text/plain; charset="us-ascii"
Content-ID: <E0460F700268A2409E411A9F98948096@namprd05.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 47ae56fe-bfce-4a25-e0d2-08d7087fafcc
X-MS-Exchange-CrossTenant-originalarrivaltime: 14 Jul 2019 17:21:19.8123
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: b39138ca-3cee-4b4a-a4d6-cd83d9dd62f0
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: namit@vmware.com
X-MS-Exchange-Transport-CrossTenantHeadersStamped: BYAPR05MB4631
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> On Jul 14, 2019, at 8:23 AM, Andy Lutomirski <luto@kernel.org> wrote:
>=20
> 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.
>=20
> 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.
>=20
> 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(-)
>=20
> 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
>=20
> 	/*
> -	 * 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 =3D=3D 0, but we
> +	 * would think it's an exception and terrible things will happen.  We
> +	 * never change this later on.
> 	 */
> 	value =3D apic_read(APIC_TASKPRI);
> 	value &=3D ~APIC_TPRI_MASK;
> +	value |=3D 0x10;
> 	apic_write(APIC_TASKPRI, value);
>=20
> 	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) th=
e
TPR will not produce any warning is not that great. Not that I know what th=
e
right course of action is (adding checks in write_cr8()? but then what abou=
t
if APIC_TASKPRI is not restored after some APIC reset?)