Re: [PATCH 2/3] Ovmfpkg/VirtioBlkDxe: map virtio-blk request and response buffers

[Brijesh Singh <brijesh.singh@amd.com>]

On 8/27/17 5:05 PM, Laszlo Ersek wrote:

[...]

> (4) I think we should be careful with BusMasterCommonBuffer operations
> similarly to BusMasterWrite operations -- populate first, map second.
>
> This is to avoid exposing any stale data, even for a short window, to
> the device.
>
> Speaking in SEV terms, IoMmuMap() will decrypt NumberOfBytes in-place --
> which is by-design, but then it should decrypt what we just put there,
> not whatever used to be there (until we overwrite it).
>
> IOW, please move the mapping just under the *HostStatus assignment.
>
> ... I've now checked all the VirtioOperationBusMasterCommonBuffer
> mappings in the tree, and the rest is fine -- in fact there is only one
> (outside of this patch) at the moment: in VirtioRingMap(). And that is
> fine, because VirtioRingInit() zeroes out the entire ring, after
> allocating it.
>
> Probably a good idea to attend to this in the other drivers (wherever
> they use BusMasterCommonBuffer).

Will do.

[...]

>>
>>> +  if (EFI_ERROR (Ret)) {
>>> +    Status = EFI_DEVICE_ERROR;
>>> +  }
>>> +
>>> +  if (!EFI_ERROR (Status) &&
>>> +      *HostStatus == VIRTIO_BLK_S_OK) {
>>> +    Status = EFI_SUCCESS;
>>> +  } else {
>>> +    Status = EFI_DEVICE_ERROR;
>>>    }
>> should be written like this:
>>
>>   if (EFI_ERROR (Status) || EFI_ERROR (UnmapStatus) ||
>>       *HostStatus != VIRTIO_BLK_S_OK) {
>>     Status = EFI_DEVICE_ERROR;
>>   }
>>
>> Namely,
>>
>> - this block will ensure that we only look at *HostStatus when we're
>> allowed to (i.e., after both VirtioFlush() and Unmap succeeded),

Actually since the HostStatus is mapped with BusMasterCommonBuffer hence
we can safely move the unmapping of HostStatus buffer later in the code.
In summary, I can still retain the original if statement and do the
unmapping outside the check. In v2 I will try to move the code around.
thanks


>>
>> - If VirtioFlush() fails and sets Status to some error code, then we
>> coerce Status to EFI_DEVICE_ERROR,
>>
>> - If the entire condition evaluates to FALSE, then Status is already
>> EFI_SUCCESS, so no need to touch it.
>>
>>
>> Regarding the VirtioScsiDxe driver... in this patch, we get away with
>> the above shortcut (without making a mess of the code), but in the
>> VirtioScsiDxe driver, we won't. In that driver, the bus master device
>> can produce *two* output buffers, so you will have to unmap at least one
>> of those under an error-handling label -- when you mapped the first
>> successfully, and failed to map the second.
>>
>> (In this patch you managed to unmap StatusMapping in shared code, but
>> that only worked because you had only one output buffer, and you could
>> put its mapping last.)
>>
>> And once you unmap an output buffer on both the success path and the
>> failure path, things get more complex. I think that's OK, we should just
>> be consistent about it. So, for VirtioScsiDxe, I suggest the pattern I
>> laid out in
>> <http://mid.mail-archive.com/f7736294-0368-cddc-3fcd-de76cae5650e@redhat.com>.