| Message ID | 20210820151933.22401-36-brijesh.singh@amd.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add AMD Secure Nested Paging (SEV-SNP) Guest Support | expand |
Hi Brijesh, On 20/08/2021 18:19, Brijesh Singh wrote: > Version 2 of GHCB specification provides NAEs that can be used by the SNP > guest to communicate with the PSP without risk from a malicious hypervisor > who wishes to read, alter, drop or replay the messages sent. > > In order to communicate with the PSP, the guest need to locate the secrets > page inserted by the hypervisor during the SEV-SNP guest launch. The > secrets page contains the communication keys used to send and receive the > encrypted messages between the guest and the PSP. The secrets page location > is passed through the setup_data. > > Create a platform device that the SNP guest driver can bind to get the > platform resources such as encryption key and message id to use to > communicate with the PSP. The SNP guest driver can provide userspace > interface to get the attestation report, key derivation, extended > attestation report etc. > > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > --- > arch/x86/kernel/sev.c | 68 +++++++++++++++++++++++++++++++++++++++ > include/linux/sev-guest.h | 5 +++ > 2 files changed, 73 insertions(+) > > diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c > index f42cd5a8e7bb..ab17c93634e9 100644 > --- a/arch/x86/kernel/sev.c > +++ b/arch/x86/kernel/sev.c > @@ -22,6 +22,8 @@ > #include <linux/log2.h> > #include <linux/efi.h> > #include <linux/sev-guest.h> > +#include <linux/platform_device.h> > +#include <linux/io.h> > > #include <asm/cpu_entry_area.h> > #include <asm/stacktrace.h> > @@ -37,6 +39,7 @@ > #include <asm/apic.h> > #include <asm/efi.h> > #include <asm/cpuid.h> > +#include <asm/setup.h> > > #include "sev-internal.h" > > @@ -2164,3 +2167,68 @@ int snp_issue_guest_request(int type, struct snp_guest_request_data *input, unsi > return ret; > } > EXPORT_SYMBOL_GPL(snp_issue_guest_request); > + > +static struct platform_device guest_req_device = { > + .name = "snp-guest", > + .id = -1, > +}; > + > +static u64 find_secrets_paddr(void) > +{ > + u64 pa_data = boot_params.cc_blob_address; > + struct cc_blob_sev_info info; > + void *map; > + > + /* > + * The CC blob contains the address of the secrets page, check if the > + * blob is present. > + */ > + if (!pa_data) > + return 0; > + > + map = early_memremap(pa_data, sizeof(info)); > + memcpy(&info, map, sizeof(info)); > + early_memunmap(map, sizeof(info)); > + > + /* Verify that secrets page address is passed */ > + if (info.secrets_phys && info.secrets_len == PAGE_SIZE) > + return info.secrets_phys; > + > + return 0; > +} > + > +static int __init add_snp_guest_request(void) > +{ > + struct snp_secrets_page_layout *layout; > + struct snp_guest_platform_data data; > + > + if (!sev_feature_enabled(SEV_SNP)) > + return -ENODEV; > + > + snp_secrets_phys = find_secrets_paddr(); > + if (!snp_secrets_phys) > + return -ENODEV; > + > + layout = snp_map_secrets_page(); > + if (!layout) > + return -ENODEV; > + > + /* > + * The secrets page contains three VMPCK that can be used for > + * communicating with the PSP. We choose the VMPCK0 to encrypt guest > + * messages send and receive by the Linux. Provide the key and > + * id through the platform data to the driver. > + */ > + data.vmpck_id = 0; > + memcpy_fromio(data.vmpck, layout->vmpck0, sizeof(data.vmpck)); > + > + iounmap(layout); > + > + platform_device_add_data(&guest_req_device, &data, sizeof(data)); > + > + if (!platform_device_register(&guest_req_device)) > + dev_info(&guest_req_device.dev, "secret phys 0x%llx\n", snp_secrets_phys); Should you return the error code from platform_device_register() in case it fails (returns something other than zero)? -Dov > + > + return 0; > +} > +device_initcall(add_snp_guest_request); > diff --git a/include/linux/sev-guest.h b/include/linux/sev-guest.h > index 16b6af24fda7..e1cb3f7dd034 100644 > --- a/include/linux/sev-guest.h > +++ b/include/linux/sev-guest.h > @@ -68,6 +68,11 @@ struct snp_guest_request_data { > unsigned int data_npages; > }; > > +struct snp_guest_platform_data { > + u8 vmpck_id; > + char vmpck[VMPCK_KEY_LEN]; > +}; > + > #ifdef CONFIG_AMD_MEM_ENCRYPT > int snp_issue_guest_request(int vmgexit_type, struct snp_guest_request_data *input, > unsigned long *fw_err); >
Hi Dov, On 8/31/21 6:37 AM, Dov Murik wrote: >> + >> + if (!platform_device_register(&guest_req_device)) >> + dev_info(&guest_req_device.dev, "secret phys 0x%llx\n", snp_secrets_phys); > > Should you return the error code from platform_device_register() in case > it fails (returns something other than zero)? > Yes, I will fix it in next rev. Will return a non-zero on failure to register the device. thanks
On Fri, Aug 20, 2021 at 10:19:30AM -0500, Brijesh Singh wrote: > Version 2 of GHCB specification provides NAEs that can be used by the SNP Resolve the "NAE" abbreviation here so that it is clear what this means. > guest to communicate with the PSP without risk from a malicious hypervisor > who wishes to read, alter, drop or replay the messages sent. This here says "malicious hypervisor" from which we protect from... > In order to communicate with the PSP, the guest need to locate the secrets > page inserted by the hypervisor during the SEV-SNP guest launch. The ... but this here says the secrets page is inserted by the same hypervisor from which we're actually protecting. You wanna rephrase that to explain what exactly happens so that it doesn't sound like we're really trusting the HV with the secrets page. > secrets page contains the communication keys used to send and receive the > encrypted messages between the guest and the PSP. The secrets page location > is passed through the setup_data. > > Create a platform device that the SNP guest driver can bind to get the > platform resources such as encryption key and message id to use to > communicate with the PSP. The SNP guest driver can provide userspace > interface to get the attestation report, key derivation, extended > attestation report etc. > > Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> > --- > arch/x86/kernel/sev.c | 68 +++++++++++++++++++++++++++++++++++++++ > include/linux/sev-guest.h | 5 +++ > 2 files changed, 73 insertions(+) ... > +static u64 find_secrets_paddr(void) > +{ > + u64 pa_data = boot_params.cc_blob_address; > + struct cc_blob_sev_info info; > + void *map; > + > + /* > + * The CC blob contains the address of the secrets page, check if the > + * blob is present. > + */ > + if (!pa_data) > + return 0; > + > + map = early_memremap(pa_data, sizeof(info)); > + memcpy(&info, map, sizeof(info)); > + early_memunmap(map, sizeof(info)); > + > + /* Verify that secrets page address is passed */ That's hardly verifying something - if anything, it should say /* smoke-test the secrets page passed */ > + if (info.secrets_phys && info.secrets_len == PAGE_SIZE) > + return info.secrets_phys; ... which begs the question: how do we verify the HV is not passing some garbage instead of an actual secrets page? I guess it is that: "SNP_LAUNCH_UPDATE can insert two special pages into the guest’s memory: the secrets page and the CPUID page. The secrets page contains encryption keys used by the guest to interact with the firmware. Because the secrets page is encrypted with the guest’s memory encryption key, the hypervisor cannot read the keys. The CPUID page contains hypervisor provided CPUID function values that it passes to the guest. The firmware validates these values to ensure the hypervisor is not providing out-of-range values." From "4.5 Launching a Guest" in the SNP FW ABI spec. I think that explanation above is very important wrt to explaining the big picture how this all works with those pages injected into the guest so I guess somewhere around here a comment should say "See section 4.5 Launching a Guest in the SNP FW ABI spec for details about those special pages." or so. > + > + return 0; > +} > + > +static int __init add_snp_guest_request(void) If anything, that should be called init_snp_platform_device() or so. > +{ > + struct snp_secrets_page_layout *layout; > + struct snp_guest_platform_data data; > + > + if (!sev_feature_enabled(SEV_SNP)) > + return -ENODEV; > + > + snp_secrets_phys = find_secrets_paddr(); > + if (!snp_secrets_phys) > + return -ENODEV; > + > + layout = snp_map_secrets_page(); > + if (!layout) > + return -ENODEV; > + > + /* > + * The secrets page contains three VMPCK that can be used for What's VMPCK? > + * communicating with the PSP. We choose the VMPCK0 to encrypt guest "We" is? > + * messages send and receive by the Linux. Provide the key and "... by the Linux."?! That sentence needs more love. > + * id through the platform data to the driver. > + */ > + data.vmpck_id = 0; > + memcpy_fromio(data.vmpck, layout->vmpck0, sizeof(data.vmpck)); > + > + iounmap(layout); > + > + platform_device_add_data(&guest_req_device, &data, sizeof(data)); Oh look, that function can return an error. > + > + if (!platform_device_register(&guest_req_device)) > + dev_info(&guest_req_device.dev, "secret phys 0x%llx\n", snp_secrets_phys); Make that message human-readable - not a debug one. Thx.
On 9/2/21 11:40 AM, Borislav Petkov wrote: > On Fri, Aug 20, 2021 at 10:19:30AM -0500, Brijesh Singh wrote: >> Version 2 of GHCB specification provides NAEs that can be used by the SNP > > Resolve the "NAE" abbreviation here so that it is clear what this means. > Noted. >> guest to communicate with the PSP without risk from a malicious hypervisor >> who wishes to read, alter, drop or replay the messages sent. > > This here says "malicious hypervisor" from which we protect from... > >> In order to communicate with the PSP, the guest need to locate the secrets >> page inserted by the hypervisor during the SEV-SNP guest launch. The > > ... but this here says the secrets page is inserted by the same > hypervisor from which we're actually protecting. > The content of the secret page is populated by the PSP. Hypervisor cannot alter the contents; all it can do tell the guest where the secrets page is present in the memory. The guest will read the secrets page to get the VM communication key and use that key to encrypt the message send between the PSP and guest. > You wanna rephrase that to explain what exactly happens so that it > doesn't sound like we're really trusting the HV with the secrets page. > Sure, I will expand it a bit more. >> secrets page contains the communication keys used to send and receive the >> encrypted messages between the guest and the PSP. The secrets page location >> is passed through the setup_data. >> >> Create a platform device that the SNP guest driver can bind to get the >> platform resources such as encryption key and message id to use to >> communicate with the PSP. The SNP guest driver can provide userspace >> interface to get the attestation report, key derivation, extended >> attestation report etc. >> >> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> >> --- >> arch/x86/kernel/sev.c | 68 +++++++++++++++++++++++++++++++++++++++ >> include/linux/sev-guest.h | 5 +++ >> 2 files changed, 73 insertions(+) > > ... > >> +static u64 find_secrets_paddr(void) >> +{ >> + u64 pa_data = boot_params.cc_blob_address; >> + struct cc_blob_sev_info info; >> + void *map; >> + >> + /* >> + * The CC blob contains the address of the secrets page, check if the >> + * blob is present. >> + */ >> + if (!pa_data) >> + return 0; >> + >> + map = early_memremap(pa_data, sizeof(info)); >> + memcpy(&info, map, sizeof(info)); >> + early_memunmap(map, sizeof(info)); >> + >> + /* Verify that secrets page address is passed */ > > That's hardly verifying something - if anything, it should say > > /* smoke-test the secrets page passed */ > Noted. >> + if (info.secrets_phys && info.secrets_len == PAGE_SIZE) >> + return info.secrets_phys; > > ... which begs the question: how do we verify the HV is not passing some > garbage instead of an actual secrets page? > Unfortunately, the secrets page does not contain a magic header or uuid which a guest can read to verify that the page is actually populated by the PSP. But since the page is encrypted before the launch so this page is always accessed encrypted. If hypervisor is tricking us then all that means is guest OS will get a wrong key and will not be able to communicate with the PSP to get the attestation reports etc. > I guess it is that: > > "SNP_LAUNCH_UPDATE can insert two special pages into the guest’s > memory: the secrets page and the CPUID page. The secrets page contains > encryption keys used by the guest to interact with the firmware. Because > the secrets page is encrypted with the guest’s memory encryption > key, the hypervisor cannot read the keys. The CPUID page contains > hypervisor provided CPUID function values that it passes to the guest. > The firmware validates these values to ensure the hypervisor is not > providing out-of-range values." > > From "4.5 Launching a Guest" in the SNP FW ABI spec. > > I think that explanation above is very important wrt to explaining the > big picture how this all works with those pages injected into the guest > so I guess somewhere around here a comment should say > I will add more explanation. > "See section 4.5 Launching a Guest in the SNP FW ABI spec for details > about those special pages." > > or so. > >> + >> + return 0; >> +} >> + >> +static int __init add_snp_guest_request(void) > > If anything, that should be called > > init_snp_platform_device() > > or so. > Noted. >> +{ >> + struct snp_secrets_page_layout *layout; >> + struct snp_guest_platform_data data; >> + >> + if (!sev_feature_enabled(SEV_SNP)) >> + return -ENODEV; >> + >> + snp_secrets_phys = find_secrets_paddr(); >> + if (!snp_secrets_phys) >> + return -ENODEV; >> + >> + layout = snp_map_secrets_page(); >> + if (!layout) >> + return -ENODEV; >> + >> + /* >> + * The secrets page contains three VMPCK that can be used for > > What's VMPCK? > VM platform communication key. >> + * communicating with the PSP. We choose the VMPCK0 to encrypt guest > > "We" is? > >> + * messages send and receive by the Linux. Provide the key and > > "... by the Linux."?! That sentence needs more love. > I will expand comment a bit more. >> + * id through the platform data to the driver. >> + */ >> + data.vmpck_id = 0; >> + memcpy_fromio(data.vmpck, layout->vmpck0, sizeof(data.vmpck)); >> + >> + iounmap(layout); >> + >> + platform_device_add_data(&guest_req_device, &data, sizeof(data)); > > Oh look, that function can return an error. > Yes, after seeing Dov comment I am adding more checks and return failure. >> + >> + if (!platform_device_register(&guest_req_device)) >> + dev_info(&guest_req_device.dev, "secret phys 0x%llx\n", snp_secrets_phys); > > Make that message human-readable - not a debug one. > Sure. thank
On 02/09/2021 22:58, Brijesh Singh wrote: > > > On 9/2/21 11:40 AM, Borislav Petkov wrote: [...] >> >>> +static u64 find_secrets_paddr(void) >>> +{ >>> + u64 pa_data = boot_params.cc_blob_address; >>> + struct cc_blob_sev_info info; >>> + void *map; >>> + >>> + /* >>> + * The CC blob contains the address of the secrets page, check >>> if the >>> + * blob is present. >>> + */ >>> + if (!pa_data) >>> + return 0; >>> + >>> + map = early_memremap(pa_data, sizeof(info)); >>> + memcpy(&info, map, sizeof(info)); >>> + early_memunmap(map, sizeof(info)); >>> + >>> + /* Verify that secrets page address is passed */ >> >> That's hardly verifying something - if anything, it should say >> >> /* smoke-test the secrets page passed */ >> > Noted. > >>> + if (info.secrets_phys && info.secrets_len == PAGE_SIZE) >>> + return info.secrets_phys; >> >> ... which begs the question: how do we verify the HV is not passing some >> garbage instead of an actual secrets page? >> > > Unfortunately, the secrets page does not contain a magic header or uuid > which a guest can read to verify that the page is actually populated by > the PSP. In the SNP FW ABI document section 8.14.2.5 there's a Table 61 titled Secrets Page Format, which states that the first field in that page is a u32 VERSION field which should equal 2h. While not as strict as GUID header, this can help detect early that the content of the SNP secrets page is invalid. -Dov > But since the page is encrypted before the launch so this page > is always accessed encrypted. If hypervisor is tricking us then all that > means is guest OS will get a wrong key and will not be able to > communicate with the PSP to get the attestation reports etc. > > >> I guess it is that: >> >> "SNP_LAUNCH_UPDATE can insert two special pages into the guest’s >> memory: the secrets page and the CPUID page. The secrets page contains >> encryption keys used by the guest to interact with the firmware. Because >> the secrets page is encrypted with the guest’s memory encryption >> key, the hypervisor cannot read the keys. The CPUID page contains >> hypervisor provided CPUID function values that it passes to the guest. >> The firmware validates these values to ensure the hypervisor is not >> providing out-of-range values." >> >> From "4.5 Launching a Guest" in the SNP FW ABI spec. >> >> I think that explanation above is very important wrt to explaining the >> big picture how this all works with those pages injected into the guest >> so I guess somewhere around here a comment should say >> > > I will add more explanation. > >> "See section 4.5 Launching a Guest in the SNP FW ABI spec for details >> about those special pages." >> >> or so. >>
On 9/3/21 3:15 AM, Dov Murik wrote: >> Unfortunately, the secrets page does not contain a magic header or uuid >> which a guest can read to verify that the page is actually populated by >> the PSP. > In the SNP FW ABI document section 8.14.2.5 there's a Table 61 titled > Secrets Page Format, which states that the first field in that page is a > u32 VERSION field which should equal 2h. > > While not as strict as GUID header, this can help detect early that the > content of the SNP secrets page is invalid. The description indicates that the field is a version number of the secrets page format; it will get bumped every time the spec steals the reserved bytes for something new. IMHO, we should not depend on the version number. thanks
diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c index f42cd5a8e7bb..ab17c93634e9 100644 --- a/arch/x86/kernel/sev.c +++ b/arch/x86/kernel/sev.c @@ -22,6 +22,8 @@ #include <linux/log2.h> #include <linux/efi.h> #include <linux/sev-guest.h> +#include <linux/platform_device.h> +#include <linux/io.h> #include <asm/cpu_entry_area.h> #include <asm/stacktrace.h> @@ -37,6 +39,7 @@ #include <asm/apic.h> #include <asm/efi.h> #include <asm/cpuid.h> +#include <asm/setup.h> #include "sev-internal.h" @@ -2164,3 +2167,68 @@ int snp_issue_guest_request(int type, struct snp_guest_request_data *input, unsi return ret; } EXPORT_SYMBOL_GPL(snp_issue_guest_request); + +static struct platform_device guest_req_device = { + .name = "snp-guest", + .id = -1, +}; + +static u64 find_secrets_paddr(void) +{ + u64 pa_data = boot_params.cc_blob_address; + struct cc_blob_sev_info info; + void *map; + + /* + * The CC blob contains the address of the secrets page, check if the + * blob is present. + */ + if (!pa_data) + return 0; + + map = early_memremap(pa_data, sizeof(info)); + memcpy(&info, map, sizeof(info)); + early_memunmap(map, sizeof(info)); + + /* Verify that secrets page address is passed */ + if (info.secrets_phys && info.secrets_len == PAGE_SIZE) + return info.secrets_phys; + + return 0; +} + +static int __init add_snp_guest_request(void) +{ + struct snp_secrets_page_layout *layout; + struct snp_guest_platform_data data; + + if (!sev_feature_enabled(SEV_SNP)) + return -ENODEV; + + snp_secrets_phys = find_secrets_paddr(); + if (!snp_secrets_phys) + return -ENODEV; + + layout = snp_map_secrets_page(); + if (!layout) + return -ENODEV; + + /* + * The secrets page contains three VMPCK that can be used for + * communicating with the PSP. We choose the VMPCK0 to encrypt guest + * messages send and receive by the Linux. Provide the key and + * id through the platform data to the driver. + */ + data.vmpck_id = 0; + memcpy_fromio(data.vmpck, layout->vmpck0, sizeof(data.vmpck)); + + iounmap(layout); + + platform_device_add_data(&guest_req_device, &data, sizeof(data)); + + if (!platform_device_register(&guest_req_device)) + dev_info(&guest_req_device.dev, "secret phys 0x%llx\n", snp_secrets_phys); + + return 0; +} +device_initcall(add_snp_guest_request); diff --git a/include/linux/sev-guest.h b/include/linux/sev-guest.h index 16b6af24fda7..e1cb3f7dd034 100644 --- a/include/linux/sev-guest.h +++ b/include/linux/sev-guest.h @@ -68,6 +68,11 @@ struct snp_guest_request_data { unsigned int data_npages; }; +struct snp_guest_platform_data { + u8 vmpck_id; + char vmpck[VMPCK_KEY_LEN]; +}; + #ifdef CONFIG_AMD_MEM_ENCRYPT int snp_issue_guest_request(int vmgexit_type, struct snp_guest_request_data *input, unsigned long *fw_err);
Version 2 of GHCB specification provides NAEs that can be used by the SNP guest to communicate with the PSP without risk from a malicious hypervisor who wishes to read, alter, drop or replay the messages sent. In order to communicate with the PSP, the guest need to locate the secrets page inserted by the hypervisor during the SEV-SNP guest launch. The secrets page contains the communication keys used to send and receive the encrypted messages between the guest and the PSP. The secrets page location is passed through the setup_data. Create a platform device that the SNP guest driver can bind to get the platform resources such as encryption key and message id to use to communicate with the PSP. The SNP guest driver can provide userspace interface to get the attestation report, key derivation, extended attestation report etc. Signed-off-by: Brijesh Singh <brijesh.singh@amd.com> --- arch/x86/kernel/sev.c | 68 +++++++++++++++++++++++++++++++++++++++ include/linux/sev-guest.h | 5 +++ 2 files changed, 73 insertions(+)