Re: [PATCH v2 1/2] UefiCpuPkg/CpuDxe: allow accessing (DXE) page table in SMM mode

[Laszlo Ersek <lersek@redhat.com>]

On 06/13/18 17:14, Andrew Fish wrote:
> 
> 
>> On Jun 12, 2018, at 10:35 PM, Jian J Wang <jian.j.wang@intel.com> wrote:
>>
>>> v2:
>>>  a. add more specific explanations in commit message
>>>  b. add more comments in code
>>>  c. remove redundant logic in IsInSmm()
>>>  d. fix a logic hole in GetCurrentPagingContext()
>>>  e. replace meanless constant macro with meaning ones
>>
>> The MdePkg/Library/SmmMemoryAllocationLib, used only by DXE_SMM_DRIVER,
>> allows to free memory allocated in DXE (before EndOfDxe). This is done
>> by checking the memory range and calling gBS services to do real
>> operation if the memory to free is out of SMRAM. If some memory related
>> features, like Heap Guard, are enabled, gBS interface will turn to
>> EFI_CPU_ARCH_PROTOCOL.SetMemoryAttributes(), provided by
>> DXE driver UefiCpuPkg/CpuDxe, to change memory paging attributes. This
>> means we have part of DXE code running in SMM mode in certain
>> circumstances.
>>
>> Because page table in SMM mode is different from DXE mode and CpuDxe
>> always uses current registers (CR0, CR3, etc.) to get memory paging
>> attributes, it cannot get the correct attributes of DXE memory in SMM
>> mode from SMM page table. This will cause incorrect memory manipulations,
>> like fail the releasing of Guard pages if Heap Guard is enabled.
>>
>> The solution in this patch is to store the DXE page table information
>> (e.g. value of CR0, CR3 registers, etc.) in a global variable of CpuDxe
>> driver. If CpuDxe detects it's in SMM mode, it will use this global
>> variable to access page table instead of current processor registers.
>> This can avoid retrieving wrong DXE memory paging attributes and changing
>> SMM page table attributes unexpectedly.
>>
> 
> Are there any security implications having SMM depend on attacker controlled data (DXE page tables)?

This thought crossed my mind as well, and a part of the questions that I
raised earlier are related.

The important thing to consider here is the call chain; in other words,
the circumstances under which we can reach CpuDxe code *at all* while
the processor is in SMM.

The call chain starts when an SMM driver (a traditional SMM driver I
reckon) calls FreePool() or FreePages(), and the SmmMemoryAllocationLib
instance determines that the SMM driver is freeing *DXE* pool (or pages)
memory, and not SMRAM. In this case, SmmMemoryAllocationLib forwards the
call out to gBS->FreePool() or gBS->FreePages(). And, it is *from there*
that we gradually reach CpuDxe (the provider of the CpuArch protocol) to
mess with DXE page table entries -- because the gBS->FreePool() and
gBS->FreePages() services do that in edk2, regardless of SMM, when the
heap guard is enabled.

Obviously, gBS points into attacker controlled memory, so as soon as
SmmMemoryAllocationLib calls gBS->anything, the calling SMM driver has
been compromised immediately, way before we reach CpuDxe or anything
else in DXE. So why isn't SmmMemoryAllocationLib already broken?

Because, I think, a traditional SMM driver *must not* call FreePool()
and FreePages(), for potentially releasing DXE memory, outside its entry
point function anyway. That is, such FreePool / FreePages calls are
permitted only under the exact same circumstances where the driver is
allowed to utilize any other DXE protocols and services anyway; despite
potentially executing in SMM.

Once all DXE and SMM drivers have been dispatched and we reach BDS,
SMRAM is locked down (by platform BDS), and SMM drivers aren't allowed
to touch *anything* DXE any longer.

In other words, it is safe to rely on CpuDxe global variables in SMM
because we only do that when calling gBS->anything is still safe as well
-- that is, before any 3rd party code gets a chance to run.

In my previous review at

http://mid.mail-archive.com/aada511c-bdb9-d833-caa5-bee56cc47d27@redhat.com

this topic was the subject of my question (9a):

> So basically what I'd like to see here is evidence that
>
> (9a) the IsInSmm() helper function is never called outside of the
>      entry point of an MM driver, due to the restrictions on
>      gBS->LocateProtocol() and mSmmBase2->InSmm()

I didn't get a clear answer to that, but I managed to convince myself,
with the above argument. If any of the new code added to CpuDxe is
reached in SMM *after* 3rd party code was launched, then at least one
SMM driver messed up (calling FreePool / FreePages on DXE memory at the
wrong time), and then the vulnerability exists in that driver without
the CpuDxe changes already.

Thus, I believe this series does not introduce a vulnerability or new
risks, it just adds more logic to the end of a call chain that is *already*
- either broken (if an SMM driver calls FreePool / FreePages on DXE
  memory at the wrong time),
- or safe (if all SMM drivers call FreePool / FreePages on DXE memory
  only in their entry point functions).


More generally, to be honest: I find it terribly risky that
SmmMemoryAllocationLib contains convenience code like this. In my
opinion, SmmMemoryAllocationLib should never ever silently call out to
gBS->anything; if it determines that the memory to release is outside of
SMRAM, it should call CpuDeadLoop(). SMM drivers that want to release
DXE memory should always call gBS services directly, not through
MemoryAllocationLib APIs. Such a clear distinction would have also made
the verification of this patch a lot easier -- the argument would have
started with, "CpuDxe code is reached in SMM when SMM drivers call
gBS->FreeXxx()". And the restrictions on gBS->FreeXxx() (namely, that
they are restricted to the entry point function) make for a simpler
argument.

Thanks,
Laszlo