Re: [Qemu-devel] [PATCH 5/6] [wip] tseg, part1, not (yet) tested

[Laszlo Ersek]

On 04/23/15 09:41, Laszlo Ersek wrote:
> On 04/23/15 09:02, Gerd Hoffmann wrote:
>>   Hi,
>>
>>>> The third one is messy. It relies on SMI_EN, which is an ioport at
>>>> PMBA+0x30. It requires a configured PMBA.
>>
>> Isn't that initialized early anyway, because the pm timer lives there?
> 
> TimerLib (which is based in OVMF's case on the PM timer) is not needed /
> used before PEI in the default case. It is used in SEC only when -D
> SOURCE_DEBUG_ENABLE is passed for the build (which is "never" in practice).
> 
>> It's not a regular pci bar exactly to allow early init, so the full pci
>> bus scan + bar allocation done later will not mess up things.
>>
>> [ just a question, could very well be that even when initialized early
>>   it isn't early enough because you need to know the memory layout
>>   before uncompressing the firmware modules ]
> 
> I'm trying to avoid that; the decompression happens to a low fixed range.
> 
>>> There's another problem with basing this decision in OVMF on
>>> SMI_EN.APMC_EN: it is not an idempotent check. At some point the
>>> firmware itself has to set SMI_EN.APMC_EN.
>>
>> Yep, you'll need some variable saying whenever smm is there or not and
>> check that.  Because of this.  Or (in case we are going for the fw_cfg
>> file) because you don't want dig into fw_cfg each time you need to know
>> this.
> 
> The annoying limitation with SEC is that it cannot use normal C static
> variables, nor PCDs. It is okay if SEC is a bit wasteful on fw_cfg (and
> even in SEC I might be able to cache the result in a local variable and
> pass it around). In PEI I can already use static variables (because OVMF
> is unorthodox and runs PEI modules from RAM, not flash), plus I can set
> PCDs (because in PEI the PCDs live in a dedicated HOB, and that HOB is
> allocated from RAM (independently of OVMF)).
> 
> I'll ask Jordan too about the dynamic feature detection, because there's
> another big hurdle with it (selecting a LockBox library instance at
> runtime, dependent on the presence of SMM). Deciding about SMM support
> at build time would make things hugely easier (because that's how
> firmware for physical platforms is built as well).

Okay, looks like Jordan agrees with using a static build option.

This makes things much easier, and I think I won't need a new fw_cfg file.

I will certainly do a sanity check (based on the condition we discussed
eariler in this thread, using the PCI config and iospace registers).

It's going to be "somewhere" in the firmware (still thinking about the
best place), but this way I won't have to handle everything (fall back
to non-SMM behavior) if the sanity check fails; in that case I can just
stop.

Public IRC transcript follows.

Thanks!
Laszlo

* Loaded log from Wed Apr 22 21:45:19 2015

* Now talking on #edk2
* Topic for #edk2 is: EDK II / OVMF / UEFI development discussions |
  http://www.tianocore.org/edk2
* Topic for #edk2 set by
  address@hidden at Thu
  Feb  5 03:36:47 2015

<lersek>   jljusten, are you still online?
<lersek>   (I guess you are, from your recent email)
<jljusten> lersek: yeah. it's getting a bit late though. :)
<lersek>   jljusten, okay, just briefly then
<lersek>   I can send an email too if you prefer that
<lersek>   the question is dynamic detection of SMM/SMRAM
           support
<lersek>   it is quite messy
<lersek>   and has a large number of ties into things
<lersek>   several modules / module types are affected
<lersek>   but the worst question is how we'd select a
           LockBox library instance dynamically
<lersek>   dependent on the presence of the SMM/SMRAM
           capability in QEMU
<lersek>   one idea I had
<lersek>   is quite ugly
<lersek>   but could work
<jljusten> ah, yikes
<lersek>   (1) introduce a new PPI and a new protocol
<lersek>   with some GUID we generate
<lersek>   this PPI and protocol would abstract the LockBox
           library interface
<lersek>   the PPI for PEIMs
<lersek>   the protocol for DXE modules
<lersek>   (2) we'd build two such drivers
<lersek>   for each of the phases
<lersek>   one pair would back the PPI / protocol with the
           SMM-based (unprivileged) lockbox library instance
<lersek>   another pair would back the PPI / protocol with
           our current (insecure) lockbox library instance
<lersek>   (3) we'd build both pairs of drivers into OVMF
<lersek>   and their entry points would check for SMM/SMRAM
<lersek>   only one pair would remain active and install the
           PPI / protocol
<lersek>   (4) we'd introduce another LockBox library
           instance
<lersek>   that would depend on the PPI / protocol
<lersek>   on the depex level
<lersek>   (PPI depexes can also be stated for PEIMs)
<jljusten> wow. any chance this could be a build time
           option? :)
<lersek>   then all modules using this library instance
           would be delayed until after the corect driver
<lersek>   exactly!
<lersek>   so you can see where this is going
<lersek>   a huge mess
<lersek>   I actually *want* to make this a build time option
<lersek>   but I was worried you'd ask me to do it
           dynamically
<lersek>   basing it on -D SMM_ENABLE, statically
<lersek>   would make things hugely easier
<lersek>   not just for the LockBox lib instance
<lersek>   but a number of other things as well
<lersek>   so if you agree to make SMM/SMRAM support a
           static build time config option
<lersek>   that would be awesome
<jljusten> Whenever I think of SMM, I think, yeah that'd be
           cool to have a sample platform in EDK II, but it never
           sounds like something we'd want to enable by default.
<jljusten> Of course, that could change over time...
<lersek>   right
<lersek>   so let's agree on the -D SMM_ENABLE for now
<lersek>   thank you very much
<lersek>   another question quickly while we're at it
<lersek>   how would you prefer checks for this in the C
           code?
<lersek>   feature PCD?
<lersek>   controlled by the build flag?
<lersek>   I think that's usually the best method for stuff
           like this
<lersek>   it's better than conditional inclusion (ie.
           #ifdef etc)
<lersek>   because all code gets verified by the compiler
<lersek>   I'll also add some sanity checks of course
<jljusten> Yeah, I think so.
<lersek>   so that a -D SMM_ENABLE build rejects running
           early, if qemu doesn't actually provide the feature
<lersek>   jljusten, thank you
<jljusten> Any chance to get video running before then?
           Probably not critical. If they are using the SMM build, they
           should know they need a newer QEMU...
<lersek>   jljusten, hm, I think video is initialized quite
           late; it depends on a UEFI driver and is connected in BDS --
           presence of SMM/SMRAM communicates that the user wants
           things to be secure, so it affects even SEC and PEI.
<lersek>   If those phases expect SMM/SMRAM and it's not
           there, the best would be just to stop right there (after
           logging an error message)
<lersek>   anyway we can perhaps refine this in one of the
           versions of the patchset-to-be
<lersek>   the important thing is that I can now start out
           with a static config flag, knowing that I won't have to
           rewrite *that*. :)
<lersek>   thanks!
<jljusten> I think SMM is not initialized until DXE, but
           yeah, I somehow doubt video would work in that case.
<lersek>   jljusten, indeed, SMM *drivers* only come into
           the picture in DXE
<lersek>   but until then the SMRAM area (TSEG on Q35/MCH)
           needs to be removed from the system memory HOB we install in
           PlatformPei
<lersek>   so that no memory allocations are served from
           there
<lersek>   the TSEG area is the last 1, 2, or 8 MB just
           below the end of the low RAM (ie. top of RAM under 4GB)
<lersek>   we need to set that aside while in PEI
<lersek>   even the permanent PEI ram cannot be installed
           the way it is now
<lersek>   (last 64 MB under top of RAM below 4GB)
<lersek>   so this affects a number of things
<lersek>   I'll also have to audit all of the special memory
           ranges that OVMF uses (see the "comprehensive memory map"
           section in the whitepaper)
<lersek>   because those ranges are sensitive (SMRAM is open
           while SEC and PEI run) and we must not allow the OS to poke
           data directly into some of them
<lersek>   so for example
<lersek>   we can't let SEC just reuse the pre-decompressed
           PEIFV on S3 resume
<lersek>   if SMM/SMRAM is selected
<lersek>   because a malicious OS may have overwritten the
           PEIFV
<lersek>   and then if it suspends, then the user resumes
<lersek>   the firmware will run the OS-injected code as
           part of PEI, with the SMRAM open
<lersek>   etc
<lersek>   it's going to be hugely intrusive
<jljusten> Right. I think most real platforms run code from
           flash in that case.
<lersek>   exactly
<lersek>   so that's one area where our smartness (ie.
           running PEI from RAM, and only SEC from flash) is hurting us
           wrt. SMM a little bit
<jljusten> You ought to be able to run from the SMRAM region
           though.
<lersek>   considering the above
<lersek>   the first idea I've come up with
<lersek>   is to just decompress the firmware volumes again
<lersek>   on S3 resume
<lersek>   overwriting whatever a potentially malicious
           runtime guest OS may have poked into there
<lersek>   but
<lersek>   of course
<lersek>   given the way we compress things, for the sake of
           good compression ratio
<lersek>   that means we'd decompress not just PEIFV but
           DXEFV too
<lersek>   and for that we'll need to set aside much more RAM
<lersek>   even from a non-malicious OS
<lersek>   because decompressing DXEFV too during S3 resume
           must not overwrite OS data
<lersek>   so this would be an argument for reverting to a
           more traditional layout
<lersek>   where PEI is uncompressed and runs from flash
<lersek>   and DXEFV is separately decompressed
<lersek>   in any case,
<lersek>   let me just run with this "decompress them both
           on S3" initial idea
<lersek>   we can discuss things better when there are
           patches for illustration
<lersek>   it doesn't have to be perfect in v1
<jljusten> We could trust code in SMRAM right?
<lersek>   see the original S3 series :)
<lersek>   that's one idea as well
<lersek>   I did think of it
<lersek>   we could place PEI in SMRAM
<lersek>   there are at least two problems with it though
<lersek>   (1) it would require making SEC too smart
<lersek>   configuring the SMRAM control registers and stuff
<lersek>   but that's the smaller issue
<lersek>   the big issue is the S3 PEIM itself
<lersek>   (2) that one has builtin knowledge about SMRAM
<lersek>   and it explicitly uses the SMM ACCESS2 PPI
<lersek>   to open and close smram
<lersek>   obviously
<lersek>   if it is running from SMRAM
<lersek>   it doesn't help if it closes down SMRAM
           underneath itself. :)
<lersek>   anyway what we know now should be enough for
           prototyping
<lersek>   without committing to directions that would be
           certainly wrong in the future
<lersek>   I'll send out a transcript of this discussion in
           the qemu-devel thread
<lersek>   I'll CC you too, and then from the message ID
           you'll be able to find the entire thread on gmane