* [PATCH 1/2] virt: sev-guest: Allocate request data dynamically [not found] <20250307013700.437505-1-aik@amd.com> @ 2025-03-07 1:36 ` Alexey Kardashevskiy 2025-03-07 10:52 ` Alexey Kardashevskiy ` (2 more replies) 2025-03-07 1:37 ` [PATCH 2/2] virt: sev-guest: Move SNP Guest Request data pages handling under snp_cmd_mutex Alexey Kardashevskiy 1 sibling, 3 replies; 8+ messages in thread From: Alexey Kardashevskiy @ 2025-03-07 1:36 UTC (permalink / raw) To: x86 Cc: linux-kernel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, H. Peter Anvin, Tom Lendacky, Nikunj A Dadhania, Ard Biesheuvel, Pavan Kumar Paluri, Ashish Kalra, Paolo Bonzini, Michael Roth, Kevin Loughlin, Kuppuswamy Sathyanarayanan, Brijesh Singh, Liam Merwick, Alexey Kardashevskiy, stable, andreas.stuehrk From: Nikunj A Dadhania <nikunj@amd.com> Commit ae596615d93d ("virt: sev-guest: Reduce the scope of SNP command mutex") narrowed the command mutex scope to snp_send_guest_request. However, GET_REPORT, GET_DERIVED_KEY, and GET_EXT_REPORT share the req structure in snp_guest_dev. Without the mutex protection, concurrent requests can overwrite each other's data. Fix it by dynamically allocating the request structure. Fixes: ae596615d93d ("virt: sev-guest: Reduce the scope of SNP command mutex") Cc: stable@vger.kernel.org Reported-by: andreas.stuehrk@yaxi.tech Closes: https://github.com/AMDESE/AMDSEV/issues/265 Signed-off-by: Nikunj A Dadhania <nikunj@amd.com> --- drivers/virt/coco/sev-guest/sev-guest.c | 24 ++++++++++++-------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c index ddec5677e247..4699fdc9ed44 100644 --- a/drivers/virt/coco/sev-guest/sev-guest.c +++ b/drivers/virt/coco/sev-guest/sev-guest.c @@ -39,12 +39,6 @@ struct snp_guest_dev { struct miscdevice misc; struct snp_msg_desc *msg_desc; - - union { - struct snp_report_req report; - struct snp_derived_key_req derived_key; - struct snp_ext_report_req ext_report; - } req; }; /* @@ -72,7 +66,7 @@ struct snp_req_resp { static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg) { - struct snp_report_req *report_req = &snp_dev->req.report; + struct snp_report_req *report_req __free(kfree) = NULL; struct snp_msg_desc *mdesc = snp_dev->msg_desc; struct snp_report_resp *report_resp; struct snp_guest_req req = {}; @@ -81,6 +75,10 @@ static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_io if (!arg->req_data || !arg->resp_data) return -EINVAL; + report_req = kzalloc(sizeof(*report_req), GFP_KERNEL_ACCOUNT); + if (!report_req) + return -ENOMEM; + if (copy_from_user(report_req, (void __user *)arg->req_data, sizeof(*report_req))) return -EFAULT; @@ -117,7 +115,7 @@ static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_io static int get_derived_key(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg) { - struct snp_derived_key_req *derived_key_req = &snp_dev->req.derived_key; + struct snp_derived_key_req *derived_key_req __free(kfree) = NULL; struct snp_derived_key_resp derived_key_resp = {0}; struct snp_msg_desc *mdesc = snp_dev->msg_desc; struct snp_guest_req req = {}; @@ -137,6 +135,10 @@ static int get_derived_key(struct snp_guest_dev *snp_dev, struct snp_guest_reque if (sizeof(buf) < resp_len) return -ENOMEM; + derived_key_req = kzalloc(sizeof(*derived_key_req), GFP_KERNEL_ACCOUNT); + if (!derived_key_req) + return -ENOMEM; + if (copy_from_user(derived_key_req, (void __user *)arg->req_data, sizeof(*derived_key_req))) return -EFAULT; @@ -169,7 +171,7 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques struct snp_req_resp *io) { - struct snp_ext_report_req *report_req = &snp_dev->req.ext_report; + struct snp_ext_report_req *report_req __free(kfree) = NULL; struct snp_msg_desc *mdesc = snp_dev->msg_desc; struct snp_report_resp *report_resp; struct snp_guest_req req = {}; @@ -179,6 +181,10 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques if (sockptr_is_null(io->req_data) || sockptr_is_null(io->resp_data)) return -EINVAL; + report_req = kzalloc(sizeof(*report_req), GFP_KERNEL_ACCOUNT); + if (!report_req) + return -ENOMEM; + if (copy_from_sockptr(report_req, io->req_data, sizeof(*report_req))) return -EFAULT; -- 2.47.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] virt: sev-guest: Allocate request data dynamically 2025-03-07 1:36 ` [PATCH 1/2] virt: sev-guest: Allocate request data dynamically Alexey Kardashevskiy @ 2025-03-07 10:52 ` Alexey Kardashevskiy 2025-03-07 18:25 ` [tip: x86/urgent] " tip-bot2 for Nikunj A Dadhania 2025-03-10 2:14 ` [PATCH 1/2] " Sasha Levin 2 siblings, 0 replies; 8+ messages in thread From: Alexey Kardashevskiy @ 2025-03-07 10:52 UTC (permalink / raw) To: x86 Cc: linux-kernel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, H. Peter Anvin, Tom Lendacky, Nikunj A Dadhania, Ard Biesheuvel, Pavan Kumar Paluri, Ashish Kalra, Paolo Bonzini, Michael Roth, Kevin Loughlin, Kuppuswamy Sathyanarayanan, Brijesh Singh, Liam Merwick, stable, andreas.stuehrk On 7/3/25 12:36, Alexey Kardashevskiy wrote: > From: Nikunj A Dadhania <nikunj@amd.com> > > Commit ae596615d93d ("virt: sev-guest: Reduce the scope of SNP command > mutex") narrowed the command mutex scope to snp_send_guest_request. > However, GET_REPORT, GET_DERIVED_KEY, and GET_EXT_REPORT share the req > structure in snp_guest_dev. Without the mutex protection, concurrent > requests can overwrite each other's data. Fix it by dynamically allocating > the request structure. > > Fixes: ae596615d93d ("virt: sev-guest: Reduce the scope of SNP command mutex") > Cc: stable@vger.kernel.org > Reported-by: andreas.stuehrk@yaxi.tech > Closes: https://github.com/AMDESE/AMDSEV/issues/265 > Signed-off-by: Nikunj A Dadhania <nikunj@amd.com> oh. forgot: Signed-off-by: Alexey Kardashevskiy <aik@amd.com> > --- > drivers/virt/coco/sev-guest/sev-guest.c | 24 ++++++++++++-------- > 1 file changed, 15 insertions(+), 9 deletions(-) > > diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c > index ddec5677e247..4699fdc9ed44 100644 > --- a/drivers/virt/coco/sev-guest/sev-guest.c > +++ b/drivers/virt/coco/sev-guest/sev-guest.c > @@ -39,12 +39,6 @@ struct snp_guest_dev { > struct miscdevice misc; > > struct snp_msg_desc *msg_desc; > - > - union { > - struct snp_report_req report; > - struct snp_derived_key_req derived_key; > - struct snp_ext_report_req ext_report; > - } req; > }; > > /* > @@ -72,7 +66,7 @@ struct snp_req_resp { > > static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg) > { > - struct snp_report_req *report_req = &snp_dev->req.report; > + struct snp_report_req *report_req __free(kfree) = NULL; > struct snp_msg_desc *mdesc = snp_dev->msg_desc; > struct snp_report_resp *report_resp; > struct snp_guest_req req = {}; > @@ -81,6 +75,10 @@ static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_io > if (!arg->req_data || !arg->resp_data) > return -EINVAL; > > + report_req = kzalloc(sizeof(*report_req), GFP_KERNEL_ACCOUNT); > + if (!report_req) > + return -ENOMEM; > + > if (copy_from_user(report_req, (void __user *)arg->req_data, sizeof(*report_req))) > return -EFAULT; > > @@ -117,7 +115,7 @@ static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_io > > static int get_derived_key(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg) > { > - struct snp_derived_key_req *derived_key_req = &snp_dev->req.derived_key; > + struct snp_derived_key_req *derived_key_req __free(kfree) = NULL; > struct snp_derived_key_resp derived_key_resp = {0}; > struct snp_msg_desc *mdesc = snp_dev->msg_desc; > struct snp_guest_req req = {}; > @@ -137,6 +135,10 @@ static int get_derived_key(struct snp_guest_dev *snp_dev, struct snp_guest_reque > if (sizeof(buf) < resp_len) > return -ENOMEM; > > + derived_key_req = kzalloc(sizeof(*derived_key_req), GFP_KERNEL_ACCOUNT); > + if (!derived_key_req) > + return -ENOMEM; > + > if (copy_from_user(derived_key_req, (void __user *)arg->req_data, > sizeof(*derived_key_req))) > return -EFAULT; > @@ -169,7 +171,7 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques > struct snp_req_resp *io) > > { > - struct snp_ext_report_req *report_req = &snp_dev->req.ext_report; > + struct snp_ext_report_req *report_req __free(kfree) = NULL; > struct snp_msg_desc *mdesc = snp_dev->msg_desc; > struct snp_report_resp *report_resp; > struct snp_guest_req req = {}; > @@ -179,6 +181,10 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques > if (sockptr_is_null(io->req_data) || sockptr_is_null(io->resp_data)) > return -EINVAL; > > + report_req = kzalloc(sizeof(*report_req), GFP_KERNEL_ACCOUNT); > + if (!report_req) > + return -ENOMEM; > + > if (copy_from_sockptr(report_req, io->req_data, sizeof(*report_req))) > return -EFAULT; > -- Alexey ^ permalink raw reply [flat|nested] 8+ messages in thread
* [tip: x86/urgent] virt: sev-guest: Allocate request data dynamically 2025-03-07 1:36 ` [PATCH 1/2] virt: sev-guest: Allocate request data dynamically Alexey Kardashevskiy 2025-03-07 10:52 ` Alexey Kardashevskiy @ 2025-03-07 18:25 ` tip-bot2 for Nikunj A Dadhania 2025-03-10 2:14 ` [PATCH 1/2] " Sasha Levin 2 siblings, 0 replies; 8+ messages in thread From: tip-bot2 for Nikunj A Dadhania @ 2025-03-07 18:25 UTC (permalink / raw) To: linux-tip-commits Cc: andreas.stuehrk, Nikunj A Dadhania, Alexey Kardashevskiy, Borislav Petkov (AMD), stable, x86, linux-kernel The following commit has been merged into the x86/urgent branch of tip: Commit-ID: ac7c06acaa3738b38e83815ac0f07140ad320f13 Gitweb: https://git.kernel.org/tip/ac7c06acaa3738b38e83815ac0f07140ad320f13 Author: Nikunj A Dadhania <nikunj@amd.com> AuthorDate: Thu, 06 Mar 2025 19:17:21 +11:00 Committer: Borislav Petkov (AMD) <bp@alien8.de> CommitterDate: Fri, 07 Mar 2025 13:34:25 +01:00 virt: sev-guest: Allocate request data dynamically Commit ae596615d93d ("virt: sev-guest: Reduce the scope of SNP command mutex") narrowed the command mutex scope to snp_send_guest_request(). However, GET_REPORT, GET_DERIVED_KEY, and GET_EXT_REPORT share the req structure in snp_guest_dev. Without the mutex protection, concurrent requests can overwrite each other's data. Fix it by dynamically allocating the request structure. Fixes: ae596615d93d ("virt: sev-guest: Reduce the scope of SNP command mutex") Closes: https://github.com/AMDESE/AMDSEV/issues/265 Reported-by: andreas.stuehrk@yaxi.tech Signed-off-by: Nikunj A Dadhania <nikunj@amd.com> Signed-off-by: Alexey Kardashevskiy <aik@amd.com> Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de> Cc: stable@vger.kernel.org Link: https://lore.kernel.org/r/20250307013700.437505-2-aik@amd.com --- drivers/virt/coco/sev-guest/sev-guest.c | 24 +++++++++++++++--------- 1 file changed, 15 insertions(+), 9 deletions(-) diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c index 264b652..23ac177 100644 --- a/drivers/virt/coco/sev-guest/sev-guest.c +++ b/drivers/virt/coco/sev-guest/sev-guest.c @@ -38,12 +38,6 @@ struct snp_guest_dev { struct miscdevice misc; struct snp_msg_desc *msg_desc; - - union { - struct snp_report_req report; - struct snp_derived_key_req derived_key; - struct snp_ext_report_req ext_report; - } req; }; /* @@ -71,7 +65,7 @@ struct snp_req_resp { static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg) { - struct snp_report_req *report_req = &snp_dev->req.report; + struct snp_report_req *report_req __free(kfree) = NULL; struct snp_msg_desc *mdesc = snp_dev->msg_desc; struct snp_report_resp *report_resp; struct snp_guest_req req = {}; @@ -80,6 +74,10 @@ static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_io if (!arg->req_data || !arg->resp_data) return -EINVAL; + report_req = kzalloc(sizeof(*report_req), GFP_KERNEL_ACCOUNT); + if (!report_req) + return -ENOMEM; + if (copy_from_user(report_req, (void __user *)arg->req_data, sizeof(*report_req))) return -EFAULT; @@ -116,7 +114,7 @@ e_free: static int get_derived_key(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg) { - struct snp_derived_key_req *derived_key_req = &snp_dev->req.derived_key; + struct snp_derived_key_req *derived_key_req __free(kfree) = NULL; struct snp_derived_key_resp derived_key_resp = {0}; struct snp_msg_desc *mdesc = snp_dev->msg_desc; struct snp_guest_req req = {}; @@ -136,6 +134,10 @@ static int get_derived_key(struct snp_guest_dev *snp_dev, struct snp_guest_reque if (sizeof(buf) < resp_len) return -ENOMEM; + derived_key_req = kzalloc(sizeof(*derived_key_req), GFP_KERNEL_ACCOUNT); + if (!derived_key_req) + return -ENOMEM; + if (copy_from_user(derived_key_req, (void __user *)arg->req_data, sizeof(*derived_key_req))) return -EFAULT; @@ -168,7 +170,7 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques struct snp_req_resp *io) { - struct snp_ext_report_req *report_req = &snp_dev->req.ext_report; + struct snp_ext_report_req *report_req __free(kfree) = NULL; struct snp_msg_desc *mdesc = snp_dev->msg_desc; struct snp_report_resp *report_resp; struct snp_guest_req req = {}; @@ -178,6 +180,10 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques if (sockptr_is_null(io->req_data) || sockptr_is_null(io->resp_data)) return -EINVAL; + report_req = kzalloc(sizeof(*report_req), GFP_KERNEL_ACCOUNT); + if (!report_req) + return -ENOMEM; + if (copy_from_sockptr(report_req, io->req_data, sizeof(*report_req))) return -EFAULT; ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/2] virt: sev-guest: Allocate request data dynamically 2025-03-07 1:36 ` [PATCH 1/2] virt: sev-guest: Allocate request data dynamically Alexey Kardashevskiy 2025-03-07 10:52 ` Alexey Kardashevskiy 2025-03-07 18:25 ` [tip: x86/urgent] " tip-bot2 for Nikunj A Dadhania @ 2025-03-10 2:14 ` Sasha Levin 2 siblings, 0 replies; 8+ messages in thread From: Sasha Levin @ 2025-03-10 2:14 UTC (permalink / raw) To: stable, aik; +Cc: Sasha Levin [ Sasha's backport helper bot ] Hi, Summary of potential issues: ❌ Build failures detected ⚠️ Found matching upstream commit but patch is missing proper reference to it Found matching upstream commit: ac7c06acaa3738b38e83815ac0f07140ad320f13 WARNING: Author mismatch between patch and found commit: Backport author: Alexey Kardashevskiy<aik@amd.com> Commit author: Nikunj A Dadhania<nikunj@amd.com> Status in newer kernel trees: 6.6.y | Not found Note: The patch differs from the upstream commit: --- 1: ac7c06acaa373 ! 1: 9460661fc99bf virt: sev-guest: Allocate request data dynamically @@ Metadata ## Commit message ## virt: sev-guest: Allocate request data dynamically - Commit - - ae596615d93d ("virt: sev-guest: Reduce the scope of SNP command mutex") - - narrowed the command mutex scope to snp_send_guest_request(). However, - GET_REPORT, GET_DERIVED_KEY, and GET_EXT_REPORT share the req structure in - snp_guest_dev. Without the mutex protection, concurrent requests can overwrite - each other's data. Fix it by dynamically allocating the request structure. + Commit ae596615d93d ("virt: sev-guest: Reduce the scope of SNP command + mutex") narrowed the command mutex scope to snp_send_guest_request. + However, GET_REPORT, GET_DERIVED_KEY, and GET_EXT_REPORT share the req + structure in snp_guest_dev. Without the mutex protection, concurrent + requests can overwrite each other's data. Fix it by dynamically allocating + the request structure. Fixes: ae596615d93d ("virt: sev-guest: Reduce the scope of SNP command mutex") - Closes: https://github.com/AMDESE/AMDSEV/issues/265 + Cc: stable@vger.kernel.org Reported-by: andreas.stuehrk@yaxi.tech + Closes: https://github.com/AMDESE/AMDSEV/issues/265 Signed-off-by: Nikunj A Dadhania <nikunj@amd.com> - Signed-off-by: Alexey Kardashevskiy <aik@amd.com> - Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de> - Cc: stable@vger.kernel.org - Link: https://lore.kernel.org/r/20250307013700.437505-2-aik@amd.com ## drivers/virt/coco/sev-guest/sev-guest.c ## @@ drivers/virt/coco/sev-guest/sev-guest.c: struct snp_guest_dev { --- Results of testing on various branches: | Branch | Patch Apply | Build Test | |---------------------------|-------------|------------| | stable/linux-6.13.y | Success | Success | | stable/linux-6.12.y | Failed | N/A | | stable/linux-6.6.y | Failed | N/A | | stable/linux-6.1.y | Failed | N/A | | stable/linux-5.15.y | Failed | N/A | | stable/linux-5.10.y | Failed | N/A | | stable/linux-5.4.y | Failed | N/A | Build Errors: Patch failed to apply on stable/linux-6.12.y. Reject: diff a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c (rejected hunks) @@ -39,12 +39,6 @@ struct snp_guest_dev { struct miscdevice misc; struct snp_msg_desc *msg_desc; - - union { - struct snp_report_req report; - struct snp_derived_key_req derived_key; - struct snp_ext_report_req ext_report; - } req; }; /* @@ -72,7 +66,7 @@ struct snp_req_resp { static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg) { - struct snp_report_req *report_req = &snp_dev->req.report; + struct snp_report_req *report_req __free(kfree) = NULL; struct snp_msg_desc *mdesc = snp_dev->msg_desc; struct snp_report_resp *report_resp; struct snp_guest_req req = {}; @@ -117,7 +115,7 @@ static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_io static int get_derived_key(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg) { - struct snp_derived_key_req *derived_key_req = &snp_dev->req.derived_key; + struct snp_derived_key_req *derived_key_req __free(kfree) = NULL; struct snp_derived_key_resp derived_key_resp = {0}; struct snp_msg_desc *mdesc = snp_dev->msg_desc; struct snp_guest_req req = {}; @@ -169,7 +171,7 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques struct snp_req_resp *io) { - struct snp_ext_report_req *report_req = &snp_dev->req.ext_report; + struct snp_ext_report_req *report_req __free(kfree) = NULL; struct snp_msg_desc *mdesc = snp_dev->msg_desc; struct snp_report_resp *report_resp; struct snp_guest_req req = {}; Patch failed to apply on stable/linux-6.6.y. Reject: diff a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c (rejected hunks) @@ -39,12 +39,6 @@ struct snp_guest_dev { struct miscdevice misc; struct snp_msg_desc *msg_desc; - - union { - struct snp_report_req report; - struct snp_derived_key_req derived_key; - struct snp_ext_report_req ext_report; - } req; }; /* @@ -72,7 +66,7 @@ struct snp_req_resp { static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg) { - struct snp_report_req *report_req = &snp_dev->req.report; + struct snp_report_req *report_req __free(kfree) = NULL; struct snp_msg_desc *mdesc = snp_dev->msg_desc; struct snp_report_resp *report_resp; struct snp_guest_req req = {}; @@ -81,6 +75,10 @@ static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_io if (!arg->req_data || !arg->resp_data) return -EINVAL; + report_req = kzalloc(sizeof(*report_req), GFP_KERNEL_ACCOUNT); + if (!report_req) + return -ENOMEM; + if (copy_from_user(report_req, (void __user *)arg->req_data, sizeof(*report_req))) return -EFAULT; @@ -117,7 +115,7 @@ static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_io static int get_derived_key(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg) { - struct snp_derived_key_req *derived_key_req = &snp_dev->req.derived_key; + struct snp_derived_key_req *derived_key_req __free(kfree) = NULL; struct snp_derived_key_resp derived_key_resp = {0}; struct snp_msg_desc *mdesc = snp_dev->msg_desc; struct snp_guest_req req = {}; @@ -137,6 +135,10 @@ static int get_derived_key(struct snp_guest_dev *snp_dev, struct snp_guest_reque if (sizeof(buf) < resp_len) return -ENOMEM; + derived_key_req = kzalloc(sizeof(*derived_key_req), GFP_KERNEL_ACCOUNT); + if (!derived_key_req) + return -ENOMEM; + if (copy_from_user(derived_key_req, (void __user *)arg->req_data, sizeof(*derived_key_req))) return -EFAULT; @@ -169,7 +171,7 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques struct snp_req_resp *io) { - struct snp_ext_report_req *report_req = &snp_dev->req.ext_report; + struct snp_ext_report_req *report_req __free(kfree) = NULL; struct snp_msg_desc *mdesc = snp_dev->msg_desc; struct snp_report_resp *report_resp; struct snp_guest_req req = {}; @@ -179,6 +181,10 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques if (sockptr_is_null(io->req_data) || sockptr_is_null(io->resp_data)) return -EINVAL; + report_req = kzalloc(sizeof(*report_req), GFP_KERNEL_ACCOUNT); + if (!report_req) + return -ENOMEM; + if (copy_from_sockptr(report_req, io->req_data, sizeof(*report_req))) return -EFAULT; Patch failed to apply on stable/linux-6.1.y. Reject: diff a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c (rejected hunks) @@ -39,12 +39,6 @@ struct snp_guest_dev { struct miscdevice misc; struct snp_msg_desc *msg_desc; - - union { - struct snp_report_req report; - struct snp_derived_key_req derived_key; - struct snp_ext_report_req ext_report; - } req; }; /* @@ -72,7 +66,7 @@ struct snp_req_resp { static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg) { - struct snp_report_req *report_req = &snp_dev->req.report; + struct snp_report_req *report_req __free(kfree) = NULL; struct snp_msg_desc *mdesc = snp_dev->msg_desc; struct snp_report_resp *report_resp; struct snp_guest_req req = {}; @@ -81,6 +75,10 @@ static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_io if (!arg->req_data || !arg->resp_data) return -EINVAL; + report_req = kzalloc(sizeof(*report_req), GFP_KERNEL_ACCOUNT); + if (!report_req) + return -ENOMEM; + if (copy_from_user(report_req, (void __user *)arg->req_data, sizeof(*report_req))) return -EFAULT; @@ -117,7 +115,7 @@ static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_io static int get_derived_key(struct snp_guest_dev *snp_dev, struct snp_guest_request_ioctl *arg) { - struct snp_derived_key_req *derived_key_req = &snp_dev->req.derived_key; + struct snp_derived_key_req *derived_key_req __free(kfree) = NULL; struct snp_derived_key_resp derived_key_resp = {0}; struct snp_msg_desc *mdesc = snp_dev->msg_desc; struct snp_guest_req req = {}; @@ -137,6 +135,10 @@ static int get_derived_key(struct snp_guest_dev *snp_dev, struct snp_guest_reque if (sizeof(buf) < resp_len) return -ENOMEM; + derived_key_req = kzalloc(sizeof(*derived_key_req), GFP_KERNEL_ACCOUNT); + if (!derived_key_req) + return -ENOMEM; + if (copy_from_user(derived_key_req, (void __user *)arg->req_data, sizeof(*derived_key_req))) return -EFAULT; @@ -169,7 +171,7 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques struct snp_req_resp *io) { - struct snp_ext_report_req *report_req = &snp_dev->req.ext_report; + struct snp_ext_report_req *report_req __free(kfree) = NULL; struct snp_msg_desc *mdesc = snp_dev->msg_desc; struct snp_report_resp *report_resp; struct snp_guest_req req = {}; @@ -179,6 +181,10 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques if (sockptr_is_null(io->req_data) || sockptr_is_null(io->resp_data)) return -EINVAL; + report_req = kzalloc(sizeof(*report_req), GFP_KERNEL_ACCOUNT); + if (!report_req) + return -ENOMEM; + if (copy_from_sockptr(report_req, io->req_data, sizeof(*report_req))) return -EFAULT; Patch failed to apply on stable/linux-5.15.y but no reject information available. Patch failed to apply on stable/linux-5.10.y but no reject information available. Patch failed to apply on stable/linux-5.4.y but no reject information available. ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 2/2] virt: sev-guest: Move SNP Guest Request data pages handling under snp_cmd_mutex [not found] <20250307013700.437505-1-aik@amd.com> 2025-03-07 1:36 ` [PATCH 1/2] virt: sev-guest: Allocate request data dynamically Alexey Kardashevskiy @ 2025-03-07 1:37 ` Alexey Kardashevskiy 2025-03-07 9:00 ` Nikunj A. Dadhania ` (2 more replies) 1 sibling, 3 replies; 8+ messages in thread From: Alexey Kardashevskiy @ 2025-03-07 1:37 UTC (permalink / raw) To: x86 Cc: linux-kernel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, H. Peter Anvin, Tom Lendacky, Nikunj A Dadhania, Ard Biesheuvel, Pavan Kumar Paluri, Ashish Kalra, Paolo Bonzini, Michael Roth, Kevin Loughlin, Kuppuswamy Sathyanarayanan, Brijesh Singh, Liam Merwick, Alexey Kardashevskiy, stable Compared to the SNP Guest Request, the "Extended" version adds data pages for receiving certificates. If not enough pages provided, the HV can report to the VM how much is needed so the VM can reallocate and repeat. Commit ae596615d93d ("virt: sev-guest: Reduce the scope of SNP command mutex") moved handling of the allocated/desired pages number out of scope of said mutex and create a possibility for a race (multiple instances trying to trigger Extended request in a VM) as there is just one instance of snp_msg_desc per /dev/sev-guest and no locking other than snp_cmd_mutex. Fix the issue by moving the data blob/size and the GHCB input struct (snp_req_data) into snp_guest_req which is allocated on stack now and accessed by the GHCB caller under that mutex. Stop allocating SEV_FW_BLOB_MAX_SIZE in snp_msg_alloc() as only one of four callers needs it. Free the received blob in get_ext_report() right after it is copied to the userspace. Possible future users of snp_send_guest_request() are likely to have different ideas about the buffer size anyways. Fixes: ae596615d93d ("virt: sev-guest: Reduce the scope of SNP command mutex") Cc: stable@vger.kernel.org Cc: Nikunj A Dadhania <nikunj@amd.com> Signed-off-by: Alexey Kardashevskiy <aik@amd.com> --- arch/x86/include/asm/sev.h | 6 ++-- arch/x86/coco/sev/core.c | 23 +++++-------- drivers/virt/coco/sev-guest/sev-guest.c | 34 ++++++++++++++++---- 3 files changed, 39 insertions(+), 24 deletions(-) diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h index 1581246491b5..ba7999f66abe 100644 --- a/arch/x86/include/asm/sev.h +++ b/arch/x86/include/asm/sev.h @@ -203,6 +203,9 @@ struct snp_guest_req { unsigned int vmpck_id; u8 msg_version; u8 msg_type; + + struct snp_req_data input; + void *certs_data; }; /* @@ -263,9 +266,6 @@ struct snp_msg_desc { struct snp_guest_msg secret_request, secret_response; struct snp_secrets_page *secrets; - struct snp_req_data input; - - void *certs_data; struct aesgcm_ctx *ctx; diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c index 82492efc5d94..d02eea5e3d50 100644 --- a/arch/x86/coco/sev/core.c +++ b/arch/x86/coco/sev/core.c @@ -2853,19 +2853,8 @@ struct snp_msg_desc *snp_msg_alloc(void) if (!mdesc->response) goto e_free_request; - mdesc->certs_data = alloc_shared_pages(SEV_FW_BLOB_MAX_SIZE); - if (!mdesc->certs_data) - goto e_free_response; - - /* initial the input address for guest request */ - mdesc->input.req_gpa = __pa(mdesc->request); - mdesc->input.resp_gpa = __pa(mdesc->response); - mdesc->input.data_gpa = __pa(mdesc->certs_data); - return mdesc; -e_free_response: - free_shared_pages(mdesc->response, sizeof(struct snp_guest_msg)); e_free_request: free_shared_pages(mdesc->request, sizeof(struct snp_guest_msg)); e_unmap: @@ -2885,7 +2874,6 @@ void snp_msg_free(struct snp_msg_desc *mdesc) kfree(mdesc->ctx); free_shared_pages(mdesc->response, sizeof(struct snp_guest_msg)); free_shared_pages(mdesc->request, sizeof(struct snp_guest_msg)); - free_shared_pages(mdesc->certs_data, SEV_FW_BLOB_MAX_SIZE); iounmap((__force void __iomem *)mdesc->secrets); memset(mdesc, 0, sizeof(*mdesc)); @@ -3054,7 +3042,7 @@ static int __handle_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_r * sequence number must be incremented or the VMPCK must be deleted to * prevent reuse of the IV. */ - rc = snp_issue_guest_request(req, &mdesc->input, rio); + rc = snp_issue_guest_request(req, &req->input, rio); switch (rc) { case -ENOSPC: /* @@ -3064,7 +3052,7 @@ static int __handle_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_r * order to increment the sequence number and thus avoid * IV reuse. */ - override_npages = mdesc->input.data_npages; + override_npages = req->input.data_npages; req->exit_code = SVM_VMGEXIT_GUEST_REQUEST; /* @@ -3120,7 +3108,7 @@ static int __handle_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_r } if (override_npages) - mdesc->input.data_npages = override_npages; + req->input.data_npages = override_npages; return rc; } @@ -3158,6 +3146,11 @@ int snp_send_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_req *req */ memcpy(mdesc->request, &mdesc->secret_request, sizeof(mdesc->secret_request)); + /* initial the input address for guest request */ + req->input.req_gpa = __pa(mdesc->request); + req->input.resp_gpa = __pa(mdesc->response); + req->input.data_gpa = req->certs_data ? __pa(req->certs_data) : 0; + rc = __handle_guest_request(mdesc, req, rio); if (rc) { if (rc == -EIO && diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c index 4699fdc9ed44..cf3fb61f4d5b 100644 --- a/drivers/virt/coco/sev-guest/sev-guest.c +++ b/drivers/virt/coco/sev-guest/sev-guest.c @@ -177,6 +177,7 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques struct snp_guest_req req = {}; int ret, npages = 0, resp_len; sockptr_t certs_address; + struct page *page; if (sockptr_is_null(io->req_data) || sockptr_is_null(io->resp_data)) return -EINVAL; @@ -210,8 +211,20 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques * the host. If host does not supply any certs in it, then copy * zeros to indicate that certificate data was not provided. */ - memset(mdesc->certs_data, 0, report_req->certs_len); npages = report_req->certs_len >> PAGE_SHIFT; + page = alloc_pages(GFP_KERNEL_ACCOUNT | __GFP_ZERO, + get_order(report_req->certs_len)); + if (!page) + return -ENOMEM; + + req.certs_data = page_address(page); + ret = set_memory_decrypted((unsigned long)req.certs_data, npages); + if (ret) { + pr_err("failed to mark page shared, ret=%d\n", ret); + __free_pages(page, get_order(report_req->certs_len)); + return -EFAULT; + } + cmd: /* * The intermediate response buffer is used while decrypting the @@ -220,10 +233,12 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques */ resp_len = sizeof(report_resp->data) + mdesc->ctx->authsize; report_resp = kzalloc(resp_len, GFP_KERNEL_ACCOUNT); - if (!report_resp) - return -ENOMEM; + if (!report_resp) { + ret = -ENOMEM; + goto e_free_data; + } - mdesc->input.data_npages = npages; + req.input.data_npages = npages; req.msg_version = arg->msg_version; req.msg_type = SNP_MSG_REPORT_REQ; @@ -238,7 +253,7 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques /* If certs length is invalid then copy the returned length */ if (arg->vmm_error == SNP_GUEST_VMM_ERR_INVALID_LEN) { - report_req->certs_len = mdesc->input.data_npages << PAGE_SHIFT; + report_req->certs_len = req.input.data_npages << PAGE_SHIFT; if (copy_to_sockptr(io->req_data, report_req, sizeof(*report_req))) ret = -EFAULT; @@ -247,7 +262,7 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques if (ret) goto e_free; - if (npages && copy_to_sockptr(certs_address, mdesc->certs_data, report_req->certs_len)) { + if (npages && copy_to_sockptr(certs_address, req.certs_data, report_req->certs_len)) { ret = -EFAULT; goto e_free; } @@ -257,6 +272,13 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques e_free: kfree(report_resp); +e_free_data: + if (npages) { + if (set_memory_encrypted((unsigned long)req.certs_data, npages)) + WARN_ONCE(ret, "failed to restore encryption mask (leak it)\n"); + else + __free_pages(page, get_order(report_req->certs_len)); + } return ret; } -- 2.47.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] virt: sev-guest: Move SNP Guest Request data pages handling under snp_cmd_mutex 2025-03-07 1:37 ` [PATCH 2/2] virt: sev-guest: Move SNP Guest Request data pages handling under snp_cmd_mutex Alexey Kardashevskiy @ 2025-03-07 9:00 ` Nikunj A. Dadhania 2025-03-07 17:50 ` Tom Lendacky 2025-03-07 18:25 ` [tip: x86/urgent] " tip-bot2 for Alexey Kardashevskiy 2 siblings, 0 replies; 8+ messages in thread From: Nikunj A. Dadhania @ 2025-03-07 9:00 UTC (permalink / raw) To: Alexey Kardashevskiy, x86 Cc: linux-kernel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, H. Peter Anvin, Tom Lendacky, Ard Biesheuvel, Pavan Kumar Paluri, Ashish Kalra, Paolo Bonzini, Michael Roth, Kevin Loughlin, Kuppuswamy Sathyanarayanan, Brijesh Singh, Liam Merwick, stable On 3/7/2025 7:07 AM, Alexey Kardashevskiy wrote: > Compared to the SNP Guest Request, the "Extended" version adds data pages > for receiving certificates. If not enough pages provided, the HV can > report to the VM how much is needed so the VM can reallocate and repeat. > > Commit ae596615d93d ("virt: sev-guest: Reduce the scope of SNP command > mutex") moved handling of the allocated/desired pages number out of scope > of said mutex and create a possibility for a race (multiple instances > trying to trigger Extended request in a VM) as there is just one instance > of snp_msg_desc per /dev/sev-guest and no locking other than snp_cmd_mutex. > > Fix the issue by moving the data blob/size and the GHCB input struct > (snp_req_data) into snp_guest_req which is allocated on stack now > and accessed by the GHCB caller under that mutex. > > Stop allocating SEV_FW_BLOB_MAX_SIZE in snp_msg_alloc() as only one of > four callers needs it. Free the received blob in get_ext_report() right > after it is copied to the userspace. Possible future users of > snp_send_guest_request() are likely to have different ideas about > the buffer size anyways. > > Fixes: ae596615d93d ("virt: sev-guest: Reduce the scope of SNP command mutex") > Cc: stable@vger.kernel.org > Cc: Nikunj A Dadhania <nikunj@amd.com> > Signed-off-by: Alexey Kardashevskiy <aik@amd.com> With a minor nit below: Reviewed-by: Nikunj A Dadhania <nikunj@amd.com> > --- > arch/x86/include/asm/sev.h | 6 ++-- > arch/x86/coco/sev/core.c | 23 +++++-------- > drivers/virt/coco/sev-guest/sev-guest.c | 34 ++++++++++++++++---- > 3 files changed, 39 insertions(+), 24 deletions(-) > > diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h > index 1581246491b5..ba7999f66abe 100644 > --- a/arch/x86/include/asm/sev.h > +++ b/arch/x86/include/asm/sev.h > @@ -203,6 +203,9 @@ struct snp_guest_req { > unsigned int vmpck_id; > u8 msg_version; > u8 msg_type; > + > + struct snp_req_data input; > + void *certs_data; > }; > > /* > @@ -263,9 +266,6 @@ struct snp_msg_desc { > struct snp_guest_msg secret_request, secret_response; > > struct snp_secrets_page *secrets; > - struct snp_req_data input; > - > - void *certs_data; > > struct aesgcm_ctx *ctx; > > diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c > index 82492efc5d94..d02eea5e3d50 100644 > --- a/arch/x86/coco/sev/core.c > +++ b/arch/x86/coco/sev/core.c > @@ -2853,19 +2853,8 @@ struct snp_msg_desc *snp_msg_alloc(void) > if (!mdesc->response) > goto e_free_request; > > - mdesc->certs_data = alloc_shared_pages(SEV_FW_BLOB_MAX_SIZE); > - if (!mdesc->certs_data) > - goto e_free_response; > - > - /* initial the input address for guest request */ > - mdesc->input.req_gpa = __pa(mdesc->request); > - mdesc->input.resp_gpa = __pa(mdesc->response); > - mdesc->input.data_gpa = __pa(mdesc->certs_data); > - > return mdesc; > > -e_free_response: > - free_shared_pages(mdesc->response, sizeof(struct snp_guest_msg)); > e_free_request: > free_shared_pages(mdesc->request, sizeof(struct snp_guest_msg)); > e_unmap: > @@ -2885,7 +2874,6 @@ void snp_msg_free(struct snp_msg_desc *mdesc) > kfree(mdesc->ctx); > free_shared_pages(mdesc->response, sizeof(struct snp_guest_msg)); > free_shared_pages(mdesc->request, sizeof(struct snp_guest_msg)); > - free_shared_pages(mdesc->certs_data, SEV_FW_BLOB_MAX_SIZE); > iounmap((__force void __iomem *)mdesc->secrets); > > memset(mdesc, 0, sizeof(*mdesc)); > @@ -3054,7 +3042,7 @@ static int __handle_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_r > * sequence number must be incremented or the VMPCK must be deleted to > * prevent reuse of the IV. > */ > - rc = snp_issue_guest_request(req, &mdesc->input, rio); > + rc = snp_issue_guest_request(req, &req->input, rio); > switch (rc) { > case -ENOSPC: > /* > @@ -3064,7 +3052,7 @@ static int __handle_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_r > * order to increment the sequence number and thus avoid > * IV reuse. > */ > - override_npages = mdesc->input.data_npages; > + override_npages = req->input.data_npages; > req->exit_code = SVM_VMGEXIT_GUEST_REQUEST; > > /* > @@ -3120,7 +3108,7 @@ static int __handle_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_r > } > > if (override_npages) > - mdesc->input.data_npages = override_npages; > + req->input.data_npages = override_npages; > > return rc; > } > @@ -3158,6 +3146,11 @@ int snp_send_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_req *req > */ > memcpy(mdesc->request, &mdesc->secret_request, sizeof(mdesc->secret_request)); > > + /* initial the input address for guest request */ s/initial/Initialize/ Regards Nikunj > + req->input.req_gpa = __pa(mdesc->request); > + req->input.resp_gpa = __pa(mdesc->response); > + req->input.data_gpa = req->certs_data ? __pa(req->certs_data) : 0; > + > rc = __handle_guest_request(mdesc, req, rio); > if (rc) { > if (rc == -EIO && > diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c > index 4699fdc9ed44..cf3fb61f4d5b 100644 > --- a/drivers/virt/coco/sev-guest/sev-guest.c > +++ b/drivers/virt/coco/sev-guest/sev-guest.c > @@ -177,6 +177,7 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques > struct snp_guest_req req = {}; > int ret, npages = 0, resp_len; > sockptr_t certs_address; > + struct page *page; > > if (sockptr_is_null(io->req_data) || sockptr_is_null(io->resp_data)) > return -EINVAL; > @@ -210,8 +211,20 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques > * the host. If host does not supply any certs in it, then copy > * zeros to indicate that certificate data was not provided. > */ > - memset(mdesc->certs_data, 0, report_req->certs_len); > npages = report_req->certs_len >> PAGE_SHIFT; > + page = alloc_pages(GFP_KERNEL_ACCOUNT | __GFP_ZERO, > + get_order(report_req->certs_len)); > + if (!page) > + return -ENOMEM; > + > + req.certs_data = page_address(page); > + ret = set_memory_decrypted((unsigned long)req.certs_data, npages); > + if (ret) { > + pr_err("failed to mark page shared, ret=%d\n", ret); > + __free_pages(page, get_order(report_req->certs_len)); > + return -EFAULT; > + } > + > cmd: > /* > * The intermediate response buffer is used while decrypting the > @@ -220,10 +233,12 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques > */ > resp_len = sizeof(report_resp->data) + mdesc->ctx->authsize; > report_resp = kzalloc(resp_len, GFP_KERNEL_ACCOUNT); > - if (!report_resp) > - return -ENOMEM; > + if (!report_resp) { > + ret = -ENOMEM; > + goto e_free_data; > + } > > - mdesc->input.data_npages = npages; > + req.input.data_npages = npages; > > req.msg_version = arg->msg_version; > req.msg_type = SNP_MSG_REPORT_REQ; > @@ -238,7 +253,7 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques > > /* If certs length is invalid then copy the returned length */ > if (arg->vmm_error == SNP_GUEST_VMM_ERR_INVALID_LEN) { > - report_req->certs_len = mdesc->input.data_npages << PAGE_SHIFT; > + report_req->certs_len = req.input.data_npages << PAGE_SHIFT; > > if (copy_to_sockptr(io->req_data, report_req, sizeof(*report_req))) > ret = -EFAULT; > @@ -247,7 +262,7 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques > if (ret) > goto e_free; > > - if (npages && copy_to_sockptr(certs_address, mdesc->certs_data, report_req->certs_len)) { > + if (npages && copy_to_sockptr(certs_address, req.certs_data, report_req->certs_len)) { > ret = -EFAULT; > goto e_free; > } > @@ -257,6 +272,13 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques > > e_free: > kfree(report_resp); > +e_free_data: > + if (npages) { > + if (set_memory_encrypted((unsigned long)req.certs_data, npages)) > + WARN_ONCE(ret, "failed to restore encryption mask (leak it)\n"); > + else > + __free_pages(page, get_order(report_req->certs_len)); > + } > return ret; > } > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 2/2] virt: sev-guest: Move SNP Guest Request data pages handling under snp_cmd_mutex 2025-03-07 1:37 ` [PATCH 2/2] virt: sev-guest: Move SNP Guest Request data pages handling under snp_cmd_mutex Alexey Kardashevskiy 2025-03-07 9:00 ` Nikunj A. Dadhania @ 2025-03-07 17:50 ` Tom Lendacky 2025-03-07 18:25 ` [tip: x86/urgent] " tip-bot2 for Alexey Kardashevskiy 2 siblings, 0 replies; 8+ messages in thread From: Tom Lendacky @ 2025-03-07 17:50 UTC (permalink / raw) To: Alexey Kardashevskiy, x86 Cc: linux-kernel, Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, H. Peter Anvin, Nikunj A Dadhania, Ard Biesheuvel, Pavan Kumar Paluri, Ashish Kalra, Paolo Bonzini, Michael Roth, Kevin Loughlin, Kuppuswamy Sathyanarayanan, Brijesh Singh, Liam Merwick, stable On 3/6/25 19:37, Alexey Kardashevskiy wrote: > Compared to the SNP Guest Request, the "Extended" version adds data pages > for receiving certificates. If not enough pages provided, the HV can > report to the VM how much is needed so the VM can reallocate and repeat. > > Commit ae596615d93d ("virt: sev-guest: Reduce the scope of SNP command > mutex") moved handling of the allocated/desired pages number out of scope > of said mutex and create a possibility for a race (multiple instances > trying to trigger Extended request in a VM) as there is just one instance > of snp_msg_desc per /dev/sev-guest and no locking other than snp_cmd_mutex. > > Fix the issue by moving the data blob/size and the GHCB input struct > (snp_req_data) into snp_guest_req which is allocated on stack now > and accessed by the GHCB caller under that mutex. > > Stop allocating SEV_FW_BLOB_MAX_SIZE in snp_msg_alloc() as only one of > four callers needs it. Free the received blob in get_ext_report() right > after it is copied to the userspace. Possible future users of > snp_send_guest_request() are likely to have different ideas about > the buffer size anyways. > > Fixes: ae596615d93d ("virt: sev-guest: Reduce the scope of SNP command mutex") > Cc: stable@vger.kernel.org > Cc: Nikunj A Dadhania <nikunj@amd.com> > Signed-off-by: Alexey Kardashevskiy <aik@amd.com> > --- > arch/x86/include/asm/sev.h | 6 ++-- > arch/x86/coco/sev/core.c | 23 +++++-------- > drivers/virt/coco/sev-guest/sev-guest.c | 34 ++++++++++++++++---- > 3 files changed, 39 insertions(+), 24 deletions(-) > > diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h > index 1581246491b5..ba7999f66abe 100644 > --- a/arch/x86/include/asm/sev.h > +++ b/arch/x86/include/asm/sev.h > @@ -203,6 +203,9 @@ struct snp_guest_req { > unsigned int vmpck_id; > u8 msg_version; > u8 msg_type; > + > + struct snp_req_data input; > + void *certs_data; > }; > > /* > @@ -263,9 +266,6 @@ struct snp_msg_desc { > struct snp_guest_msg secret_request, secret_response; > > struct snp_secrets_page *secrets; > - struct snp_req_data input; > - > - void *certs_data; > > struct aesgcm_ctx *ctx; > > diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c > index 82492efc5d94..d02eea5e3d50 100644 > --- a/arch/x86/coco/sev/core.c > +++ b/arch/x86/coco/sev/core.c > @@ -2853,19 +2853,8 @@ struct snp_msg_desc *snp_msg_alloc(void) > if (!mdesc->response) > goto e_free_request; > > - mdesc->certs_data = alloc_shared_pages(SEV_FW_BLOB_MAX_SIZE); > - if (!mdesc->certs_data) > - goto e_free_response; > - > - /* initial the input address for guest request */ > - mdesc->input.req_gpa = __pa(mdesc->request); > - mdesc->input.resp_gpa = __pa(mdesc->response); > - mdesc->input.data_gpa = __pa(mdesc->certs_data); > - > return mdesc; > > -e_free_response: > - free_shared_pages(mdesc->response, sizeof(struct snp_guest_msg)); > e_free_request: > free_shared_pages(mdesc->request, sizeof(struct snp_guest_msg)); > e_unmap: > @@ -2885,7 +2874,6 @@ void snp_msg_free(struct snp_msg_desc *mdesc) > kfree(mdesc->ctx); > free_shared_pages(mdesc->response, sizeof(struct snp_guest_msg)); > free_shared_pages(mdesc->request, sizeof(struct snp_guest_msg)); > - free_shared_pages(mdesc->certs_data, SEV_FW_BLOB_MAX_SIZE); > iounmap((__force void __iomem *)mdesc->secrets); > > memset(mdesc, 0, sizeof(*mdesc)); > @@ -3054,7 +3042,7 @@ static int __handle_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_r > * sequence number must be incremented or the VMPCK must be deleted to > * prevent reuse of the IV. > */ > - rc = snp_issue_guest_request(req, &mdesc->input, rio); > + rc = snp_issue_guest_request(req, &req->input, rio); > switch (rc) { > case -ENOSPC: > /* > @@ -3064,7 +3052,7 @@ static int __handle_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_r > * order to increment the sequence number and thus avoid > * IV reuse. > */ > - override_npages = mdesc->input.data_npages; > + override_npages = req->input.data_npages; > req->exit_code = SVM_VMGEXIT_GUEST_REQUEST; > > /* > @@ -3120,7 +3108,7 @@ static int __handle_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_r > } > > if (override_npages) > - mdesc->input.data_npages = override_npages; > + req->input.data_npages = override_npages; > > return rc; > } > @@ -3158,6 +3146,11 @@ int snp_send_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_req *req > */ > memcpy(mdesc->request, &mdesc->secret_request, sizeof(mdesc->secret_request)); > > + /* initial the input address for guest request */ > + req->input.req_gpa = __pa(mdesc->request); > + req->input.resp_gpa = __pa(mdesc->response); > + req->input.data_gpa = req->certs_data ? __pa(req->certs_data) : 0; > + > rc = __handle_guest_request(mdesc, req, rio); > if (rc) { > if (rc == -EIO && > diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c > index 4699fdc9ed44..cf3fb61f4d5b 100644 > --- a/drivers/virt/coco/sev-guest/sev-guest.c > +++ b/drivers/virt/coco/sev-guest/sev-guest.c > @@ -177,6 +177,7 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques > struct snp_guest_req req = {}; > int ret, npages = 0, resp_len; > sockptr_t certs_address; > + struct page *page; > > if (sockptr_is_null(io->req_data) || sockptr_is_null(io->resp_data)) > return -EINVAL; > @@ -210,8 +211,20 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques > * the host. If host does not supply any certs in it, then copy > * zeros to indicate that certificate data was not provided. > */ > - memset(mdesc->certs_data, 0, report_req->certs_len); > npages = report_req->certs_len >> PAGE_SHIFT; > + page = alloc_pages(GFP_KERNEL_ACCOUNT | __GFP_ZERO, > + get_order(report_req->certs_len)); Not sure if it is worth using alloc_pages_exact() (and free_pages_exact()) here instead, since you only end up performing set_memory_decrypted() against npages vs the actual number allocated. It's not an issue, just looks a bit odd to my eye. > + if (!page) > + return -ENOMEM; > + > + req.certs_data = page_address(page); > + ret = set_memory_decrypted((unsigned long)req.certs_data, npages); > + if (ret) { > + pr_err("failed to mark page shared, ret=%d\n", ret); > + __free_pages(page, get_order(report_req->certs_len)); You can't be sure at what stage the failure occurred, so you need to leak the pages, just like below where you call set_memory_encrypted(). And similar to below, maybe do a WARN_ONCE() instead of pr_err()? > + return -EFAULT; > + } > + > cmd: > /* > * The intermediate response buffer is used while decrypting the > @@ -220,10 +233,12 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques > */ > resp_len = sizeof(report_resp->data) + mdesc->ctx->authsize; > report_resp = kzalloc(resp_len, GFP_KERNEL_ACCOUNT); > - if (!report_resp) > - return -ENOMEM; > + if (!report_resp) { > + ret = -ENOMEM; > + goto e_free_data; > + } > > - mdesc->input.data_npages = npages; > + req.input.data_npages = npages; > > req.msg_version = arg->msg_version; > req.msg_type = SNP_MSG_REPORT_REQ; > @@ -238,7 +253,7 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques > > /* If certs length is invalid then copy the returned length */ > if (arg->vmm_error == SNP_GUEST_VMM_ERR_INVALID_LEN) { > - report_req->certs_len = mdesc->input.data_npages << PAGE_SHIFT; > + report_req->certs_len = req.input.data_npages << PAGE_SHIFT; > > if (copy_to_sockptr(io->req_data, report_req, sizeof(*report_req))) > ret = -EFAULT; > @@ -247,7 +262,7 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques > if (ret) > goto e_free; > > - if (npages && copy_to_sockptr(certs_address, mdesc->certs_data, report_req->certs_len)) { > + if (npages && copy_to_sockptr(certs_address, req.certs_data, report_req->certs_len)) { > ret = -EFAULT; > goto e_free; > } > @@ -257,6 +272,13 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques > > e_free: > kfree(report_resp); > +e_free_data: > + if (npages) { > + if (set_memory_encrypted((unsigned long)req.certs_data, npages)) > + WARN_ONCE(ret, "failed to restore encryption mask (leak it)\n"); > + else > + __free_pages(page, get_order(report_req->certs_len)); Can't report_req->certs_len have been updated with a new value at this point (from the "if (arg->vmm_error == SNP_GUEST_VMM_ERR_INVALID_LEN)") check and you'll attempt to possibly free more than you allocated? This would be covered if you stick with npages and use alloc_pages_exact() and free_pages_exact() using npages. Thanks, Tom > + } > return ret; > } > ^ permalink raw reply [flat|nested] 8+ messages in thread
* [tip: x86/urgent] virt: sev-guest: Move SNP Guest Request data pages handling under snp_cmd_mutex 2025-03-07 1:37 ` [PATCH 2/2] virt: sev-guest: Move SNP Guest Request data pages handling under snp_cmd_mutex Alexey Kardashevskiy 2025-03-07 9:00 ` Nikunj A. Dadhania 2025-03-07 17:50 ` Tom Lendacky @ 2025-03-07 18:25 ` tip-bot2 for Alexey Kardashevskiy 2 siblings, 0 replies; 8+ messages in thread From: tip-bot2 for Alexey Kardashevskiy @ 2025-03-07 18:25 UTC (permalink / raw) To: linux-tip-commits Cc: Alexey Kardashevskiy, Borislav Petkov (AMD), Nikunj A Dadhania, stable, x86, linux-kernel The following commit has been merged into the x86/urgent branch of tip: Commit-ID: 3e385c0d6ce88ac9916dcf84267bd5855d830748 Gitweb: https://git.kernel.org/tip/3e385c0d6ce88ac9916dcf84267bd5855d830748 Author: Alexey Kardashevskiy <aik@amd.com> AuthorDate: Fri, 07 Mar 2025 12:37:00 +11:00 Committer: Borislav Petkov (AMD) <bp@alien8.de> CommitterDate: Fri, 07 Mar 2025 14:09:33 +01:00 virt: sev-guest: Move SNP Guest Request data pages handling under snp_cmd_mutex Compared to the SNP Guest Request, the "Extended" version adds data pages for receiving certificates. If not enough pages provided, the HV can report to the VM how much is needed so the VM can reallocate and repeat. Commit ae596615d93d ("virt: sev-guest: Reduce the scope of SNP command mutex") moved handling of the allocated/desired pages number out of scope of said mutex and create a possibility for a race (multiple instances trying to trigger Extended request in a VM) as there is just one instance of snp_msg_desc per /dev/sev-guest and no locking other than snp_cmd_mutex. Fix the issue by moving the data blob/size and the GHCB input struct (snp_req_data) into snp_guest_req which is allocated on stack now and accessed by the GHCB caller under that mutex. Stop allocating SEV_FW_BLOB_MAX_SIZE in snp_msg_alloc() as only one of four callers needs it. Free the received blob in get_ext_report() right after it is copied to the userspace. Possible future users of snp_send_guest_request() are likely to have different ideas about the buffer size anyways. Fixes: ae596615d93d ("virt: sev-guest: Reduce the scope of SNP command mutex") Signed-off-by: Alexey Kardashevskiy <aik@amd.com> Signed-off-by: Borislav Petkov (AMD) <bp@alien8.de> Reviewed-by: Nikunj A Dadhania <nikunj@amd.com> Cc: stable@vger.kernel.org Link: https://lore.kernel.org/r/20250307013700.437505-3-aik@amd.com --- arch/x86/coco/sev/core.c | 23 +++++----------- arch/x86/include/asm/sev.h | 6 ++-- drivers/virt/coco/sev-guest/sev-guest.c | 34 +++++++++++++++++++----- 3 files changed, 39 insertions(+), 24 deletions(-) diff --git a/arch/x86/coco/sev/core.c b/arch/x86/coco/sev/core.c index 82492ef..96c7bc6 100644 --- a/arch/x86/coco/sev/core.c +++ b/arch/x86/coco/sev/core.c @@ -2853,19 +2853,8 @@ struct snp_msg_desc *snp_msg_alloc(void) if (!mdesc->response) goto e_free_request; - mdesc->certs_data = alloc_shared_pages(SEV_FW_BLOB_MAX_SIZE); - if (!mdesc->certs_data) - goto e_free_response; - - /* initial the input address for guest request */ - mdesc->input.req_gpa = __pa(mdesc->request); - mdesc->input.resp_gpa = __pa(mdesc->response); - mdesc->input.data_gpa = __pa(mdesc->certs_data); - return mdesc; -e_free_response: - free_shared_pages(mdesc->response, sizeof(struct snp_guest_msg)); e_free_request: free_shared_pages(mdesc->request, sizeof(struct snp_guest_msg)); e_unmap: @@ -2885,7 +2874,6 @@ void snp_msg_free(struct snp_msg_desc *mdesc) kfree(mdesc->ctx); free_shared_pages(mdesc->response, sizeof(struct snp_guest_msg)); free_shared_pages(mdesc->request, sizeof(struct snp_guest_msg)); - free_shared_pages(mdesc->certs_data, SEV_FW_BLOB_MAX_SIZE); iounmap((__force void __iomem *)mdesc->secrets); memset(mdesc, 0, sizeof(*mdesc)); @@ -3054,7 +3042,7 @@ retry_request: * sequence number must be incremented or the VMPCK must be deleted to * prevent reuse of the IV. */ - rc = snp_issue_guest_request(req, &mdesc->input, rio); + rc = snp_issue_guest_request(req, &req->input, rio); switch (rc) { case -ENOSPC: /* @@ -3064,7 +3052,7 @@ retry_request: * order to increment the sequence number and thus avoid * IV reuse. */ - override_npages = mdesc->input.data_npages; + override_npages = req->input.data_npages; req->exit_code = SVM_VMGEXIT_GUEST_REQUEST; /* @@ -3120,7 +3108,7 @@ retry_request: } if (override_npages) - mdesc->input.data_npages = override_npages; + req->input.data_npages = override_npages; return rc; } @@ -3158,6 +3146,11 @@ int snp_send_guest_request(struct snp_msg_desc *mdesc, struct snp_guest_req *req */ memcpy(mdesc->request, &mdesc->secret_request, sizeof(mdesc->secret_request)); + /* Initialize the input address for guest request */ + req->input.req_gpa = __pa(mdesc->request); + req->input.resp_gpa = __pa(mdesc->response); + req->input.data_gpa = req->certs_data ? __pa(req->certs_data) : 0; + rc = __handle_guest_request(mdesc, req, rio); if (rc) { if (rc == -EIO && diff --git a/arch/x86/include/asm/sev.h b/arch/x86/include/asm/sev.h index 1581246..ba7999f 100644 --- a/arch/x86/include/asm/sev.h +++ b/arch/x86/include/asm/sev.h @@ -203,6 +203,9 @@ struct snp_guest_req { unsigned int vmpck_id; u8 msg_version; u8 msg_type; + + struct snp_req_data input; + void *certs_data; }; /* @@ -263,9 +266,6 @@ struct snp_msg_desc { struct snp_guest_msg secret_request, secret_response; struct snp_secrets_page *secrets; - struct snp_req_data input; - - void *certs_data; struct aesgcm_ctx *ctx; diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c index 23ac177..70fbc9a 100644 --- a/drivers/virt/coco/sev-guest/sev-guest.c +++ b/drivers/virt/coco/sev-guest/sev-guest.c @@ -176,6 +176,7 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques struct snp_guest_req req = {}; int ret, npages = 0, resp_len; sockptr_t certs_address; + struct page *page; if (sockptr_is_null(io->req_data) || sockptr_is_null(io->resp_data)) return -EINVAL; @@ -209,8 +210,20 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques * the host. If host does not supply any certs in it, then copy * zeros to indicate that certificate data was not provided. */ - memset(mdesc->certs_data, 0, report_req->certs_len); npages = report_req->certs_len >> PAGE_SHIFT; + page = alloc_pages(GFP_KERNEL_ACCOUNT | __GFP_ZERO, + get_order(report_req->certs_len)); + if (!page) + return -ENOMEM; + + req.certs_data = page_address(page); + ret = set_memory_decrypted((unsigned long)req.certs_data, npages); + if (ret) { + pr_err("failed to mark page shared, ret=%d\n", ret); + __free_pages(page, get_order(report_req->certs_len)); + return -EFAULT; + } + cmd: /* * The intermediate response buffer is used while decrypting the @@ -219,10 +232,12 @@ cmd: */ resp_len = sizeof(report_resp->data) + mdesc->ctx->authsize; report_resp = kzalloc(resp_len, GFP_KERNEL_ACCOUNT); - if (!report_resp) - return -ENOMEM; + if (!report_resp) { + ret = -ENOMEM; + goto e_free_data; + } - mdesc->input.data_npages = npages; + req.input.data_npages = npages; req.msg_version = arg->msg_version; req.msg_type = SNP_MSG_REPORT_REQ; @@ -237,7 +252,7 @@ cmd: /* If certs length is invalid then copy the returned length */ if (arg->vmm_error == SNP_GUEST_VMM_ERR_INVALID_LEN) { - report_req->certs_len = mdesc->input.data_npages << PAGE_SHIFT; + report_req->certs_len = req.input.data_npages << PAGE_SHIFT; if (copy_to_sockptr(io->req_data, report_req, sizeof(*report_req))) ret = -EFAULT; @@ -246,7 +261,7 @@ cmd: if (ret) goto e_free; - if (npages && copy_to_sockptr(certs_address, mdesc->certs_data, report_req->certs_len)) { + if (npages && copy_to_sockptr(certs_address, req.certs_data, report_req->certs_len)) { ret = -EFAULT; goto e_free; } @@ -256,6 +271,13 @@ cmd: e_free: kfree(report_resp); +e_free_data: + if (npages) { + if (set_memory_encrypted((unsigned long)req.certs_data, npages)) + WARN_ONCE(ret, "failed to restore encryption mask (leak it)\n"); + else + __free_pages(page, get_order(report_req->certs_len)); + } return ret; } ^ permalink raw reply related [flat|nested] 8+ messages in thread
end of thread, other threads:[~2025-03-10 2:14 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20250307013700.437505-1-aik@amd.com>
2025-03-07 1:36 ` [PATCH 1/2] virt: sev-guest: Allocate request data dynamically Alexey Kardashevskiy
2025-03-07 10:52 ` Alexey Kardashevskiy
2025-03-07 18:25 ` [tip: x86/urgent] " tip-bot2 for Nikunj A Dadhania
2025-03-10 2:14 ` [PATCH 1/2] " Sasha Levin
2025-03-07 1:37 ` [PATCH 2/2] virt: sev-guest: Move SNP Guest Request data pages handling under snp_cmd_mutex Alexey Kardashevskiy
2025-03-07 9:00 ` Nikunj A. Dadhania
2025-03-07 17:50 ` Tom Lendacky
2025-03-07 18:25 ` [tip: x86/urgent] " tip-bot2 for Alexey Kardashevskiy
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox