From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mx.groups.io with SMTP id smtpd.web10.26916.1683621328444440049 for ; Tue, 09 May 2023 01:35:28 -0700 Authentication-Results: mx.groups.io; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=EcVp2HrL; spf=pass (domain: redhat.com, ip: 170.10.133.124, mailfrom: lersek@redhat.com) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1683621327; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=LFU3n2gZkGcDLKz/aFzhGBM17D5kohhzJQuYFsmQWZo=; b=EcVp2HrL4D6iu4caml32FIIRDSwgwESqrV5TX02HeCrxP9GUGjQBJmg1aXEXmoj3bHrlt2 VmRetzUL26Ys3UxxKwzaDRjqJqINPF1lhVjevYoR5Eh0qDfHy6qBeqPq7l3t3ldUrVpHoo WsBIrV4Nh8vcQGiCuI3mqd0LULNJAcY= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-564-UtjDpT_7MUWcbq7a5No3dw-1; Tue, 09 May 2023 04:35:26 -0400 X-MC-Unique: UtjDpT_7MUWcbq7a5No3dw-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 09AC7800047; Tue, 9 May 2023 08:35:26 +0000 (UTC) Received: from [10.39.192.152] (unknown [10.39.192.152]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 4232F63F84; Tue, 9 May 2023 08:35:25 +0000 (UTC) Message-ID: Date: Tue, 9 May 2023 10:35:24 +0200 MIME-Version: 1.0 Subject: Re: [PATCH 2/2] OvmfPkg: Relax assertion that interrupts do not occur at TPL_HIGH_LEVEL To: Michael Brown , devel@edk2.groups.io Cc: Gerd Hoffmann References: <476bbc17-6484-9afd-9be9-08de14d1d72e@redhat.com> <20230508213100.3949708-1-mcb30@ipxe.org> <01020187fd46e7d4-9dba3e15-efcc-4ea7-87ba-2391a3f372d5-000000@eu-west-1.amazonses.com> From: "Laszlo Ersek" In-Reply-To: <01020187fd46e7d4-9dba3e15-efcc-4ea7-87ba-2391a3f372d5-000000@eu-west-1.amazonses.com> X-Scanned-By: MIMEDefang 3.1 on 10.11.54.5 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit On 5/8/23 23:31, Michael Brown wrote: > At TPL_HIGH_LEVEL, CPU interrupts are disabled (as per the UEFI > specification) and so we should never encounter a situation in which > an interrupt occurs at TPL_HIGH_LEVEL. The specification also > restricts usage of TPL_HIGH_LEVEL to the firmware itself. Great introduction! Regarding the rest of the commit message, I'd like us to tone it down a little bit. Here's why: I'd been used to Microsoft *not* cooperating usefully in Windows-on-QEMU/KVM situations. But this instance was totally different. In fact I'm still a bit shocked, in the positive sense. We got a fast and helpful, to-the-point response. It's a first, considering my own experience, and it has strongly changed my impression of Microsoft's Windows team. I'd like us to acknowledge that in the commit message, if possible. Mind you, I'm not a native English speaker, so I could be seeing things (and proposing worse language than the original). With all that said: > However, nothing prevents a rogue UEFI application from illegally I request s/rogue/non-conformant/. I'd also request "invalidly" rather than "illegally"; the latter has connotations with the law, and seeing such in a commit message makes me fidget uncomfortably. I do apologize. > calling gBS->RaiseTPL(TPL_HIGH_LEVEL) and then deliberately violating You are not wrong about "deliberately", but I'd still like us to remove that word. :) > the invariant by enabling interrupts via the STI or equivalent > instruction. Some versions of the Microsoft Windows bootloader are > known to do this. > > NestedInterruptTplLib maintains the invariant that interrupts are > disabled at TPL_HIGH_LEVEL (even when performing the dark art of > deliberately manipulating the stack so that IRET will return with > interrupts still disabled), but does not itself rely on external code > maintaining this invariant. > > Relax the assertion that the interrupted TPL is below TPL_HIGH_LEVEL > to an error message, to allow rogue UEFI applications such as the s/rogue/non-conformant/ > Microsoft Windows bootloader to continue to function. Can we say "particular version of the Microsoft Windows bootloader"? > > Debugged-by: Gerd Hoffmann > Debugged-by: Laszlo Ersek > Ref: https://bugzilla.redhat.com/show_bug.cgi?id=2189136 > Signed-off-by: Michael Brown > --- > OvmfPkg/Library/NestedInterruptTplLib/Tpl.c | 21 ++++++++++++++++++--- > 1 file changed, 18 insertions(+), 3 deletions(-) > > diff --git a/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c b/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c > index e921a09c5599..a91f2d3cb8c7 100644 > --- a/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c > +++ b/OvmfPkg/Library/NestedInterruptTplLib/Tpl.c > @@ -34,12 +34,27 @@ NestedInterruptRaiseTPL ( > > // > // Raise TPL and assert that we were called from within an interrupt > - // handler (i.e. with TPL below TPL_HIGH_LEVEL but with interrupts > - // disabled). > + // handler (i.e. with interrupts already disabled before raising the > + // TPL). > // > ASSERT (GetInterruptState () == FALSE); > InterruptedTPL = gBS->RaiseTPL (TPL_HIGH_LEVEL); > - ASSERT (InterruptedTPL < TPL_HIGH_LEVEL); > + > + // > + // At TPL_HIGH_LEVEL, CPU interrupts are disabled (as per the UEFI > + // specification) and so we should never encounter a situation in > + // which InterruptedTPL==TPL_HIGH_LEVEL. The specification also > + // restricts usage of TPL_HIGH_LEVEL to the firmware itself. > + // > + // However, nothing prevents a rogue UEFI application from illegally > + // calling gBS->RaiseTPL(TPL_HIGH_LEVEL) and then deliberately > + // violating the invariant by enabling interrupts via the STI or > + // equivalent instruction. Some versions of the Microsoft Windows > + // bootloader are known to do this. > + // Same three requests on the wording: rogue, illegally, deliberately. (I am happy with "Some versions of"!) > + if (InterruptedTPL >= TPL_HIGH_LEVEL) { > + DEBUG ((DEBUG_ERROR, "Illegal interrupt at TPL_HIGH_LEVEL!\n")); s/Illegal/Invalid/ please! :) > + } > > return InterruptedTPL; > } Thank you for the patch; I do apologize about splitting hairs. The debugging was difficult, and you *are* working around a bug here -- but I'd really like our tone of voice to be positive here, simply because of the stunningly positive attitude I've experienced from Microsoft. Thanks! Laszlo