Re: [Qemu-devel] Configuring pflash devices for OVMF firmware

[Markus Armbruster <armbru@redhat.com>]

Laszlo Ersek <lersek@redhat.com> writes:

> On 01/30/19 15:33, Paolo Bonzini wrote:
>> On 30/01/19 15:13, Markus Armbruster wrote:
>>>     -global driver=cfi.pflash01,property=secure,value=on
>>>
>>> Affects *all* such devices, but fortunately we have at most two, and
>>> the one we don't want to affect happens to ignore the property value.
>>
>> Is this true?  I think both need secure=on, at least on x86.
>
> commit f71e42a5c98722d6faa5be84a34fbad90d27dc04
> Author: Paolo Bonzini <pbonzini@redhat.com>
> Date:   Wed Apr 8 14:09:43 2015 +0200
>
>     pflash_cfi01: add secure property
>
>     When this property is set, MMIO accesses are only allowed with the
>     MEMTXATTRS_SECURE attribute.  This is used for secure access to UEFI
>     variables stored in flash.
>
>     Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>
> If you don't add "secure=on" to unit#0, then MMIO accesses will be
> possible outside of SMM. From those, I'd hazard "MMIO reads" are
> generally irrelevant. "MMIO writes" could succeed to the RAM image, but:
>
> - they are never written back to the disk (due to readonly=on),
>
> - the actual contents of unit#0 stops mattering as soon as the SEC phase
>   decompresses the PEIFV and DXEFV firmware volumes from it, to DRAM.
>   The SMM infrastructure is then constructed in SMRAM from DRAM. By the
>   time a 3rd party UEFI application or driver, or an OS, is reached, the
>   sensitive bits are all locked in SMRAM.
>
> ... But, I wonder if S3 resume would be under threat in this case. In
> that case, SEC runs again (from pflash), and it re-decompresses
> PEIFV/DXEFV from pflash to DRAM. If the in-memory image of pflash
> doesn't revert to the (pristine, due to readonly=on) disk image at
> platform reset, then I reckon there could be a problem; the SEC code and
> the compressed FVs could have been tampered with in memory.
>
> I guess it's best to apply secure=on to unit#0 as well, after all :)

I thought secure=on affected only writes (and so wouldn't matter with
readonly=on), but I was wrong:

    static MemTxResult pflash_mem_read_with_attrs(void *opaque, hwaddr addr, uint64_t *value,
                                                  unsigned len, MemTxAttrs attrs)
    {
        pflash_t *pfl = opaque;
        bool be = !!(pfl->features & (1 << PFLASH_BE));

        if ((pfl->features & (1 << PFLASH_SECURE)) && !attrs.secure) {
            *value = pflash_data_read(opaque, addr, len, be);
        } else {
            *value = pflash_read(opaque, addr, len, be);
        }
        return MEMTX_OK;
    }

pflash_data_read() is what pflash_read() does when pfl->cmd is 0.

Hmm, why is it okay to treat all pfl->cmd values the same when
secure=on?