Re: [Qemu-devel] [PATCH 23/31] ich9: implement SMI_LOCK

[Laszlo Ersek <lersek@redhat.com>]

On 05/11/15 17:21, Paolo Bonzini wrote:
> 
> 
> On 11/05/2015 17:17, Laszlo Ersek wrote:
>> If I understand correctly, this makes SMI_LOCK lock down the GBL_SMI_EN
>> bit (and my OVMF patch that relies on that / tests it is satisfied too).
>>
>> But, it doesn't seem to lock down APMC_EN. According to the ICH9 spec,
>> it doesn't need to -- however when we discussed this earlier (see
>> Message-Id: <553F4D23.3060305@redhat.com>), the idea was to lock down
>> APMC_EN as well. (And I don't understand why the ICH9 spec / hw
>> implementation doesn't lock APMC_EN; without that, APM_CNT won't
>> necessarily trigger an SMI.)
> 
> I don't think it should.  See here
> <https://lists.gnu.org/archive/html/qemu-devel/2015-04/msg02758.html>
> where I wrote explicitly "Even if the OS tries to maliciously set
> APMC_EN to 0 (SMI_LOCK doesn't lock APMC_EN)...".

It's not about feature detection -- the question (from
<553F4D23.3060305@redhat.com>) is whether I should set APMC_EN myself
*every time* before writing to APM_CNT, in the
EFI_SMM_CONTROL2_PROTOCOL.Trigger() method. That protocol is provided by
a runtime DXE driver and would be exercised by eg. the non-privileged
half of the runtime variable service driver.

It's no problem to set it, I have the code ready, I was just wondering
if I should keep that hunk. (In fact it might not even matter: if the OS
interferes and clears APMC_EN before the non-privileged half mentioned
above manages to raise the SMI, then the call / transition to SMM will
simply not happen, which is bad for the OS, and probably irrelevant for
the firmware (... the security thereof).)

I guess I'll keep re-setting APMC_EN in the Trigger() method; it won't hurt.

Thanks
Laszlo