Re: [Qemu-devel] [RFC PATCH v1 20/22] fw_cfg: sev: disable dma in real mode

[Paolo Bonzini]

On 14/09/2016 14:09, Eduardo Habkost wrote:
> On Wed, Sep 14, 2016 at 05:33:09AM +0300, Michael S. Tsirkin wrote:
>> On Wed, Sep 14, 2016 at 12:53:36AM +0200, Paolo Bonzini wrote:
>>>
>>>
>>> On 13/09/2016 16:50, Brijesh Singh wrote:
>>>> In SEV-enabled guest dma should be performed on shared pages. Since
>>>> the SeaBIOS executes in non PAE mode and does not have access to C-bit
>>>> to create a shared page hence disable the dma operation when reading
>>>> from fw_cfg interface.
>>>>
>>>> Signed-off-by: Brijesh Singh <address@hidden>
>>>> ---
>>>>  hw/nvram/fw_cfg.c |    6 ++++++
>>>>  1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>>>> index 6a68e59..aca99e9 100644
>>>> --- a/hw/nvram/fw_cfg.c
>>>> +++ b/hw/nvram/fw_cfg.c
>>>> @@ -24,6 +24,7 @@
>>>>  #include "qemu/osdep.h"
>>>>  #include "hw/hw.h"
>>>>  #include "sysemu/sysemu.h"
>>>> +#include "sysemu/kvm.h"
>>>>  #include "sysemu/dma.h"
>>>>  #include "hw/boards.h"
>>>>  #include "hw/isa/isa.h"
>>>> @@ -1009,6 +1010,11 @@ static void fw_cfg_io_realize(DeviceState *dev, 
>>>> Error **errp)
>>>>      FWCfgIoState *s = FW_CFG_IO(dev);
>>>>      SysBusDevice *sbd = SYS_BUS_DEVICE(dev);
>>>>  
>>>> +    /* disable dma on fw_cfg when SEV is enabled */
>>>> +    if (kvm_sev_enabled()) {
>>>> +        qdev_prop_set_bit(dev, "dma_enabled", false);
>>>> +    }
>>>> +
>>>>      /* when using port i/o, the 8-bit data register ALWAYS overlaps
>>>>       * with half of the 16-bit control register. Hence, the total size
>>>>       * of the i/o region used is FW_CFG_CTL_SIZE */
>>>>
>>>>
>>>>
>>>
>>> As Michael said, a workaround is possible using -global.  However, I
>>> really think that SEV should be implemented in UEFI firmware.  Because
>>> it runs in 64-bit mode, it would be able to run in paged mode and it
>>> would not have to encrypt everything before the SEV launch command.
>>>
>>> For example secure boot can be used to authenticate the kernel against
>>> keys provided by the owner in encrypted flash, or GRUB2 can be placed in
>>> the firmware by the owner and used to boot from a LUKS-encrypted /boot
>>> partition.
>>>
>>> Paolo
>>
>> Frankly I don't understand why do you need to mess with boot at all.
>> Quoting the cover letter:
>>
>>      SEV is designed to protect guest VMs from a benign but vulnerable
>>      (i.e. not fully malicious) hypervisor. In particular, it reduces the
>>      attack
>>      surface of guest VMs and can prevent certain types of VM-escape bugs
>>      (e.g. hypervisor read-anywhere) from being used to steal guest data.
>>
>> it seems highly unlikely that any secret data is used during boot.
>> So just let guest boot normally, and encrypt afterwards.
> 
> After boot seems too late for the attestation part (see Section
> 1.3.1: Launch in the spec), unless you can ensure the memory
> contents will always be exactly what the guest owner expects
> after every boot.

And the attestation is what lets the guest check that the memory
contents are indeed what the guest owner expects.

Paolo