Message ID | 20221027150558.722062-3-pgonda@google.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Fix security issue in SNP guest AES-GCM usage | expand |
I think just no to this patch? Reasons below. > > if (ghcb->save.sw_exit_info_2) { > - /* Number of expected pages are returned in RBX */ > + /* For a SNP Extended Request, if the request was placed with > + * insufficient data pages. The host will return the number of > + * pages required using RBX in the GHCB. We can than retry the > + * call as an SNP Request to fulfill the command without getting > + * the extended request data. > + */ > if (exit_code == SVM_VMGEXIT_EXT_GUEST_REQUEST && > - ghcb->save.sw_exit_info_2 == SNP_GUEST_REQ_INVALID_LEN) > - input->data_npages = ghcb_get_rbx(ghcb); > - > - *fw_err = ghcb->save.sw_exit_info_2; > + ghcb->save.sw_exit_info_2 == SNP_GUEST_REQ_INVALID_LEN) { > + int npages = ghcb_get_rbx(ghcb); > + > + ghcb_clear_rax(ghcb); > + ghcb_clear_rbx(ghcb); > + > + ret = sev_es_ghcb_hv_call(ghcb, &ctxt, > + SVM_VMGEXIT_GUEST_REQUEST, > + input->req_gpa, > + input->resp_gpa); > + if (ret) > + goto e_put; > + I'm not keen on reissuing the call in this function. I think issue_request should do its job of sending a request to the host and returning the specified data, in this case the number of pages in RBX. I know it's not particularly fun to interpret exitinfo2 in a couple places, but it serves a purpose. We don't want this function to grow to have special cases for all the commands that can be sent to the psp if they don't involve data passed back through the GHCB. The get_ghcb/put_ghcb frame is the only thing we really need to respect in here. The sev-guest device owns the VMPCKn keys, the message sequence number, and the responsibility of sending a coherent response back to user space. When we account for the host changing the certificate page length during the request and not wanting to return to the guest without completing the firmware call, the length might grow past the 4KiB max constant we have so far. The driver can make the choice of issuing the request without extended data like you do here, or to reallocate its cert_data buffer and ask for the extended data again. It shouldn't matter to the core functionality of issuing a single request. When throttling comes into play and retrying needs to happen more than once, then we're in another situation where the sev-guest driver also owns the responsibility of trying not to get throttled too hard. My patches suggest a 2HZ rate limit to avoid any big penalties of running requests in a tight loop and looking like a DoS antagonist, but that doesn't belong in arch/x86/kernel/sev.c due to the variability of strategies. > + input->data_npages = npages; > + *fw_err = SNP_GUEST_REQ_INVALID_LEN; > + } else > + *fw_err = ghcb->save.sw_exit_info_2; I think in both branches of the conditional, fw_err gets set to exit_info_2. See v4 of my throttling patch series.
diff --git a/arch/x86/include/asm/svm.h b/arch/x86/include/asm/svm.h index 0361626841bc..3886b8ea18ae 100644 --- a/arch/x86/include/asm/svm.h +++ b/arch/x86/include/asm/svm.h @@ -585,6 +585,12 @@ struct vmcb { (unsigned long *)&ghcb->save.valid_bitmap); \ } \ \ + static __always_inline void ghcb_clear_##field(const struct ghcb *ghcb) \ + { \ + clear_bit(GHCB_BITMAP_IDX(field), \ + (unsigned long *)&ghcb->save.valid_bitmap); \ + } \ + \ static __always_inline u64 ghcb_get_##field(struct ghcb *ghcb) \ { \ return ghcb->save.field; \ diff --git a/arch/x86/kernel/sev.c b/arch/x86/kernel/sev.c index a428c62330d3..3f7e2105ef97 100644 --- a/arch/x86/kernel/sev.c +++ b/arch/x86/kernel/sev.c @@ -2213,12 +2213,30 @@ int snp_issue_guest_request(u64 exit_code, struct snp_req_data *input, unsigned goto e_put; if (ghcb->save.sw_exit_info_2) { - /* Number of expected pages are returned in RBX */ + /* For a SNP Extended Request, if the request was placed with + * insufficient data pages. The host will return the number of + * pages required using RBX in the GHCB. We can than retry the + * call as an SNP Request to fulfill the command without getting + * the extended request data. + */ if (exit_code == SVM_VMGEXIT_EXT_GUEST_REQUEST && - ghcb->save.sw_exit_info_2 == SNP_GUEST_REQ_INVALID_LEN) - input->data_npages = ghcb_get_rbx(ghcb); - - *fw_err = ghcb->save.sw_exit_info_2; + ghcb->save.sw_exit_info_2 == SNP_GUEST_REQ_INVALID_LEN) { + int npages = ghcb_get_rbx(ghcb); + + ghcb_clear_rax(ghcb); + ghcb_clear_rbx(ghcb); + + ret = sev_es_ghcb_hv_call(ghcb, &ctxt, + SVM_VMGEXIT_GUEST_REQUEST, + input->req_gpa, + input->resp_gpa); + if (ret) + goto e_put; + + input->data_npages = npages; + *fw_err = SNP_GUEST_REQ_INVALID_LEN; + } else + *fw_err = ghcb->save.sw_exit_info_2; ret = -EIO; } diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c index 8c54ea84bc57..ede07c0ec0c3 100644 --- a/drivers/virt/coco/sev-guest/sev-guest.c +++ b/drivers/virt/coco/sev-guest/sev-guest.c @@ -496,7 +496,7 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques if (!resp) return -ENOMEM; - snp_dev->input.data_npages = sizeof(*snp_dev->certs_data) >> PAGE_SHIFT; + snp_dev->input.data_npages = req_cert_len >> PAGE_SHIFT; ret = handle_guest_request(snp_dev, SVM_VMGEXIT_EXT_GUEST_REQUEST, arg->msg_version, SNP_MSG_REPORT_REQ, &req.data, sizeof(req.data), resp->data, resp_len, &arg->fw_err);
If an SNP Extended Request is placed without enough data pages the host will return an error to the guest and tell it the number of required data pages. If we place an extended request without enogh data page we can retry the command portion of the request using an SNP Request. This allows us to keep our command sequence numbers with the ASP in sync while also supporting the SNP Extended Request's data size querying capability. This happens inside of snp_issue_guest_request() to keep the safety of the sequence numbers easy. Any failure snp_issue_guest_request() should result in no further of the VMCPK due to issues with the sequence number being the IV in the AES-GCM communication channel. IV reuse, meaning sequence number reuse in this driver, can result in the secure channel being compromised. Signed-off-by: Peter Gonda <pgonda@google.com> Cc: Borislav Petkov <bp@suse.de> Cc: Tom Lendacky <thomas.lendacky@amd.com> Cc: Michael Roth <michael.roth@amd.com> Cc: Haowen Bai <baihaowen@meizu.com> Cc: Yang Yingliang <yangyingliang@huawei.com> Cc: Marc Orr <marcorr@google.com> Cc: David Rientjes <rientjes@google.com> Cc: Ashish Kalra <Ashish.Kalra@amd.com> Cc: linux-kernel@vger.kernel.org Cc: kvm@vger.kernel.org --- arch/x86/include/asm/svm.h | 6 ++++++ arch/x86/kernel/sev.c | 28 ++++++++++++++++++++----- drivers/virt/coco/sev-guest/sev-guest.c | 2 +- 3 files changed, 30 insertions(+), 6 deletions(-)