* [PATCH v2 1/1] OvmfPkg/AmdSev/SecretDxe: Allocate CC secret location as EfiACPIReclaimMemory
@ 2022-12-12 8:08 Dov Murik
2022-12-12 15:00 ` Lendacky, Thomas
0 siblings, 1 reply; 4+ messages in thread
From: Dov Murik @ 2022-12-12 8:08 UTC (permalink / raw)
To: devel
Cc: Dov Murik, Tobin Feldman-Fitzthum, Ard Biesheuvel, Erdem Aktas,
Gerd Hoffmann, James Bottomley, Jiewen Yao, Jordan Justen,
Michael Roth, Min Xu, Tobin Feldman-Fitzthum, Tom Lendacky
BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4186
Commit 079a58276b98 ("OvmfPkg/AmdSev/SecretPei: Mark SEV launch secret
area as reserved") marked the launch secret area itself (1 page) as
reserved so the guest OS can use it during the lifetime of the OS.
However, the address and size of the secret area held in the
CONFIDENTIAL_COMPUTING_SECRET_LOCATION struct are declared as STATIC in
OVMF (in AmdSev/SecretDxe); therefore there's no guarantee that it will
not be written over by OS data.
Fix this by allocating the memory for the
CONFIDENTIAL_COMPUTING_SECRET_LOCATION struct with the
EfiACPIReclaimMemory memory type to ensure the guest OS will not reuse
this memory.
Fixes: 079a58276b98 ("OvmfPkg/AmdSev/SecretPei: Mark SEV launch secret area as reserved")
Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
Cc: Erdem Aktas <erdemaktas@google.com>
Cc: Gerd Hoffmann <kraxel@redhat.com>
Cc: James Bottomley <jejb@linux.ibm.com>
Cc: Jiewen Yao <Jiewen.Yao@intel.com>
Cc: Jordan Justen <jordan.l.justen@intel.com>
Cc: Michael Roth <michael.roth@amd.com>
Cc: Min Xu <min.m.xu@intel.com>
Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>
Cc: Tom Lendacky <thomas.lendacky@amd.com>
Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
v2 changes:
* Allocate with EfiACPIReclaimMemory memory type (thanks Ard)
---
OvmfPkg/AmdSev/SecretDxe/SecretDxe.c | 22 ++++++++++++++------
1 file changed, 16 insertions(+), 6 deletions(-)
diff --git a/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
index 3d84b2545052..4f65b1ce5ba5 100644
--- a/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
+++ b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
@@ -8,11 +8,6 @@
#include <Library/UefiBootServicesTableLib.h>
#include <Guid/ConfidentialComputingSecret.h>
-STATIC CONFIDENTIAL_COMPUTING_SECRET_LOCATION mSecretDxeTable = {
- FixedPcdGet32 (PcdSevLaunchSecretBase),
- FixedPcdGet32 (PcdSevLaunchSecretSize),
-};
-
EFI_STATUS
EFIAPI
InitializeSecretDxe (
@@ -20,8 +15,23 @@ InitializeSecretDxe (
IN EFI_SYSTEM_TABLE *SystemTable
)
{
+ EFI_STATUS Status;
+ CONFIDENTIAL_COMPUTING_SECRET_LOCATION *SecretDxeTable;
+
+ Status = gBS->AllocatePool (
+ EfiACPIReclaimMemory,
+ sizeof (CONFIDENTIAL_COMPUTING_SECRET_LOCATION),
+ (VOID **)&SecretDxeTable
+ );
+ if (EFI_ERROR (Status)) {
+ return Status;
+ }
+
+ SecretDxeTable->Base = FixedPcdGet32 (PcdSevLaunchSecretBase);
+ SecretDxeTable->Size = FixedPcdGet32 (PcdSevLaunchSecretSize);
+
return gBS->InstallConfigurationTable (
&gConfidentialComputingSecretGuid,
- &mSecretDxeTable
+ SecretDxeTable
);
}
--
2.25.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH v2 1/1] OvmfPkg/AmdSev/SecretDxe: Allocate CC secret location as EfiACPIReclaimMemory
2022-12-12 8:08 [PATCH v2 1/1] OvmfPkg/AmdSev/SecretDxe: Allocate CC secret location as EfiACPIReclaimMemory Dov Murik
@ 2022-12-12 15:00 ` Lendacky, Thomas
2022-12-15 2:42 ` Yao, Jiewen
0 siblings, 1 reply; 4+ messages in thread
From: Lendacky, Thomas @ 2022-12-12 15:00 UTC (permalink / raw)
To: Dov Murik, devel
Cc: Tobin Feldman-Fitzthum, Ard Biesheuvel, Erdem Aktas,
Gerd Hoffmann, James Bottomley, Jiewen Yao, Jordan Justen,
Michael Roth, Min Xu, Tobin Feldman-Fitzthum
On 12/12/22 02:08, Dov Murik wrote:
> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4186
>
> Commit 079a58276b98 ("OvmfPkg/AmdSev/SecretPei: Mark SEV launch secret
> area as reserved") marked the launch secret area itself (1 page) as
> reserved so the guest OS can use it during the lifetime of the OS.
> However, the address and size of the secret area held in the
> CONFIDENTIAL_COMPUTING_SECRET_LOCATION struct are declared as STATIC in
> OVMF (in AmdSev/SecretDxe); therefore there's no guarantee that it will
> not be written over by OS data.
>
> Fix this by allocating the memory for the
> CONFIDENTIAL_COMPUTING_SECRET_LOCATION struct with the
> EfiACPIReclaimMemory memory type to ensure the guest OS will not reuse
> this memory.
>
> Fixes: 079a58276b98 ("OvmfPkg/AmdSev/SecretPei: Mark SEV launch secret area as reserved")
> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> Cc: Erdem Aktas <erdemaktas@google.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: James Bottomley <jejb@linux.ibm.com>
> Cc: Jiewen Yao <Jiewen.Yao@intel.com>
> Cc: Jordan Justen <jordan.l.justen@intel.com>
> Cc: Michael Roth <michael.roth@amd.com>
> Cc: Min Xu <min.m.xu@intel.com>
> Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>
> Cc: Tom Lendacky <thomas.lendacky@amd.com>
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
>
> ---
>
> v2 changes:
> * Allocate with EfiACPIReclaimMemory memory type (thanks Ard)
> ---
> OvmfPkg/AmdSev/SecretDxe/SecretDxe.c | 22 ++++++++++++++------
> 1 file changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
> index 3d84b2545052..4f65b1ce5ba5 100644
> --- a/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
> +++ b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
> @@ -8,11 +8,6 @@
> #include <Library/UefiBootServicesTableLib.h>
> #include <Guid/ConfidentialComputingSecret.h>
>
> -STATIC CONFIDENTIAL_COMPUTING_SECRET_LOCATION mSecretDxeTable = {
> - FixedPcdGet32 (PcdSevLaunchSecretBase),
> - FixedPcdGet32 (PcdSevLaunchSecretSize),
> -};
> -
> EFI_STATUS
> EFIAPI
> InitializeSecretDxe (
> @@ -20,8 +15,23 @@ InitializeSecretDxe (
> IN EFI_SYSTEM_TABLE *SystemTable
> )
> {
> + EFI_STATUS Status;
> + CONFIDENTIAL_COMPUTING_SECRET_LOCATION *SecretDxeTable;
> +
> + Status = gBS->AllocatePool (
> + EfiACPIReclaimMemory,
> + sizeof (CONFIDENTIAL_COMPUTING_SECRET_LOCATION),
> + (VOID **)&SecretDxeTable
> + );
> + if (EFI_ERROR (Status)) {
> + return Status;
> + }
> +
> + SecretDxeTable->Base = FixedPcdGet32 (PcdSevLaunchSecretBase);
> + SecretDxeTable->Size = FixedPcdGet32 (PcdSevLaunchSecretSize);
> +
> return gBS->InstallConfigurationTable (
> &gConfidentialComputingSecretGuid,
> - &mSecretDxeTable
> + SecretDxeTable
> );
> }
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2 1/1] OvmfPkg/AmdSev/SecretDxe: Allocate CC secret location as EfiACPIReclaimMemory
2022-12-12 15:00 ` Lendacky, Thomas
@ 2022-12-15 2:42 ` Yao, Jiewen
2022-12-15 6:33 ` Dov Murik
0 siblings, 1 reply; 4+ messages in thread
From: Yao, Jiewen @ 2022-12-15 2:42 UTC (permalink / raw)
To: Tom Lendacky, Dov Murik, devel@edk2.groups.io
Cc: Tobin Feldman-Fitzthum, Ard Biesheuvel, Aktas, Erdem,
Gerd Hoffmann, James Bottomley, Justen, Jordan L, Michael Roth,
Xu, Min M, Tobin Feldman-Fitzthum
Hey
CI failed - https://github.com/tianocore/edk2/pull/3772
Have you run CI before submit patch? Please take a look.
Thank you
Yao, Jiewen
> -----Original Message-----
> From: Tom Lendacky <thomas.lendacky@amd.com>
> Sent: Monday, December 12, 2022 11:01 PM
> To: Dov Murik <dovmurik@linux.ibm.com>; devel@edk2.groups.io
> Cc: Tobin Feldman-Fitzthum <tobin@ibm.com>; Ard Biesheuvel
> <ardb+tianocore@kernel.org>; Aktas, Erdem <erdemaktas@google.com>;
> Gerd Hoffmann <kraxel@redhat.com>; James Bottomley
> <jejb@linux.ibm.com>; Yao, Jiewen <jiewen.yao@intel.com>; Justen, Jordan
> L <jordan.l.justen@intel.com>; Michael Roth <michael.roth@amd.com>; Xu,
> Min M <min.m.xu@intel.com>; Tobin Feldman-Fitzthum
> <tobin@linux.ibm.com>
> Subject: Re: [PATCH v2 1/1] OvmfPkg/AmdSev/SecretDxe: Allocate CC secret
> location as EfiACPIReclaimMemory
>
> On 12/12/22 02:08, Dov Murik wrote:
> > BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4186
> >
> > Commit 079a58276b98 ("OvmfPkg/AmdSev/SecretPei: Mark SEV launch
> secret
> > area as reserved") marked the launch secret area itself (1 page) as
> > reserved so the guest OS can use it during the lifetime of the OS.
> > However, the address and size of the secret area held in the
> > CONFIDENTIAL_COMPUTING_SECRET_LOCATION struct are declared as
> STATIC in
> > OVMF (in AmdSev/SecretDxe); therefore there's no guarantee that it will
> > not be written over by OS data.
> >
> > Fix this by allocating the memory for the
> > CONFIDENTIAL_COMPUTING_SECRET_LOCATION struct with the
> > EfiACPIReclaimMemory memory type to ensure the guest OS will not reuse
> > this memory.
> >
> > Fixes: 079a58276b98 ("OvmfPkg/AmdSev/SecretPei: Mark SEV launch
> secret area as reserved")
> > Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> > Cc: Erdem Aktas <erdemaktas@google.com>
> > Cc: Gerd Hoffmann <kraxel@redhat.com>
> > Cc: James Bottomley <jejb@linux.ibm.com>
> > Cc: Jiewen Yao <Jiewen.Yao@intel.com>
> > Cc: Jordan Justen <jordan.l.justen@intel.com>
> > Cc: Michael Roth <michael.roth@amd.com>
> > Cc: Min Xu <min.m.xu@intel.com>
> > Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>
> > Cc: Tom Lendacky <thomas.lendacky@amd.com>
> > Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
>
> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
>
> >
> > ---
> >
> > v2 changes:
> > * Allocate with EfiACPIReclaimMemory memory type (thanks Ard)
> > ---
> > OvmfPkg/AmdSev/SecretDxe/SecretDxe.c | 22 ++++++++++++++------
> > 1 file changed, 16 insertions(+), 6 deletions(-)
> >
> > diff --git a/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
> b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
> > index 3d84b2545052..4f65b1ce5ba5 100644
> > --- a/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
> > +++ b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
> > @@ -8,11 +8,6 @@
> > #include <Library/UefiBootServicesTableLib.h>
> > #include <Guid/ConfidentialComputingSecret.h>
> >
> > -STATIC CONFIDENTIAL_COMPUTING_SECRET_LOCATION
> mSecretDxeTable = {
> > - FixedPcdGet32 (PcdSevLaunchSecretBase),
> > - FixedPcdGet32 (PcdSevLaunchSecretSize),
> > -};
> > -
> > EFI_STATUS
> > EFIAPI
> > InitializeSecretDxe (
> > @@ -20,8 +15,23 @@ InitializeSecretDxe (
> > IN EFI_SYSTEM_TABLE *SystemTable
> > )
> > {
> > + EFI_STATUS Status;
> > + CONFIDENTIAL_COMPUTING_SECRET_LOCATION *SecretDxeTable;
> > +
> > + Status = gBS->AllocatePool (
> > + EfiACPIReclaimMemory,
> > + sizeof (CONFIDENTIAL_COMPUTING_SECRET_LOCATION),
> > + (VOID **)&SecretDxeTable
> > + );
> > + if (EFI_ERROR (Status)) {
> > + return Status;
> > + }
> > +
> > + SecretDxeTable->Base = FixedPcdGet32 (PcdSevLaunchSecretBase);
> > + SecretDxeTable->Size = FixedPcdGet32 (PcdSevLaunchSecretSize);
> > +
> > return gBS->InstallConfigurationTable (
> > &gConfidentialComputingSecretGuid,
> > - &mSecretDxeTable
> > + SecretDxeTable
> > );
> > }
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH v2 1/1] OvmfPkg/AmdSev/SecretDxe: Allocate CC secret location as EfiACPIReclaimMemory
2022-12-15 2:42 ` Yao, Jiewen
@ 2022-12-15 6:33 ` Dov Murik
0 siblings, 0 replies; 4+ messages in thread
From: Dov Murik @ 2022-12-15 6:33 UTC (permalink / raw)
To: Yao, Jiewen, Tom Lendacky, devel@edk2.groups.io
Cc: Tobin Feldman-Fitzthum, Ard Biesheuvel, Aktas, Erdem,
Gerd Hoffmann, James Bottomley, Justen, Jordan L, Michael Roth,
Xu, Min M, Tobin Feldman-Fitzthum, Dov Murik
Thank you Jiewen.
On 15/12/2022 4:42, Yao, Jiewen wrote:
> Hey
> CI failed - https://github.com/tianocore/edk2/pull/3772
> Have you run CI before submit patch? Please take a look.
I haven't -- last time I contributed to edk2 was a long time ago; I'll
look for the instructions on triggering the CI myself.
I see patch format errors (lines a bit too long) and uncrustify errors
(missing space in variable declaration). I'll fix these and send
another spin.
-Dov
>
> Thank you
> Yao, Jiewen
>
>> -----Original Message-----
>> From: Tom Lendacky <thomas.lendacky@amd.com>
>> Sent: Monday, December 12, 2022 11:01 PM
>> To: Dov Murik <dovmurik@linux.ibm.com>; devel@edk2.groups.io
>> Cc: Tobin Feldman-Fitzthum <tobin@ibm.com>; Ard Biesheuvel
>> <ardb+tianocore@kernel.org>; Aktas, Erdem <erdemaktas@google.com>;
>> Gerd Hoffmann <kraxel@redhat.com>; James Bottomley
>> <jejb@linux.ibm.com>; Yao, Jiewen <jiewen.yao@intel.com>; Justen, Jordan
>> L <jordan.l.justen@intel.com>; Michael Roth <michael.roth@amd.com>; Xu,
>> Min M <min.m.xu@intel.com>; Tobin Feldman-Fitzthum
>> <tobin@linux.ibm.com>
>> Subject: Re: [PATCH v2 1/1] OvmfPkg/AmdSev/SecretDxe: Allocate CC secret
>> location as EfiACPIReclaimMemory
>>
>> On 12/12/22 02:08, Dov Murik wrote:
>>> BZ: https://bugzilla.tianocore.org/show_bug.cgi?id=4186
>>>
>>> Commit 079a58276b98 ("OvmfPkg/AmdSev/SecretPei: Mark SEV launch
>> secret
>>> area as reserved") marked the launch secret area itself (1 page) as
>>> reserved so the guest OS can use it during the lifetime of the OS.
>>> However, the address and size of the secret area held in the
>>> CONFIDENTIAL_COMPUTING_SECRET_LOCATION struct are declared as
>> STATIC in
>>> OVMF (in AmdSev/SecretDxe); therefore there's no guarantee that it will
>>> not be written over by OS data.
>>>
>>> Fix this by allocating the memory for the
>>> CONFIDENTIAL_COMPUTING_SECRET_LOCATION struct with the
>>> EfiACPIReclaimMemory memory type to ensure the guest OS will not reuse
>>> this memory.
>>>
>>> Fixes: 079a58276b98 ("OvmfPkg/AmdSev/SecretPei: Mark SEV launch
>> secret area as reserved")
>>> Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
>>> Cc: Erdem Aktas <erdemaktas@google.com>
>>> Cc: Gerd Hoffmann <kraxel@redhat.com>
>>> Cc: James Bottomley <jejb@linux.ibm.com>
>>> Cc: Jiewen Yao <Jiewen.Yao@intel.com>
>>> Cc: Jordan Justen <jordan.l.justen@intel.com>
>>> Cc: Michael Roth <michael.roth@amd.com>
>>> Cc: Min Xu <min.m.xu@intel.com>
>>> Cc: Tobin Feldman-Fitzthum <tobin@linux.ibm.com>
>>> Cc: Tom Lendacky <thomas.lendacky@amd.com>
>>> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
>>
>> Reviewed-by: Tom Lendacky <thomas.lendacky@amd.com>
>>
>>>
>>> ---
>>>
>>> v2 changes:
>>> * Allocate with EfiACPIReclaimMemory memory type (thanks Ard)
>>> ---
>>> OvmfPkg/AmdSev/SecretDxe/SecretDxe.c | 22 ++++++++++++++------
>>> 1 file changed, 16 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
>> b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
>>> index 3d84b2545052..4f65b1ce5ba5 100644
>>> --- a/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
>>> +++ b/OvmfPkg/AmdSev/SecretDxe/SecretDxe.c
>>> @@ -8,11 +8,6 @@
>>> #include <Library/UefiBootServicesTableLib.h>
>>> #include <Guid/ConfidentialComputingSecret.h>
>>>
>>> -STATIC CONFIDENTIAL_COMPUTING_SECRET_LOCATION
>> mSecretDxeTable = {
>>> - FixedPcdGet32 (PcdSevLaunchSecretBase),
>>> - FixedPcdGet32 (PcdSevLaunchSecretSize),
>>> -};
>>> -
>>> EFI_STATUS
>>> EFIAPI
>>> InitializeSecretDxe (
>>> @@ -20,8 +15,23 @@ InitializeSecretDxe (
>>> IN EFI_SYSTEM_TABLE *SystemTable
>>> )
>>> {
>>> + EFI_STATUS Status;
>>> + CONFIDENTIAL_COMPUTING_SECRET_LOCATION *SecretDxeTable;
>>> +
>>> + Status = gBS->AllocatePool (
>>> + EfiACPIReclaimMemory,
>>> + sizeof (CONFIDENTIAL_COMPUTING_SECRET_LOCATION),
>>> + (VOID **)&SecretDxeTable
>>> + );
>>> + if (EFI_ERROR (Status)) {
>>> + return Status;
>>> + }
>>> +
>>> + SecretDxeTable->Base = FixedPcdGet32 (PcdSevLaunchSecretBase);
>>> + SecretDxeTable->Size = FixedPcdGet32 (PcdSevLaunchSecretSize);
>>> +
>>> return gBS->InstallConfigurationTable (
>>> &gConfidentialComputingSecretGuid,
>>> - &mSecretDxeTable
>>> + SecretDxeTable
>>> );
>>> }
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-12-15 6:33 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2022-12-12 8:08 [PATCH v2 1/1] OvmfPkg/AmdSev/SecretDxe: Allocate CC secret location as EfiACPIReclaimMemory Dov Murik
2022-12-12 15:00 ` Lendacky, Thomas
2022-12-15 2:42 ` Yao, Jiewen
2022-12-15 6:33 ` Dov Murik
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox