SEV: Cleanup sev-guest a bit and add throttling
Subsystems (5)
Borislav Petkov
maintainercover SEV: Cleanup sev-guest a bit and add throttling
From: "Borislav Petkov (AMD)" <[email protected]>
Hi,
so I've been looking at Dionna's patches adding the SEV guest throttling and that request issuing spaghetti was getting on my nerves. And it would've become even worse with more stuff piling ontop so here's a first round of cleanups before adding more stuff and making it an unmaintainable mess.
The final result is a lot easier to read with proper separation of functionality between functions. I want to get rid of more input/output params being passed back'n'forth and use a struct instead and Nikunj's patches have stuff which goes in that direction but first things first.
After the cleanup, the new stuff being added is a lot less code and almost trivial. But I've been staring at this for a while now so it could be only me who thinks it is trivial now. But we'll see.
Initial smoke testing seems to work ok but I might've introduced some funky bugs, ofc.
Comments and suggestions are appreciated, as always.
Thanks and thanks, Tom, for the help!
Borislav Petkov (AMD) (7):
virt/coco/sev-guest: Check SEV_SNP attribute at probe time virt/coco/sev-guest: Simplify extended guest request handling virt/coco/sev-guest: Remove the disable_vmpck label in
handle_guest_request()
virt/coco/sev-guest: Carve out the request issuing logic into a helper virt/coco/sev-guest: Do some code style cleanups virt/coco/sev-guest: Convert the sw_exit_info_2 checking to a
switch-case
crypto: ccp: Get rid of __sev_platform_init_locked()'s local function
pointer
Dionna Glaze (3):
virt/coco/sev-guest: Add throttling awareness virt/coco/sev-guest: Double-buffer messages x86/sev: Change snp_guest_issue_request()'s fw_err argument
Peter Gonda (1):
crypto: ccp - Name -1 return value as SEV_RET_NO_FW_CALL
Documentation/virt/coco/sev-guest.rst | 20 ++- arch/x86/include/asm/sev-common.h | 3 - arch/x86/include/asm/sev.h | 4 +- arch/x86/kernel/sev.c | 33 +++-- drivers/crypto/ccp/sev-dev.c | 22 +-- drivers/virt/coco/sev-guest/sev-guest.c | 169 +++++++++++++++--------- include/uapi/linux/psp-sev.h | 7 + include/uapi/linux/sev-guest.h | 18 ++- 8 files changed, 181 insertions(+), 95 deletions(-)
Signature · 1 line
2.35.1
Borislav Petkov
maintainerPATCH 1/11 crypto: ccp - Name -1 return value as SEV_RET_NO_FW_CALL
From: Peter Gonda <[email protected]>
The PSP can return a "firmware error" code of -1 in circumstances where the PSP has not actually been called. To make this protocol unambiguous, name the value SEV_RET_NO_FW_CALL.
[ bp: Massage a bit. ]
--- Documentation/virt/coco/sev-guest.rst | 4 ++-- drivers/crypto/ccp/sev-dev.c | 8 +++++--- include/uapi/linux/psp-sev.h | 7 +++++++ 3 files changed, 14 insertions(+), 5 deletions(-)
Documentation/virt/coco/sev-guest.rst +2 -2
@@ -40,8 +40,8 @@ along with a description: The guest ioctl should be issued on a file descriptor of the /dev/sev-guest device. The ioctl accepts struct snp_user_guest_request. The input and output structure is specified through the req_data and resp_data field respectively. If the ioctl fails -to execute due to a firmware error, then fw_err code will be set otherwise the -fw_err will be set to 0x00000000000000ff. +to execute due to a firmware error, then fw_err code will be set. Otherwise, fw_err +will be set to 0x00000000ffffffff, i.e., the lower 32-bits are -1. The firmware checks that the message sequence counter is one greater than the guests message sequence counter. If guest driver fails to increment message
drivers/crypto/ccp/sev-dev.c +5 -3
@@ -442,10 +442,10 @@ static int __sev_init_ex_locked(int *error)static int __sev_platform_init_locked (int *error) { +int rc =0 , psp_ret =SEV_RET_NO_FW_CALL ;struct psp_device *psp = psp_master; -struct sev_device *sev; -int rc =0 , psp_ret =-1 ;int (*init_function)(int *error); +struct sev_device *sev;if (!psp || !psp->sev_data)return -ENODEV ;
@@ -473,9 +473,11 @@ static int __sev_platform_init_locked(int *error)* initialization function should succeed by replacing the state * with a reset state. */ -dev_err (sev->dev,"SEV: retrying INIT command because of SECURE_DATA_INVALID error. Retrying once to reset PSP SEV state." ); +dev_err (sev->dev, +"SEV: retrying INIT command because of SECURE_DATA_INVALID error. Retrying once to reset PSP SEV state." ); rc =init_function (&psp_ret); } +if (error) *error = psp_ret;
include/uapi/linux/psp-sev.h +7 -0
@@ -36,6 +36,13 @@ enum {* SEV Firmware status code */ typedef enum { + /* + * This error code is not in the SEV spec. Its purpose is to convey that + * there was an error that prevented the SEV firmware from being called. + * The SEV API error codes are 16 bits, so the -1 value will not overlap + * with possible values from the specification. + */ + SEV_RET_NO_FW_CALL = -1, SEV_RET_SUCCESS = 0, SEV_RET_INVALID_PLATFORM_STATE, SEV_RET_INVALID_GUEST_STATE,
Signature · 1 line
2.35.1
Borislav Petkov
maintainerPATCH 2/11 virt/coco/sev-guest: Check SEV_SNP attribute at probe time
In reply to Borislav PetkovFrom: "Borislav Petkov (AMD)" <[email protected]>
No need to check it on every ioctl. And yes, this is a common SEV driver but it does only SNP-specific operations currently. This can be revisited later, when more use cases appear.
No functional changes.
--- arch/x86/kernel/sev.c | 3 --- drivers/virt/coco/sev-guest/sev-guest.c | 3 +++ 2 files changed, 3 insertions(+), 3 deletions(-)
arch/x86/kernel/sev.c +0 -3
@@ -2183,9 +2183,6 @@ int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, unsignedstruct ghcb *ghcb;int ret; -if (!cc_platform_has (CC_ATTR_GUEST_SEV_SNP )) -return -ENODEV ; -if (!fw_err)return -EINVAL ;
drivers/virt/coco/sev-guest/sev-guest.c +3 -0
@@ -689,6 +689,9 @@ static int __init sev_guest_probe(struct platform_device *pdev)void __iomem *mapping;int ret; +if (!cc_platform_has (CC_ATTR_GUEST_SEV_SNP )) +return -ENODEV ; +if (!dev->platform_data)return -ENODEV ;
Signature · 1 line
2.35.1
Borislav Petkov
maintainerPATCH 3/11 virt/coco/sev-guest: Simplify extended guest request handling
In reply to Borislav PetkovFrom: "Borislav Petkov (AMD)" <[email protected]>
Return a specific error code - -ENOSPC - to signal the too small cert data buffer instead of checking exit code and exitinfo2.
--- arch/x86/kernel/sev.c | 8 +++-- drivers/virt/coco/sev-guest/sev-guest.c | 46 ++++++++++++++----------- 2 files changed, 30 insertions(+), 24 deletions(-)
arch/x86/kernel/sev.c +5 -3
@@ -2212,12 +2212,14 @@ int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, unsignedif (ghcb->save.sw_exit_info_2) {/* Number of expected pages are returned in RBX */ if (exit_code ==SVM_VMGEXIT_EXT_GUEST_REQUEST && - ghcb->save.sw_exit_info_2 ==SNP_GUEST_REQ_INVALID_LEN ) + ghcb->save.sw_exit_info_2 ==SNP_GUEST_REQ_INVALID_LEN ) { input->data_npages =ghcb_get_rbx (ghcb); + ret = -ENOSPC ; + }else { + ret = -EIO ; + } *fw_err = ghcb->save.sw_exit_info_2; - - ret = -EIO ; } e_put:
drivers/virt/coco/sev-guest/sev-guest.c +25 -21
74 lines not on this page · raw message
Signature · 1 line
2.35.1
Re: [PATCH 03/11] virt/coco/sev-guest: Simplify extended guest request handling
Quoted 5 lines
if (ghcb->save.sw_exit_info_2) { /* Number of expected pages are returned in RBX */ if (exit_code == SVM_VMGEXIT_EXT_GUEST_REQUEST &&arch/x86/kernel/sev.c +1
-1- ghcb->save.sw_exit_info_2 == SNP_GUEST_REQ_INVALID_LEN) + ghcb->save.sw_exit_info_2 == SNP_GUEST_REQ_INVALID_LEN) {
arch/x86/kernel/sev.c int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, unsigned · line 2223
Quoted 10 lines
arch/x86/kernel/sev.c +4
-2input->data_npages =ghcb_get_rbx (ghcb); + ret = -ENOSPC ; + }else { + ret = -EIO ; + } *fw_err = ghcb->save.sw_exit_info_2; - - ret = -EIO ; }
I think *fw_err still needs to be hoisted out of this if block so that a 0 fw_err gets written back to user space rather than the 0xff placeholder. The 0 would more correctly indicate the lack of an error.
Signature · 1 line
-Dionna Glaze, PhD (she/her)
Borislav Petkov
maintainerPATCH 4/11 virt/coco/sev-guest: Remove the disable_vmpck label in handle_guest_request()
In reply to Borislav PetkovFrom: "Borislav Petkov (AMD)" <[email protected]>
Call the function directly instead.
No functional changes.
--- drivers/virt/coco/sev-guest/sev-guest.c | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-)
drivers/virt/coco/sev-guest/sev-guest.c +4 -6
@@ -388,7 +388,8 @@ static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code, indev_alert (snp_dev->dev,"Detected error from ASP request. rc: %d, fw_err: %llu\n" , rc, *fw_err); - goto disable_vmpck; +snp_disable_vmpck (snp_dev); +return rc; } rc =verify_and_dec_payload (snp_dev, resp_buf, resp_sz);
@@ -396,17 +397,14 @@ static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code, indev_alert (snp_dev->dev,"Detected unexpected decode failure from ASP. rc: %d\n" , rc); - goto disable_vmpck; +snp_disable_vmpck (snp_dev); +return rc; }/* Increment to new message sequence after payload decryption was successful. */ snp_inc_msg_seqno (snp_dev);return 0 ; - -disable_vmpck: -snp_disable_vmpck (snp_dev); -return rc; }static int get_report (struct snp_guest_dev *snp_dev,struct snp_guest_request_ioctl *arg)
Signature · 1 line
2.35.1
Borislav Petkov
maintainerPATCH 5/11 virt/coco/sev-guest: Carve out the request issuing logic into a helper
In reply to Borislav PetkovFrom: "Borislav Petkov (AMD)" <[email protected]>
This makes the code flow a lot easier to follow.
No functional changes.
--- drivers/virt/coco/sev-guest/sev-guest.c | 41 +++++++++++++++---------- 1 file changed, 24 insertions(+), 17 deletions(-)
drivers/virt/coco/sev-guest/sev-guest.c +24 -17
@@ -318,27 +318,12 @@ static int enc_payload(struct snp_guest_dev *snp_dev, u64 seqno, int version, u8return __enc_payload (snp_dev, req, payload, sz); } -static int handle_guest_request (struct snp_guest_dev *snp_dev,u64 exit_code,int msg_ver, -u8 type,void *req_buf,size_t req_sz,void *resp_buf, -u32 resp_sz,__u64 *fw_err) +static int __handle_guest_request (struct snp_guest_dev *snp_dev,u64 exit_code,__u64 *fw_err) {unsigned long err, override_err =0 ;unsigned int override_npages =0 ; -u64 seqno;int rc; -/* Get message sequence and verify that its a non-zero */ - seqno =snp_get_msg_seqno (snp_dev); -if (!seqno) -return -EIO ; - -memset (snp_dev->response,0 ,sizeof (struct snp_guest_msg )); - -/* Encrypt the userspace provided payload */ - rc =enc_payload (snp_dev, seqno, msg_ver, type, req_buf, req_sz); -if (rc) -return rc; - retry_request:/* * Call firmware to process the request. In this function the encrypted
@@ -347,7 +332,6 @@ static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code, in* prevent reuse of the IV. */ rc =snp_issue_guest_request (exit_code, &snp_dev->input, &err); -switch (rc) {case -ENOSPC :/*
@@ -384,6 +368,29 @@ static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code, inif (override_npages) snp_dev->input.data_npages = override_npages; +return rc; +} + +static int handle_guest_request (struct snp_guest_dev *snp_dev,u64 exit_code,int msg_ver, +u8 type,void *req_buf,size_t req_sz,void *resp_buf, +u32 resp_sz,__u64 *fw_err) +{ +u64 seqno; +int rc; + +/* Get message sequence and verify that its a non-zero */ + seqno =snp_get_msg_seqno (snp_dev); +if (!seqno) +return -EIO ; + +memset (snp_dev->response,0 ,sizeof (struct snp_guest_msg )); + +/* Encrypt the userspace provided payload */ + rc =enc_payload (snp_dev, seqno, msg_ver, type, req_buf, req_sz); +if (rc) +return rc; + + rc =__handle_guest_request (snp_dev, exit_code, fw_err);if (rc) { dev_alert(snp_dev->dev, "Detected error from ASP request. rc: %d, fw_err: %llu\n",
Signature · 1 line
2.35.1
Borislav Petkov
maintainerPATCH 6/11 virt/coco/sev-guest: Do some code style cleanups
In reply to Borislav PetkovFrom: "Borislav Petkov (AMD)" <[email protected]>
Remove unnecessary linebreaks, make the code more compact.
No functional changes.
--- drivers/virt/coco/sev-guest/sev-guest.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-)
drivers/virt/coco/sev-guest/sev-guest.c +2 -6
@@ -392,18 +392,14 @@ static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code, in rc =__handle_guest_request (snp_dev, exit_code, fw_err);if (rc) { -dev_alert (snp_dev->dev, -"Detected error from ASP request. rc: %d, fw_err: %llu\n" , - rc, *fw_err); +dev_alert (snp_dev->dev,"Detected error from ASP request. rc: %d, fw_err: %llu\n" , rc, *fw_err);snp_disable_vmpck (snp_dev);return rc; } rc =verify_and_dec_payload (snp_dev, resp_buf, resp_sz);if (rc) { -dev_alert (snp_dev->dev, -"Detected unexpected decode failure from ASP. rc: %d\n" , - rc); +dev_alert (snp_dev->dev,"Detected unexpected decode failure from ASP. rc: %d\n" , rc);snp_disable_vmpck (snp_dev);return rc; }
Signature · 1 line
2.35.1
Borislav Petkov
maintainerPATCH 7/11 virt/coco/sev-guest: Convert the sw_exit_info_2 checking to a switch-case
From: "Borislav Petkov (AMD)" <[email protected]>
snp_issue_guest_request() checks the value returned by the hypervisor in sw_exit_info_2 and returns a different error depending on it.
Convert those checks into a switch-case to make it more readable when more error values are going to be checked in the future.
No functional changes.
--- arch/x86/kernel/sev.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)
arch/x86/kernel/sev.c +12 -6
@@ -2209,17 +2209,23 @@ int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, unsignedif (ret) goto e_put; -if (ghcb->save.sw_exit_info_2) { + *fw_err = ghcb->save.sw_exit_info_2; +switch (*fw_err) { +case 0 : +break ; + +case SNP_GUEST_REQ_INVALID_LEN :/* Number of expected pages are returned in RBX */ -if (exit_code ==SVM_VMGEXIT_EXT_GUEST_REQUEST && - ghcb->save.sw_exit_info_2 ==SNP_GUEST_REQ_INVALID_LEN ) { +if (exit_code ==SVM_VMGEXIT_EXT_GUEST_REQUEST ) { input->data_npages =ghcb_get_rbx (ghcb); ret = -ENOSPC ; - }else { - ret = -EIO ; +break ; } + fallthrough; - *fw_err = ghcb->save.sw_exit_info_2; +default : + ret = -EIO ; +break ; } e_put:
Signature · 1 line
2.35.1
Borislav Petkov
maintainerPATCH 8/11 virt/coco/sev-guest: Add throttling awareness
In reply to Borislav PetkovFrom: Dionna Glaze <[email protected]>
A potentially malicious SEV guest can constantly hammer the hypervisor using this driver to send down requests and thus prevent or at least considerably hinder other guests from issuing requests to the secure processor which is a shared platform resource.
Therefore, the host is permitted and encouraged to throttle such guest requests.
Add the capability to handle the case when the hypervisor throttles excessive numbers of requests issued by the guest. Otherwise, the VM platform communication key will be disabled, preventing the guest from attesting itself.
Realistically speaking, a well-behaved guest should not even care about throttling. During its lifetime, it would end up issuing a handful of requests which the hardware can easily handle.
This is more to address the case of a malicious guest. Such guest should get throttled and if its VMPCK gets disabled, then that's its own wrongdoing and perhaps that guest even deserves it.
To the implementation: the hypervisor signals with SNP_GUEST_REQ_ERR_BUSY that the guest requests should be throttled. That error code is returned in the upper 32-bit half of exitinfo2 and this is part of the GHCB spec v2.
So the guest is given a throttling period of 1 minute in which it retries the request every 2 seconds. This is a good default but if it turns out to not pan out in practice, it can be tweaked later.
For safety, since the encryption algorithm in GHCBv2 is AES_GCM, control must remain in the kernel to complete the request with the current sequence number. Returning without finishing the request allows the guest to make another request but with different message contents. This is IV reuse, and breaks cryptographic protections.
[ bp: Rewrite commit message and do a simplified version. ]
--- arch/x86/include/asm/sev-common.h | 3 ++- arch/x86/kernel/sev.c | 4 ++++ drivers/virt/coco/sev-guest/sev-guest.c | 19 ++++++++++++++++++- 3 files changed, 24 insertions(+), 2 deletions(-)
arch/x86/include/asm/sev-common.h +2 -1
@@ -128,8 +128,9 @@ struct snp_psc_desc {struct psc_entry entries[VMGEXIT_PSC_MAX_ENTRY ]; } __packed; -/* Guest message request error code */ +/* Guest message request error codes */ #define SNP_GUEST_REQ_INVALID_LEN BIT_ULL(32) +#define SNP_GUEST_REQ_ERR_BUSY BIT_ULL(33)#define GHCB_MSR_TERM_REQ 0x100#define GHCB_MSR_TERM_REASON_SET_POS 12
arch/x86/kernel/sev.c +4 -0
@@ -2214,6 +2214,10 @@ int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, unsignedcase 0 :break ; +case SNP_GUEST_REQ_ERR_BUSY : + ret = -EAGAIN ; +break ; +case SNP_GUEST_REQ_INVALID_LEN :/* Number of expected pages are returned in RBX */ if (exit_code ==SVM_VMGEXIT_EXT_GUEST_REQUEST ) {
drivers/virt/coco/sev-guest/sev-guest.c +18 -1
@@ -31,6 +31,9 @@#define AAD_LEN 48#define MSG_HDR_VER 1 +#define SNP_REQ_MAX_RETRY_DURATION (60*HZ) +#define SNP_REQ_RETRY_DELAY (2*HZ) +struct snp_guest_crypto {struct crypto_aead *tfm;u8 *iv, *authtag;
@@ -320,8 +323,9 @@ static int enc_payload(struct snp_guest_dev *snp_dev, u64 seqno, int version, u8static int __handle_guest_request (struct snp_guest_dev *snp_dev,u64 exit_code,__u64 *fw_err) { -unsigned long err, override_err =0 ; +unsigned long err =0xff , override_err =0 ;unsigned int override_npages =0 ; +unsigned long req_start = jiffies;int rc; retry_request:
@@ -360,6 +364,19 @@ static int __handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code,* user as an ioctl() return code. */ goto retry_request; + +/* +* The host may return SNP_GUEST_REQ_ERR_EBUSY if the request has been +* throttled. Retry in the driver to avoid returning and reusing the +* message sequence number on a different message. +*/ +case -EAGAIN : +if (jiffies - req_start >SNP_REQ_MAX_RETRY_DURATION ) { + rc = -ETIMEDOUT ; +break ; + } +schedule_timeout_killable (SNP_REQ_RETRY_DELAY ); + goto retry_request; }if (fw_err)
Signature · 1 line
2.35.1
Borislav Petkov
maintainerPATCH 9/11 virt/coco/sev-guest: Double-buffer messages
In reply to Borislav PetkovFrom: Dionna Glaze <[email protected]>
The encryption algorithms read and write directly to shared unencrypted memory, which may leak information as well as permit the host to tamper with the message integrity. Instead, copy whole messages in or out as needed before doing any computation on them.
--- drivers/virt/coco/sev-guest/sev-guest.c | 27 +++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-)
drivers/virt/coco/sev-guest/sev-guest.c +23 -4
@@ -46,7 +46,15 @@ struct snp_guest_dev {void *certs_data;struct snp_guest_crypto *crypto; +/* request and response are in unencrypted memory */ struct snp_guest_msg *request, *response; + +/* +* Avoid information leakage by double-buffering shared messages +* in fields that are in regular encrypted memory. +*/ +struct snp_guest_msg secret_request, secret_response; +struct snp_secrets_page_layout *layout;struct snp_req_data input;u32 *os_area_msg_seqno;
@@ -266,14 +274,17 @@ static int dec_payload(struct snp_guest_dev *snp_dev, struct snp_guest_msg *msg,static int verify_and_dec_payload (struct snp_guest_dev *snp_dev,void *payload,u32 sz) {struct snp_guest_crypto *crypto = snp_dev->crypto; -struct snp_guest_msg *resp = snp_dev->response; -struct snp_guest_msg *req = snp_dev->request; +struct snp_guest_msg *resp = &snp_dev->secret_response; +struct snp_guest_msg *req = &snp_dev->secret_request;struct snp_guest_msg_hdr *req_hdr = &req->hdr;struct snp_guest_msg_hdr *resp_hdr = &resp->hdr;dev_dbg (snp_dev->dev,"response [seqno %lld type %d version %d sz %d]\n" , resp_hdr->msg_seqno, resp_hdr->msg_type, resp_hdr->msg_version, resp_hdr->msg_sz); +/* Copy response from shared memory to encrypted memory. */ +memcpy (resp, snp_dev->response,sizeof (*resp)); +/* Verify that the sequence counter is incremented by 1 */ if (unlikely (resp_hdr->msg_seqno != (req_hdr->msg_seqno +1 )))return -EBADMSG ;
@@ -297,7 +308,7 @@ static int verify_and_dec_payload(struct snp_guest_dev *snp_dev, void *payload,static int enc_payload (struct snp_guest_dev *snp_dev,u64 seqno,int version,u8 type,void *payload,size_t sz) { -struct snp_guest_msg *req = snp_dev->request; +struct snp_guest_msg *req = &snp_dev->secret_request;struct snp_guest_msg_hdr *hdr = &req->hdr;memset (req,0 ,sizeof (*req));
@@ -400,13 +411,21 @@ static int handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code, inif (!seqno)return -EIO ; +/* Clear shared memory's response for the host to populate. */ memset (snp_dev->response,0 ,sizeof (struct snp_guest_msg )); -/* Encrypt the userspace provided payload */ +/* Encrypt the userspace provided payload in snp_dev->secret_request. */ rc =enc_payload (snp_dev, seqno, msg_ver, type, req_buf, req_sz);if (rc)return rc; +/* +* Write the fully encrypted request to the shared unencrypted +* request page. +*/ +memcpy (snp_dev->request, &snp_dev->secret_request, +sizeof (snp_dev->secret_request)); + rc =__handle_guest_request (snp_dev, exit_code, fw_err);if (rc) {dev_alert (snp_dev->dev,"Detected error from ASP request. rc: %d, fw_err: %llu\n" , rc, *fw_err);
Signature · 1 line
2.35.1
Borislav Petkov
maintainerPATCH 10/11 crypto: ccp: Get rid of __sev_platform_init_locked()'s local function pointer
In reply to Borislav PetkovFrom: "Borislav Petkov (AMD)" <[email protected]>
Add a wrapper instead.
No functional changes.
--- drivers/crypto/ccp/sev-dev.c | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)
drivers/crypto/ccp/sev-dev.c +10 -6
@@ -440,11 +440,18 @@ static int __sev_init_ex_locked(int *error)return __sev_do_cmd_locked (SEV_CMD_INIT_EX , &data, error); } +static inline int __sev_do_init_locked (int *psp_ret) +{ +if (sev_init_ex_buffer) +return __sev_init_ex_locked (psp_ret); +else +return __sev_init_locked (psp_ret); +} +static int __sev_platform_init_locked (int *error) {int rc =0 , psp_ret =SEV_RET_NO_FW_CALL ;struct psp_device *psp = psp_master; -int (*init_function)(int *error);struct sev_device *sev;if (!psp || !psp->sev_data)
@@ -456,15 +463,12 @@ static int __sev_platform_init_locked(int *error)return 0 ;if (sev_init_ex_buffer) { - init_function = __sev_init_ex_locked; rc =sev_read_init_ex_file ();if (rc)return rc; - }else { - init_function = __sev_init_locked; } - rc =init_function (&psp_ret); + rc =__sev_do_init_locked (&psp_ret);if (rc && psp_ret ==SEV_RET_SECURE_DATA_INVALID ) {/* * Initialization command returned an integrity check failure
@@ -475,7 +479,7 @@ static int __sev_platform_init_locked(int *error)*/ dev_err (sev->dev,"SEV: retrying INIT command because of SECURE_DATA_INVALID error. Retrying once to reset PSP SEV state." ); - rc =init_function (&psp_ret); + rc =__sev_do_init_locked (&psp_ret); }if (error)
Signature · 1 line
2.35.1
Borislav Petkov
maintainerPATCH 11/11 x86/sev: Change snp_guest_issue_request()'s fw_err argument
In reply to Borislav PetkovFrom: Dionna Glaze <[email protected]>
The GHCB specification declares that the firmware error value for a guest request will be stored in the lower 32 bits of EXIT_INFO_2. The upper 32 bits are for the VMM's own error code. The fw_err argument to snp_guest_issue_request() is thus a misnomer, and callers will need access to all 64 bits.
The type of unsigned long also causes problems, since sw_exit_info2 is u64 (unsigned long long) vs the argument's unsigned long*. Change this type for issuing the guest request. Pass the ioctl command struct's error field directly instead of in a local variable, since an incomplete guest request may not set the error code, and uninitialized stack memory would be written back to user space.
The firmware might not even be called, so bookend the call with the no firmware call error and clear the error.
Since the "fw_err" field is really exitinfo2 split into the upper bits' vmm error code and lower bits' firmware error code, convert the 64 bit value to a union.
[ bp: Massage commit message, adjust code. ]
--- Documentation/virt/coco/sev-guest.rst | 20 ++++++---- arch/x86/include/asm/sev-common.h | 4 -- arch/x86/include/asm/sev.h | 4 +- arch/x86/kernel/sev.c | 16 +++++--- drivers/virt/coco/sev-guest/sev-guest.c | 51 ++++++++++++------------- include/uapi/linux/sev-guest.h | 18 ++++++++- 6 files changed, 66 insertions(+), 47 deletions(-)
Documentation/virt/coco/sev-guest.rst +13 -7
@@ -37,11 +37,11 @@ along with a description: the return value. General error numbers (-ENOMEM, -EINVAL) are not detailed, but errors with specific meanings are. -The guest ioctl should be issued on a file descriptor of the /dev/sev-guest device. -The ioctl accepts struct snp_user_guest_request. The input and output structure is -specified through the req_data and resp_data field respectively. If the ioctl fails -to execute due to a firmware error, then fw_err code will be set. Otherwise, fw_err -will be set to 0x00000000ffffffff, i.e., the lower 32-bits are -1. +The guest ioctl should be issued on a file descriptor of the /dev/sev-guest +device. The ioctl accepts struct snp_user_guest_request. The input and +output structure is specified through the req_data and resp_data field +respectively. If the ioctl fails to execute due to a firmware error, then +the fw_error code will be set, otherwise fw_error will be set to -1. The firmware checks that the message sequence counter is one greater than the guests message sequence counter. If guest driver fails to increment message
@@ -57,8 +57,14 @@ counter (e.g. counter overflow), then -EIO will be returned. __u64 req_data; __u64 resp_data; - /* firmware error code on failure (see psp-sev.h) */ - __u64 fw_err; + /* bits[63:32]: VMM error code, bits[31:0] firmware error code (see psp-sev.h) */ + union { + __u64 exitinfo2; + struct { + __u32 fw_error; + __u32 vmm_error; + }; + }; }; 2.1 SNP_GET_REPORT
arch/x86/include/asm/sev-common.h +0 -4
@@ -128,10 +128,6 @@ struct snp_psc_desc {struct psc_entry entries[VMGEXIT_PSC_MAX_ENTRY ]; } __packed; -/* Guest message request error codes */ -#define SNP_GUEST_REQ_INVALID_LEN BIT_ULL(32) -#define SNP_GUEST_REQ_ERR_BUSY BIT_ULL(33) -#define GHCB_MSR_TERM_REQ 0x100#define GHCB_MSR_TERM_REASON_SET_POS 12#define GHCB_MSR_TERM_REASON_SET_MASK 0xf
arch/x86/include/asm/sev.h +2 -2
@@ -196,7 +196,7 @@ void snp_set_memory_private(unsigned long vaddr, unsigned int npages);void snp_set_wakeup_secondary_cpu (void );bool snp_init (struct boot_params *bp);void __init __noreturn snp_abort (void ); -int snp_issue_guest_request (u64 exit_code,struct snp_req_data *input,unsigned long *fw_err); +int snp_issue_guest_request (u64 exit_code,struct snp_req_data *input,u64 *exitinfo2);#else static inline void sev_es_ist_enter (struct pt_regs *regs) { }static inline void sev_es_ist_exit (void ) { }
@@ -217,7 +217,7 @@ static inline void snp_set_wakeup_secondary_cpu(void) { }static inline bool snp_init (struct boot_params *bp) {return false; }static inline void snp_abort (void ) { }static inline int snp_issue_guest_request (u64 exit_code,struct snp_req_data *input, -unsigned long *fw_err) +u64 *exitinfo2) {return -ENOTTY ; }
arch/x86/kernel/sev.c +10 -6
@@ -22,6 +22,8 @@#include <linux/efi.h> #include <linux/platform_device.h> #include <linux/io.h> +#include <linux/psp-sev.h> +#include <uapi/linux/sev-guest.h> #include <asm/cpu_entry_area.h> #include <asm/stacktrace.h>
@@ -2175,7 +2177,7 @@ static int __init init_sev_config(char *str) }__setup ("sev=" , init_sev_config); -int snp_issue_guest_request (u64 exit_code,struct snp_req_data *input,unsigned long *fw_err) +int snp_issue_guest_request (u64 exit_code,struct snp_req_data *input,u64 *exitinfo2) {struct ghcb_state state;struct es_em_ctxt ctxt;
@@ -2183,9 +2185,11 @@ int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, unsignedstruct ghcb *ghcb;int ret; -if (!fw_err) +if (!exitinfo2)return -EINVAL ; + *exitinfo2 =SEV_RET_NO_FW_CALL ; +/* * __sev_get_ghcb() needs to run with IRQs disabled because it is using * a per-CPU GHCB.
@@ -2209,16 +2213,16 @@ int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, unsignedif (ret) goto e_put; - *fw_err = ghcb->save.sw_exit_info_2; -switch (*fw_err) { + *exitinfo2 = ghcb->save.sw_exit_info_2; +switch (*exitinfo2) {case 0 :break ; -case SNP_GUEST_REQ_ERR_BUSY : +case SNP_GUEST_VMM_ERR (SNP_GUEST_VMM_ERR_BUSY ): ret = -EAGAIN ;break ; -case SNP_GUEST_REQ_INVALID_LEN : +case SNP_GUEST_VMM_ERR (SNP_GUEST_VMM_ERR_INVALID_LEN ):/* Number of expected pages are returned in RBX */ if (exit_code ==SVM_VMGEXIT_EXT_GUEST_REQUEST ) { input->data_npages =ghcb_get_rbx (ghcb);
drivers/virt/coco/sev-guest/sev-guest.c +25 -26
142 lines not on this page · raw message
include/uapi/linux/sev-guest.h +16 -2
@@ -52,8 +52,14 @@ struct snp_guest_request_ioctl {__u64 req_data;__u64 resp_data; -/* firmware error code on failure (see psp-sev.h) */ -__u64 fw_err; +/* bits[63:32]: VMM error code, bits[31:0] firmware error code (see psp-sev.h) */ +union { +__u64 exitinfo2; +struct { +__u32 fw_error; +__u32 vmm_error; + }; + }; };struct snp_ext_report_req {
@@ -77,4 +83,12 @@ struct snp_ext_report_req {/* Get SNP extended report as defined in the GHCB specification version 2. */ #define SNP_GET_EXT_REPORT _IOWR(SNP_GUEST_REQ_IOC_TYPE, 0x2, struct snp_guest_request_ioctl) +/* Guest message request EXIT_INFO_2 constants */ +#define SNP_GUEST_FW_ERR_MASK GENMASK_ULL(31, 0) +#define SNP_GUEST_VMM_ERR_SHIFT 32 +#define SNP_GUEST_VMM_ERR (x) (((u64)x) << SNP_GUEST_VMM_ERR_SHIFT) + +#define SNP_GUEST_VMM_ERR_INVALID_LEN 1 +#define SNP_GUEST_VMM_ERR_BUSY 2 +#endif /* __UAPI_LINUX_SEV_GUEST_H_ */
Signature · 1 line
2.35.1
Borislav Petkov
maintainerRe: [PATCH 11/11] x86/sev: Change snp_guest_issue_request()'s fw_err argument
drivers/virt/coco/sev-guest/sev-guest.c static int __handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code, · line 395
Quoted 7 lines · Borislav Petkov
drivers/virt/coco/sev-guest/sev-guest.c +0
-3@@ -390,18 +390,16 @@ static int __handle_guest_request(struct snp_guest_dev *snp_dev, u64 exit_code, goto retry_request; } -if (fw_err) - *fw_err = override_err ?: err; -
As Tom pointed out offlist, we still need to do the error override. Frankly, I'm not crazy about this request retrying as we do it but I guess it is what it is and it is too late to change that now.
Signature · 4 lines
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette