Re: multiple levels of support for MOR / MORLock

[Laszlo Ersek <lersek@redhat.com>]

On 10/01/17 13:36, Yao, Jiewen wrote:
> I believe it is legal that SMM code calls UEFI services before SMM_READY_TO_LOCK event.
> 
> For example, SMM_CORE calls gBS->LocateProtocol(), and mSecurity->FileAuthenticationState(), until SmmReadyToLock event triggered.
> SMM_CPU driver calls gDS->GetMemorySpaceMap() at SmmReadyToLock event.
> 
> Because EndOfDxe happens before SmmReadyToLock, it is OK to call UEFI service at that point.
> 
> After SMM_READY_TO_LOCK, it is illegal to call UEFI services in SMM.

Great, thank you for confirming!

In OVMF, we do signal gEfiEndOfDxeEventGroupGuid before we install
gEfiDxeSmmReadyToLockProtocolGuid.

I'll go ahead and implement this as well then.

Cheers!
Laszlo

> From: Laszlo Ersek [mailto:lersek@redhat.com]
> Sent: Sunday, October 1, 2017 4:23 PM
> To: Yao, Jiewen <jiewen.yao@intel.com>
> Cc: edk2-devel-01 <edk2-devel@lists.01.org>; Ladi Prosek <lprosek@redhat.com>; Ard Biesheuvel <ard.biesheuvel@linaro.org>
> Subject: Re: [edk2] multiple levels of support for MOR / MORLock
> 
> On 10/01/17 05:56, Yao, Jiewen wrote:
>> Good test! Thanks for the investigation.
>>
>> That is a little annoying.
> 
> Agreed!
> 
> In a virtualization-only scenario, I wouldn't care much (it's very
> unusual to install two OSes in the same VM and "dual boot" them), but on
> physical machines, dual-booting Windows and Linux is very frequent, and
> whatever we put in the Variable driver now will end up on physical
> machines sooner or later.
> 
>> I still do not want to use PCD. My thought is below:
>>
>> Current MOR has TCG protocol dependence.   So can we add one more
>> check - to check the existence of TCG/TCG2 protocol in variable
>> driver? Both TCG and TCG2 are in mdepkg, there is no dependency
>> issue.
>>
>> Even if MOR is present, but neither TCG nor TCG2 is present, don't
>> create MORL but delete MOR. (Just in case people boot fedora, then
>> boot windows) I believe that can handle the fedora issue.
> According to the PI spec, "1.8 MM Traditional Driver Initialization", in
> the VariableSmm driver I can only locate UEFI protocols in the entry
> point function:
> 
>     [...] An MM Driver’s initialization phase ends when the entry point
>     returns. [...] UEFI protocols can be located and used by MM drivers
>     only during MM Initialization. [...]
> 
> The SmmEndOfDxe callback does not run as part of the VariableSmm
> driver's entry point function (VariableServiceInitialize()), so I
> believe I might not be allowed to call gBS->LocateProtocol() in the
> callback. Is that correct?
> 
> 
> Another option would be to register a UEFI protocol notify for the
> TCG/TCG2 protocols, and to set a global variable (in SMRAM) when this
> notify fires. Then the SmmEndOfDxe callback could check the variable.
> 
> However, what happens if the TCG/TCG2 protocols are installed *after*
> PiSmmCpuDxeSmm is dispatched, and the SMM IPL closes SMRAM? Then the
> protocol notify could never set the variable in SMRAM -- in fact, the
> protocol notify couldn't even be invoked, because the notify function
> itself is located in SMRAM (as part of the VariableSmm executable).
> 
> 
> So, for this to work, I believe we'd have to modify the SMM_CORE to
> "forward" the TCG/TCG2 UEFI protocols into the SMM protocol database
> (similarly to EndOfDxe/SmmEndOfDxe, although in that case, EndOfDxe is a
> UEFI event group, not a UEFI protocol).
> 
> Is my understanding correct? What do you suggest?
> 
> 
> Thank you for taking the time to discuss this with me! If it's becoming
> too long, we can continue after the weekend & the holidays end.
> 
> Cheers!
> Laszlo
> 
>>> 在 2017年10月1日，上午4:33，Laszlo Ersek <lersek@redhat.com<mailto:lersek@redhat.com>> 写道：
>>>
>>> Hello Jiewen,
>>>
>>>> On 09/30/17 05:53, Yao, Jiewen wrote:
>>>> Agree. Adding a hook function is OK for me, as long as the hook
>>>> function implementation is in TcgMorLockXXX.c
>>>
>>> I've implemented this, but I've run into a problem while testing it.
>>>
>>> Your idea requires that *nothing* create the MOR variable other than
>>>
>>>  SecurityPkg/Tcg/MemoryOverwriteControl/TcgMor.inf
>>>
>>> Unfortunately, released Fedora operating systems break this assumption.
>>> (It is a Linux kernel bug, no question about that, but it still breaks
>>> the idea.)
>>>
>>> Namely, in the current upstream Linux kernel (the *upcoming* v4.14
>>> release), there is a function called
>>> efi_enable_reset_attack_mitigation(), added in commit ccc829ba3624b:
>>>
>>>  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=ccc829ba3624b
>>>
>>>> /*
>>>> * Enable reboot attack mitigation. This requests that the firmware clear the
>>>> * RAM on next reboot before proceeding with boot, ensuring that any secrets
>>>> * are cleared. If userland has ensured that all secrets have been removed
>>>> * from RAM before reboot it can simply reset this variable.
>>>> */
>>>> void efi_enable_reset_attack_mitigation(efi_system_table_t *sys_table_arg)
>>>> {
>>>>        u8 val = 1;
>>>>        efi_guid_t var_guid = MEMORY_ONLY_RESET_CONTROL_GUID;
>>>>        efi_status_t status;
>>>>        unsigned long datasize = 0;
>>>>
>>>>        status = get_efi_var(efi_MemoryOverWriteRequest_name, &var_guid,
>>>>                             NULL, &datasize, NULL);
>>>>
>>>>        if (status == EFI_NOT_FOUND)
>>>>                return;
>>>>
>>>>        set_efi_var(efi_MemoryOverWriteRequest_name, &var_guid,
>>>>                    EFI_VARIABLE_NON_VOLATILE |
>>>>                    EFI_VARIABLE_BOOTSERVICE_ACCESS |
>>>>                    EFI_VARIABLE_RUNTIME_ACCESS, sizeof(val), &val);
>>>> }
>>>
>>> This function is correct in that it first checks whether the MOR
>>> variable exists. If it doesn't exist, the function doesn't *create* the
>>> variable.
>>>
>>> However, several Fedora distribution kernels *already* contain an
>>> *earlier* version of the same function (which had never been merged into
>>> upstream Linux), and that variant of the function is incorrect:
>>>
>>>  https://git.kernel.org/pub/scm/linux/kernel/git/jwboyer/fedora.git/commit/?id=65673e37e61d8
>>>
>>>> static void enable_reset_attack_mitigation(void)
>>>> {
>>>>        static const efi_guid_t var_guid = MEMORY_ONLY_RESET_CONTROL_GUID;
>>>>        static const efi_char16_t MemoryOverwriteRequestControl_name[] = {
>>>>                'M', 'e', 'm', 'o', 'r', 'y',
>>>>                'O', 'v', 'e', 'r', 'w', 'r', 'i', 't', 'e',
>>>>                'R', 'e', 'q', 'u', 'e', 's', 't',
>>>>                'C', 'o', 'n', 't', 'r', 'o', 'l',
>>>>                0
>>>>        };
>>>>        u8 val = 1;
>>>>
>>>>        /* Ignore the return value here - there's not really a lot we can do */
>>>>        efi_call_runtime(set_variable,
>>>>                        (efi_char16_t *)MemoryOverwriteRequestControl_name,
>>>>                        (efi_guid_t *)&var_guid,
>>>>                        EFI_VARIABLE_NON_VOLATILE |
>>>>                        EFI_VARIABLE_BOOTSERVICE_ACCESS |
>>>>                        EFI_VARIABLE_RUNTIME_ACCESS,
>>>>                        sizeof(val), val);
>>>> }
>>>
>>> This variant does not check if the MOR variable exists.
>>>
>>> If the variable doesn't exist, then this function will create it
>>> (incorrectly). At the next boot, the SmmEndOfDxe callback in
>>> "TcgMorLockSmm.c" will think that the firmware platform includes support
>>> for the MOR variable (it assumes that TcgMor.inf is part of the firmware
>>> and that TcgMor.inf created the variable), and then the callback
>>> creates/sets MorLock as well.
>>>
>>> Again, this is clearly a kernel bug, but it is part of at least the
>>> following releases:
>>>
>>> - Fedora 26:
>>>  https://git.kernel.org/pub/scm/linux/kernel/git/jwboyer/fedora.git/commit/?id=65673e37e61d
>>>
>>> - Fedora 25:
>>>  https://git.kernel.org/pub/scm/linux/kernel/git/jwboyer/fedora.git/commit/?id=1f4e5e657685
>>>
>>> - Fedora 24:
>>>  https://git.kernel.org/pub/scm/linux/kernel/git/jwboyer/fedora.git/commit/?id=677765d4db8e
>>>
>>> Thus the SmmEndOfDxe callback wouldn't work correctly together with
>>> these Fedora releases.
>>>
>>> Thanks,
>>> Laszlo
>>>
>>>> From: Laszlo Ersek [mailto:lersek@redhat.com]
>>>> Sent: Saturday, September 30, 2017 12:39 AM
>>>> To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com>>
>>>> Cc: edk2-devel-01 <edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org>>; Ladi Prosek <lprosek@redhat.com<mailto:lprosek@redhat.com>>
>>>> Subject: Re: [edk2] multiple levels of support for MOR / MORLock
>>>>
>>>>> On 09/29/17 15:26, Yao, Jiewen wrote:
>>>>> Sure. Feel free to submit patch.
>>>>>
>>>>> For the fix, I suggest we keep all MORL related code into
>>>>> TcgMorLockXXX.c. I do not suggest we mix the MORL stuff into
>>>>> VariableXXX.c.
>>>>
>>>> I've had more thoughts about this, since sending my last email.
>>>>
>>>> Currently SmmEndOfDxeCallback()
>>>> [MdeModulePkg/Universal/Variable/RuntimeDxe/VariableSmm.c] performs
>>>> the following actions:
>>>>
>>>> - sets "mEndOfDxe" to TRUE
>>>> - calls VarCheckLibInitializeAtEndOfDxe()
>>>> - calls InitializeVariableQuota()
>>>> - calls ReclaimForOS() optionally
>>>>
>>>> If we register a separate callback for gEfiSmmEndOfDxeProtocolGuid, in
>>>> order to create the MORL variable, then that callback could be invoked
>>>> after SmmEndOfDxeCallback(). I think it would not be optimal to create
>>>> MORL after SmmEndOfDxeCallback() has run, even if it worked,
>>>> technically speaking. This suggests to hook the MORL creation to the
>>>> top of SmmEndOfDxeCallback(). What do you suggest?
>>>>
>>>>> A reminder that, the PRC team will have the PRC national holiday in
>>>>> next week. As such, we won't have too much activity in next week.
>>>>>
>>>>> If you submit the patch, I will try to review in next week to unblock
>>>>> the Linux work.
>>>>
>>>> Ah, sorry, I haven't been aware of the holidays.
>>>>
>>>> I might send a patch soon, but please don't let me disturb you during
>>>> the holidays! Especially because this patch might need more discussion
>>>> and more versins. Holidays are for getting rested from work! :) We can
>>>> sort it out after you return.
>>>>
>>>>> In the patch, if you can add the detailed test result, that will be
>>>>> very helpful for us. For example, test SMM/non-SMM version, test with
>>>>> MOR present/absent, etc.
>>>>
>>>> For the "no MOR, no MORL" case, I'll have to rely on Ladi, for test
>>>> results (Ladi offered to help test that case, thanks again for that,
>>>> Ladi).
>>>>
>>>> However, for regression-testing the "both MOR and MORL" case, I can
>>>> only rely on Intel developers and quality engineers -- OVMF doesn't
>>>> have any TPM / MOR solution at the moment. (If we had one, then we
>>>> wouldn't have found or reported this issue in the first place. :) )
>>>>
>>>> The fact that I can't regression-test the MOR+MORL case myself
>>>> supports the idea that we should postpone the review and commit (if
>>>> appropriate) of the patch until after the PRC national holiday.
>>>>
>>>> Thanks!
>>>> Laszlo
>>>>
>>>>
>>>>> From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
>>>>> Sent: Friday, September 29, 2017 7:06 PM
>>>>> To: Yao, Jiewen <jiewen.yao@intel.com<mailto:jiewen.yao@intel.com<mailto:jiewen.yao@intel.com%3cmailto:jiewen.yao@intel.com>>>
>>>>> Cc: edk2-devel-01 <edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org<mailto:edk2-devel@lists.01.org%3cmailto:edk2-devel@lists.01.org>>>
>>>>> Subject: Re: [edk2] multiple levels of support for MOR / MORLock
>>>>>
>>>>>> On 09/29/17 03:52, Yao, Jiewen wrote:
>>>>>> Thanks Laszlo.
>>>>>>
>>>>>> Yes, I agree it is bug. Would you please help to file a bugzilar in
>>>>>> EDKII?
>>>>>>
>>>>>> For the fix, I think we have a way to resolve it without PCD. (I do
>>>>>> not want to bother a platform developer to set a new PCD.)
>>>>>>
>>>>>> The only invalid case we need handle is: MOR is absent, but MORL is
>>>>>> present.
>>>>>>
>>>>>> My thought is to let Variable driver check if MOR is present.
>>>>>> Variable driver can defer the MORL setting at EndOfDxe event based
>>>>>> upon the presence of MOR. If MOR driver is present, it sets MOR at
>>>>>> entrypoint. EndOfDxe is good enough to know the state.
>>>>>>
>>>>>> Also, because EndOfDxe is PI event, the UEFI OS is not aware of
>>>>>> that.
>>>>>
>>>>> Sounds great; thanks a lot!
>>>>>
>>>>> I've filed:
>>>>>
>>>>>  https://bugzilla.tianocore.org/show_bug.cgi?id=727
>>>>>
>>>>> If possible I'd like the fix to be committed sometime next week. Is
>>>>> it OK if I try and submit a patch soon? My plan is the following:
>>>>>
>>>>> - In MorLockInit() [TcgMorLockSmm.c], call
>>>>> gSmst->SmmRegisterProtocolNotify() in order to register a callback
>>>>> for gEfiSmmEndOfDxeProtocolGuid
>>>>>
>>>>> - In the callback function, call VariableServiceGetVariable(), with
>>>>> size 0, to see if MOR is present -- if EFI_BUFFER_TOO_SMALL is
>>>>> returned, MOR is present; otherwise MOR is absent.
>>>>>
>>>>> - If MOR is present, then call SetMorLockVariable(0) from the
>>>>> callback function; like MorLockInit() does now. Otherwise, do
>>>>> nothing.
>>>>>
>>>>> - It looks like there are no circumstances under which I should
>>>>> de-register the callback. (I.e. call SmmRegisterProtocolNotify() with
>>>>> a NULL Function argument.)
>>>>>
>>>>>
>>>>> Now, I can see that VariableSmm.c already installs such a callback --
>>>>> SmmEndOfDxeCallback(). Should I hook into that callback function
>>>>> through a new BOOLEAN variable, such as "mDelayedMorLockInit" (and
>>>>> then MorLockInit() would only set this variable to TRUE), or should I
>>>>> install a separate callback?
>>>>>
>>>>> Either way, I don't think that I should do the MOR/MORL stuff in the
>>>>> current SmmEndOfDxeCallback() function *unconditionally*, because
>>>>> that callback is set up when the *read* half of the variable services
>>>>> is initialized, but MORL only becomes relevant when the *write* half
>>>>> of the variable services is initialized (which occurs in the
>>>>> SmmFtwNotificationEvent() callback, i.e. when the FaultTolerantWrite
>>>>> SMM protocol becomes available). Hence I think we need either a
>>>>> separate callback registration, or a new boolean for the existent
>>>>> callback.
>>>>>
>>>>> Thanks!
>>>>> Laszlo