1. Patchew
  2. EDK2
  3. [edk2] [PATCH v1 0/4] OvmfPkg : IoMmuDxe: BusMasterCommonBuffer support when SEV is active
  4. View patch

[edk2] [PATCH v1 3/4] OvmfPkg: IommuDxe: Zero the shared page(s) on Unmap()

Brijesh Singh posted 4 patches 7 years, 4 months ago
[edk2] [PATCH v1 3/4] OvmfPkg: IommuDxe: Zero the shared page(s) on Unmap()
Posted by Brijesh Singh 7 years, 4 months ago 
To support the Map(), we allocate bounce buffer with C-bit cleared,
the buffer is referred as a DeviceAddress. Typically, DeviceAddress
is used as communication block between guest and hypervisor. When
guest is done with communication block, it calls Unmap().The Unmap()
free's the DeviceAddress, if we do not clear the content of shared
communication block during Unmap() then data remains readble to the
hypervisor for an unpredicatable time. Let's zero the bounce buffer
after we are done using it.

I did some benchmark and did not see any measure perform impact of
zeroing the page(s).

Suggested-by: Laszlo Ersek <lersek@redhat.com>
Contributed-under: TianoCore Contribution Agreement 1.0
Cc: Laszlo Ersek <lersek@redhat.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
---
 OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
index 5ae54482fffe..04e3725ff7e6 100644
--- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
+++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
@@ -67,8 +67,7 @@ SetBufferAsEncDec (
   // buffer matches with same encryption mask.
   //
   if (!Enc) {
-    Status = MemEncryptSevClearPageEncMask (0, MapInfo->DeviceAddress,
-               MapInfo->NumberOfPages, TRUE);
+    Status = MemEncryptSevClearPageEncMask (0, TempBuffer, MapInfo->NumberOfPages, TRUE);
     ASSERT_EFI_ERROR (Status);
   }
 
@@ -79,7 +78,7 @@ SetBufferAsEncDec (
   //
   CopyMem (
     (VOID *) (UINTN) TempBuffer,
-    (VOID *) (UINTN)MapInfo->HostAddress,
+    (VOID *) (UINTN) MapInfo->HostAddress,
     MapInfo->NumberOfBytes);
 
   //
@@ -109,11 +108,8 @@ SetBufferAsEncDec (
   //
   // Restore the encryption mask of the intermediate buffer
   //
-  if (!Enc) {
-    Status = MemEncryptSevSetPageEncMask (0, MapInfo->DeviceAddress,
-               MapInfo->NumberOfPages, TRUE);
-    ASSERT_EFI_ERROR (Status);
-  }
+  Status = MemEncryptSevSetPageEncMask (0, TempBuffer, MapInfo->NumberOfPages, TRUE);
+  ASSERT_EFI_ERROR (Status);
 
   //
   // Free the intermediate buffer
@@ -386,6 +382,12 @@ IoMmuUnmap (
   ASSERT_EFI_ERROR(Status);
 
   //
+  // Zero the shared memory so that hypervisor no longer able to get intelligentable
+  // data.
+  //
+  SetMem ((VOID *) (UINTN)MapInfo->DeviceAddress, MapInfo->NumberOfBytes, 0);
+
+  //
   // Free the bounce buffer
   //
   gBS->FreePages (MapInfo->DeviceAddress, MapInfo->NumberOfPages);
-- 
2.7.4

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v1 3/4] OvmfPkg: IommuDxe: Zero the shared page(s) on Unmap()
Posted by Brijesh Singh 7 years, 4 months ago 
I just realized that this patch contains some changes which should be
part of Patch 2/3. Please ignore this patch for now. I will fix it as part
of v2 when addressing review feedback. thanks

-Brijesh

On 07/31/2017 02:31 PM, Brijesh Singh wrote:
> To support the Map(), we allocate bounce buffer with C-bit cleared,
> the buffer is referred as a DeviceAddress. Typically, DeviceAddress
> is used as communication block between guest and hypervisor. When
> guest is done with communication block, it calls Unmap().The Unmap()
> free's the DeviceAddress, if we do not clear the content of shared
> communication block during Unmap() then data remains readble to the
> hypervisor for an unpredicatable time. Let's zero the bounce buffer
> after we are done using it.
> 
> I did some benchmark and did not see any measure perform impact of
> zeroing the page(s).
> 
> Suggested-by: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>   OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 18 ++++++++++--------
>   1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
> index 5ae54482fffe..04e3725ff7e6 100644
> --- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
> +++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
> @@ -67,8 +67,7 @@ SetBufferAsEncDec (
>     // buffer matches with same encryption mask.
>     //
>     if (!Enc) {
> -    Status = MemEncryptSevClearPageEncMask (0, MapInfo->DeviceAddress,
> -               MapInfo->NumberOfPages, TRUE);
> +    Status = MemEncryptSevClearPageEncMask (0, TempBuffer, MapInfo->NumberOfPages, TRUE);
>       ASSERT_EFI_ERROR (Status);
>     }
>   
> @@ -79,7 +78,7 @@ SetBufferAsEncDec (
>     //
>     CopyMem (
>       (VOID *) (UINTN) TempBuffer,
> -    (VOID *) (UINTN)MapInfo->HostAddress,
> +    (VOID *) (UINTN) MapInfo->HostAddress,
>       MapInfo->NumberOfBytes);
>   
>     //
> @@ -109,11 +108,8 @@ SetBufferAsEncDec (
>     //
>     // Restore the encryption mask of the intermediate buffer
>     //
> -  if (!Enc) {
> -    Status = MemEncryptSevSetPageEncMask (0, MapInfo->DeviceAddress,
> -               MapInfo->NumberOfPages, TRUE);
> -    ASSERT_EFI_ERROR (Status);
> -  }
> +  Status = MemEncryptSevSetPageEncMask (0, TempBuffer, MapInfo->NumberOfPages, TRUE);
> +  ASSERT_EFI_ERROR (Status);
>   
>     //
>     // Free the intermediate buffer
> @@ -386,6 +382,12 @@ IoMmuUnmap (
>     ASSERT_EFI_ERROR(Status);
>   
>     //
> +  // Zero the shared memory so that hypervisor no longer able to get intelligentable
> +  // data.
> +  //
> +  SetMem ((VOID *) (UINTN)MapInfo->DeviceAddress, MapInfo->NumberOfBytes, 0);
> +
> +  //
>     // Free the bounce buffer
>     //
>     gBS->FreePages (MapInfo->DeviceAddress, MapInfo->NumberOfPages);
> 
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v1 3/4] OvmfPkg: IommuDxe: Zero the shared page(s) on Unmap()
Posted by Laszlo Ersek 7 years, 4 months ago 
On 07/31/17 21:31, Brijesh Singh wrote:
> To support the Map(), we allocate bounce buffer with C-bit cleared,
> the buffer is referred as a DeviceAddress. Typically, DeviceAddress
> is used as communication block between guest and hypervisor. When
> guest is done with communication block, it calls Unmap().The Unmap()
> free's the DeviceAddress, if we do not clear the content of shared
> communication block during Unmap() then data remains readble to the
> hypervisor for an unpredicatable time. Let's zero the bounce buffer
> after we are done using it.
> 
> I did some benchmark and did not see any measure perform impact of
> zeroing the page(s).
> 
> Suggested-by: Laszlo Ersek <lersek@redhat.com>
> Contributed-under: TianoCore Contribution Agreement 1.0
> Cc: Laszlo Ersek <lersek@redhat.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> ---
>  OvmfPkg/IoMmuDxe/AmdSevIoMmu.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
> index 5ae54482fffe..04e3725ff7e6 100644
> --- a/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
> +++ b/OvmfPkg/IoMmuDxe/AmdSevIoMmu.c
> @@ -67,8 +67,7 @@ SetBufferAsEncDec (
>    // buffer matches with same encryption mask.
>    //
>    if (!Enc) {
> -    Status = MemEncryptSevClearPageEncMask (0, MapInfo->DeviceAddress,
> -               MapInfo->NumberOfPages, TRUE);
> +    Status = MemEncryptSevClearPageEncMask (0, TempBuffer, MapInfo->NumberOfPages, TRUE);
>      ASSERT_EFI_ERROR (Status);
>    }
>  
> @@ -79,7 +78,7 @@ SetBufferAsEncDec (
>    //
>    CopyMem (
>      (VOID *) (UINTN) TempBuffer,
> -    (VOID *) (UINTN)MapInfo->HostAddress,
> +    (VOID *) (UINTN) MapInfo->HostAddress,
>      MapInfo->NumberOfBytes);
>  
>    //
> @@ -109,11 +108,8 @@ SetBufferAsEncDec (
>    //
>    // Restore the encryption mask of the intermediate buffer
>    //
> -  if (!Enc) {
> -    Status = MemEncryptSevSetPageEncMask (0, MapInfo->DeviceAddress,
> -               MapInfo->NumberOfPages, TRUE);
> -    ASSERT_EFI_ERROR (Status);
> -  }
> +  Status = MemEncryptSevSetPageEncMask (0, TempBuffer, MapInfo->NumberOfPages, TRUE);
> +  ASSERT_EFI_ERROR (Status);
>  
>    //
>    // Free the intermediate buffer
> @@ -386,6 +382,12 @@ IoMmuUnmap (
>    ASSERT_EFI_ERROR(Status);
>  
>    //
> +  // Zero the shared memory so that hypervisor no longer able to get intelligentable
> +  // data.
> +  //
> +  SetMem ((VOID *) (UINTN)MapInfo->DeviceAddress, MapInfo->NumberOfBytes, 0);

Please use ZeroMem().

Furthermore, ZeroMem() should occur just before every FreePages() call:
- when Unmap() releases the implicitly allocated bounce buffer
- when FreeBuffer() releases the explicitly allocated common buffer
  (I thought I spelled this out in my previous email(s), but in
  retrospect it seems I only intended to :/ )
- in the virtio drivers' exit-boot-services callbacks, FreeBuffer()
  can't be called (only Unmap(), after the virtio reset), so the
  ZeroMem() should be done manually there.

Thanks
Laszlo

> +
> +  //
>    // Free the bounce buffer
>    //
>    gBS->FreePages (MapInfo->DeviceAddress, MapInfo->NumberOfPages);
> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v1 3/4] OvmfPkg: IommuDxe: Zero the shared page(s) on Unmap()
Posted by Brijesh Singh 7 years, 4 months ago 

On 8/2/17 2:37 AM, Laszlo Ersek wrote:
> //
>> +  // Zero the shared memory so that hypervisor no longer able to get intelligentable
>> +  // data.
>> +  //
>> +  SetMem ((VOID *) (UINTN)MapInfo->DeviceAddress, MapInfo->NumberOfBytes, 0);
> Please use ZeroMem().
>
> Furthermore, ZeroMem() should occur just before every FreePages() call:
> - when Unmap() releases the implicitly allocated bounce buffer
> - when FreeBuffer() releases the explicitly allocated common buffer
>   (I thought I spelled this out in my previous email(s), but in
>   retrospect it seems I only intended to :/ )
> - in the virtio drivers' exit-boot-services callbacks, FreeBuffer()
>   can't be called (only Unmap(), after the virtio reset), so the
>   ZeroMem() should be done manually there.

Not sure why do we need to ZeroMem() when FreeBuffer()  is called for
explicitly allocated common buffer ? I thought before calling the
FreeBuffer() on common buffer, client will call Unmap() which will
restore the C-bit state on the common buffer and also update the
contents (i.e now common buffer will contain encrypted data).
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel
Re: [edk2] [PATCH v1 3/4] OvmfPkg: IommuDxe: Zero the shared page(s) on Unmap()
Posted by Laszlo Ersek 7 years, 4 months ago 
On 08/02/17 13:22, Brijesh Singh wrote:
> 
> 
> On 8/2/17 2:37 AM, Laszlo Ersek wrote:
>> //
>>> +  // Zero the shared memory so that hypervisor no longer able to get intelligentable
>>> +  // data.
>>> +  //
>>> +  SetMem ((VOID *) (UINTN)MapInfo->DeviceAddress, MapInfo->NumberOfBytes, 0);
>> Please use ZeroMem().
>>
>> Furthermore, ZeroMem() should occur just before every FreePages() call:
>> - when Unmap() releases the implicitly allocated bounce buffer
>> - when FreeBuffer() releases the explicitly allocated common buffer
>>   (I thought I spelled this out in my previous email(s), but in
>>   retrospect it seems I only intended to :/ )
>> - in the virtio drivers' exit-boot-services callbacks, FreeBuffer()
>>   can't be called (only Unmap(), after the virtio reset), so the
>>   ZeroMem() should be done manually there.
> 
> Not sure why do we need to ZeroMem() when FreeBuffer()  is called for
> explicitly allocated common buffer ? I thought before calling the
> FreeBuffer() on common buffer, client will call Unmap() which will
> restore the C-bit state on the common buffer and also update the
> contents (i.e now common buffer will contain encrypted data).
> 

My bad, you are totally right -- when I wrote the above, I actually
reviewed the "BusMasterCommonBuffer" section of my earlier message

http://mid.mail-archive.com/e1137140-f5d5-7eb8-0162-952694b24f96@redhat.com

and I totally missed that in that message I had written

"Client calls Unmap(). Unmap() restores the C bit in one fell swoop, and
encrypts the buffer in-place (by bouncing it page-wise to the static
array and back)."

Sigh. Need more rest.

Thanks for catching my error!
Laszlo
_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel

Patchew 2.1-devel-8c64711

© 2016 - 2024 Red Hat, Inc.