Re: [PATCH 10/12] OvmfPkg/IoMmuDxe: implement in-place decryption/encryption for Map/Unmap

[Laszlo Ersek <lersek@redhat.com>]

(CC Andrew)

On 08/03/17 01:01, Brijesh Singh wrote:
>
>
> On 8/2/17 4:24 PM, Laszlo Ersek wrote:
>
> [Snip]
>> At the moment, we have the foll+    // The buffer at MapInfo->CryptedAddress comes from AllocateBuffer().
>>      //
>>      MapInfo->PlainTextAddress = MapInfo->CryptedAddress;
>> -
>>      //
>> -    // Therefore no mapping is necessary.
>> +    // Stash the crypted data.
>>      //
>> -    *DeviceAddress = MapInfo->PlainTextAddress;
>> -    *Mapping       = NO_MAPPING;
>> -    FreePool (MapInfo);
>> -    return EFI_SUCCESS;
>> +    CommonBufferHeader = (COMMON_BUFFER_HEADER *)(
>> +                           (UINTN)MapInfo->CryptedAddress - EFI_PAGE_SIZE
>> +                           );
>
> One question, per spec, is it legal for client to call Map() at some
> offset within allocated buffer ?
>
> e.g something like this:
>
> * AllocateBuffer (, 1, &Buffer);
> * MapBuffer = Buffer + 10;
> * Map (, BusMasterCommonBuffer, MappedBuffer, 10, ..) // Bascially Map
> 10 bytes from offset 10

The input/output parameter names seem to counter-indicate such use.
Namely, AllocateBuffer() outputs a "HostAddress" param, and Map() takes
a "HostAddress" param. Plus we have sentences like this:

Under PciIo.Map():

> ... only memory allocated via the AllocateBuffer() interface can be
> mapped for this type of operation ...

Under PciIo.AllocateBuffer():

> The AllocateBuffer() function allocates pages that are suitable for an
> EfiPciOperationBusMasterCommonBuffer or
> EfiPciOperationBusMasterCommonBuffer64 mapping. This means that the
> buffer allocated by this function must support simultaneous access by
> both the processor and a PCI Bus Master. The device address that the
> PCI Bus Master uses to access *the* buffer can be retrieved with a
> call to Map().

This second passage says *the* buffer. (Emphasis mine above.)

> If this is legal then we may need to build MapInfo during
> AllocateBuffer() to locate the "StashBuffer".

Right, in that case we'd have to build a list of allocated ranges (an
interval tree of sorts) in AllocateBuffer, and convert any
CommonBuffer[64] Map() call to its containing allocation with a search.

It would be worse than that, actually... The pattern you have raised
could be taken one step further: do one AllocateBuffer(), and several
CommonBuffer[64] Map()s into it :) What should happen if those maps are
distinct? What should happen if they overlap? :) I can't even imagine
what this would mean for SEV.

... There are guide-like sections in the generic description of
EFI_PCI_IO_PROTOCOL; Andrew quoted them earlier:

  http://mid.mail-archive.com/A29CDE8F-C82A-4C92-ABF8-008A9BF8F230@apple.com

> DMA Bus Master Common Buffer Operation
> ======================================
> * Call AllocateBuffer() to allocate a common buffer.
> * Call Map() for EfiPciIoOperationBusMasterCommonBuffer.
> * Program the DMA Bus Master with the DeviceAddress returned by Map().
> * The common buffer can now be accessed equally by the processor and
>   the DMA bus master.
> * Call Unmap().
> * Call FreeBuffer().

Look at page 854 (printed page number: 784) in UEFI 2.7.

Thus, I don't think the usage you raise is permitted.

Thanks!
Laszlo

> So far, I have not came across this usecase but I wanted to check with
> you so that we don't fail on corner cases. Good part if you have
> ASSERT() so we should be able to catch them (if any).
>
>> +    ASSERT (CommonBufferHeader->Signature == COMMON_BUFFER_SIG);
>> +    CopyMem (
>> +      CommonBufferHeader->StashBuffer,
>> +      (VOID *)(UINTN)MapInfo->CryptedAddress,
>> +      MapInfo->NumberOfBytes
>> +      );
>> +    //
>> +    // Point "DecryptionSource" to the stash buffer so that we decrypt
>> +    // it to the original location, after the switch statement.
>> +    //
>> +    DecryptionSource = CommonBufferHeader->StashBuffer;
>> +    break;
>>
> [Snip]
>