| Message ID | 20240731150811.156771-10-nikunj@amd.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Add Secure TSC support for SNP guests | expand |
On 7/31/24 10:08, Nikunj A Dadhania wrote: > The SNP command mutex is used to serialize access to the shared buffer, > command handling, and message sequence number. > > All shared buffer, command handling, and message sequence updates are done > within snp_send_guest_request(), so moving the mutex to this function is > appropriate and maintains the critical section. > > Since the mutex is now taken at a later point in time, remove the lockdep > checks that occur before taking the mutex. > > Signed-off-by: Nikunj A Dadhania <nikunj@amd.com> > --- > drivers/virt/coco/sev-guest/sev-guest.c | 17 ++--------------- > 1 file changed, 2 insertions(+), 15 deletions(-) > > diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c > index 92734a2345a6..42f7126f1718 100644 > --- a/drivers/virt/coco/sev-guest/sev-guest.c > +++ b/drivers/virt/coco/sev-guest/sev-guest.c > @@ -345,6 +345,8 @@ static int snp_send_guest_request(struct snp_guest_dev *snp_dev, struct snp_gues > u64 seqno; > int rc; > > + guard(mutex)(&snp_cmd_mutex); > + > /* Get message sequence and verify that its a non-zero */ > seqno = snp_get_msg_seqno(snp_dev); > if (!seqno) > @@ -419,8 +421,6 @@ static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_io > struct snp_report_resp *report_resp; > int rc, resp_len; > > - lockdep_assert_held(&snp_cmd_mutex); > - > if (!arg->req_data || !arg->resp_data) > return -EINVAL; > > @@ -458,8 +458,6 @@ static int get_derived_key(struct snp_guest_dev *snp_dev, struct snp_guest_reque > /* Response data is 64 bytes and max authsize for GCM is 16 bytes. */ > u8 buf[64 + 16]; > > - lockdep_assert_held(&snp_cmd_mutex); > - > if (!arg->req_data || !arg->resp_data) > return -EINVAL; > > @@ -501,8 +499,6 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques > int ret, npages = 0, resp_len; > sockptr_t certs_address; > > - lockdep_assert_held(&snp_cmd_mutex); > - > if (sockptr_is_null(io->req_data) || sockptr_is_null(io->resp_data)) > return -EINVAL; > > @@ -590,12 +586,9 @@ static long snp_guest_ioctl(struct file *file, unsigned int ioctl, unsigned long > if (!input.msg_version) > return -EINVAL; > > - mutex_lock(&snp_cmd_mutex); > - > /* Check if the VMPCK is not empty */ > if (is_vmpck_empty(snp_dev)) { Are we ok with this being outside of the lock now? I believe is_vmpck_empty() can get a false and then be waiting on the mutex while snp_disable_vmpck() is called. Suddenly the code thinks the VMPCK is valid when it isn't anymore. Not sure if that matters, as the guest request will fail anyway? Thanks, Tom > dev_err_ratelimited(snp_dev->dev, "VMPCK is disabled\n"); > - mutex_unlock(&snp_cmd_mutex); > return -ENOTTY; > } > > @@ -620,8 +613,6 @@ static long snp_guest_ioctl(struct file *file, unsigned int ioctl, unsigned long > break; > } > > - mutex_unlock(&snp_cmd_mutex); > - > if (input.exitinfo2 && copy_to_user(argp, &input, sizeof(input))) > return -EFAULT; > > @@ -736,8 +727,6 @@ static int sev_svsm_report_new(struct tsm_report *report, void *data) > man_len = SZ_4K; > certs_len = SEV_FW_BLOB_MAX_SIZE; > > - guard(mutex)(&snp_cmd_mutex); > - > if (guid_is_null(&desc->service_guid)) { > call_id = SVSM_ATTEST_CALL(SVSM_ATTEST_SERVICES); > } else { > @@ -872,8 +861,6 @@ static int sev_report_new(struct tsm_report *report, void *data) > if (!buf) > return -ENOMEM; > > - guard(mutex)(&snp_cmd_mutex); > - > /* Check if the VMPCK is not empty */ > if (is_vmpck_empty(snp_dev)) { > dev_err_ratelimited(snp_dev->dev, "VMPCK is disabled\n");
Hi Tom, On 9/13/2024 3:24 AM, Tom Lendacky wrote: > On 7/31/24 10:08, Nikunj A Dadhania wrote: >> @@ -590,12 +586,9 @@ static long snp_guest_ioctl(struct file *file, unsigned int ioctl, unsigned long >> if (!input.msg_version) >> return -EINVAL; >> >> - mutex_lock(&snp_cmd_mutex); >> - >> /* Check if the VMPCK is not empty */ >> if (is_vmpck_empty(snp_dev)) { > > Are we ok with this being outside of the lock now? We can move the check inside the lock, and get_* will try to prepare the message and after grabbing the lock if the the VMPCK is empty we would fail. Something like below: diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c index 8a2d0d751685..537f59358090 100644 --- a/drivers/virt/coco/sev-guest/sev-guest.c +++ b/drivers/virt/coco/sev-guest/sev-guest.c @@ -347,6 +347,12 @@ static int snp_send_guest_request(struct snp_guest_dev *snp_dev, struct snp_gues guard(mutex)(&snp_cmd_mutex); + /* Check if the VMPCK is not empty */ + if (is_vmpck_empty(snp_dev)) { + dev_err_ratelimited(snp_dev->dev, "VMPCK is disabled\n"); + return -ENOTTY; + } + /* Get message sequence and verify that its a non-zero */ seqno = snp_get_msg_seqno(snp_dev); if (!seqno) @@ -594,12 +600,6 @@ static long snp_guest_ioctl(struct file *file, unsigned int ioctl, unsigned long if (!input.msg_version) return -EINVAL; - /* Check if the VMPCK is not empty */ - if (is_vmpck_empty(snp_dev)) { - dev_err_ratelimited(snp_dev->dev, "VMPCK is disabled\n"); - return -ENOTTY; - } - switch (ioctl) { case SNP_GET_REPORT: ret = get_report(snp_dev, &input); @@ -869,12 +869,6 @@ static int sev_report_new(struct tsm_report *report, void *data) if (!buf) return -ENOMEM; - /* Check if the VMPCK is not empty */ - if (is_vmpck_empty(snp_dev)) { - dev_err_ratelimited(snp_dev->dev, "VMPCK is disabled\n"); - return -ENOTTY; - } - cert_table = buf + report_size; struct snp_ext_report_req ext_req = { .data = { .vmpl = desc->privlevel }, > I believe is_vmpck_empty() can get a false and then be waiting on the > mutex while snp_disable_vmpck() is called. Suddenly the code thinks the > VMPCK is valid when it isn't anymore. Not sure if that matters, as the > guest request will fail anyway? The above code will fail early. > > Thanks, > Tom > Regards Nikunj
On 9/12/24 23:26, Nikunj A. Dadhania wrote: > Hi Tom, > > On 9/13/2024 3:24 AM, Tom Lendacky wrote: >> On 7/31/24 10:08, Nikunj A Dadhania wrote: >>> @@ -590,12 +586,9 @@ static long snp_guest_ioctl(struct file *file, unsigned int ioctl, unsigned long >>> if (!input.msg_version) >>> return -EINVAL; >>> >>> - mutex_lock(&snp_cmd_mutex); >>> - >>> /* Check if the VMPCK is not empty */ >>> if (is_vmpck_empty(snp_dev)) { >> >> Are we ok with this being outside of the lock now? > > We can move the check inside the lock, and get_* will try to prepare > the message and after grabbing the lock if the the VMPCK is empty we > would fail. Something like below: Yep, works for me. Thanks, Tom > > diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c > index 8a2d0d751685..537f59358090 100644 > --- a/drivers/virt/coco/sev-guest/sev-guest.c > +++ b/drivers/virt/coco/sev-guest/sev-guest.c > @@ -347,6 +347,12 @@ static int snp_send_guest_request(struct snp_guest_dev *snp_dev, struct snp_gues > > guard(mutex)(&snp_cmd_mutex); > > + /* Check if the VMPCK is not empty */ > + if (is_vmpck_empty(snp_dev)) { > + dev_err_ratelimited(snp_dev->dev, "VMPCK is disabled\n"); > + return -ENOTTY; > + } > + > /* Get message sequence and verify that its a non-zero */ > seqno = snp_get_msg_seqno(snp_dev); > if (!seqno) > @@ -594,12 +600,6 @@ static long snp_guest_ioctl(struct file *file, unsigned int ioctl, unsigned long > if (!input.msg_version) > return -EINVAL; > > - /* Check if the VMPCK is not empty */ > - if (is_vmpck_empty(snp_dev)) { > - dev_err_ratelimited(snp_dev->dev, "VMPCK is disabled\n"); > - return -ENOTTY; > - } > - > switch (ioctl) { > case SNP_GET_REPORT: > ret = get_report(snp_dev, &input); > @@ -869,12 +869,6 @@ static int sev_report_new(struct tsm_report *report, void *data) > if (!buf) > return -ENOMEM; > > - /* Check if the VMPCK is not empty */ > - if (is_vmpck_empty(snp_dev)) { > - dev_err_ratelimited(snp_dev->dev, "VMPCK is disabled\n"); > - return -ENOTTY; > - } > - > cert_table = buf + report_size; > struct snp_ext_report_req ext_req = { > .data = { .vmpl = desc->privlevel }, > > >> I believe is_vmpck_empty() can get a false and then be waiting on the >> mutex while snp_disable_vmpck() is called. Suddenly the code thinks the >> VMPCK is valid when it isn't anymore. Not sure if that matters, as the >> guest request will fail anyway? > > The above code will fail early. > >> >> Thanks, >> Tom >> > > Regards > Nikunj >
diff --git a/drivers/virt/coco/sev-guest/sev-guest.c b/drivers/virt/coco/sev-guest/sev-guest.c index 92734a2345a6..42f7126f1718 100644 --- a/drivers/virt/coco/sev-guest/sev-guest.c +++ b/drivers/virt/coco/sev-guest/sev-guest.c @@ -345,6 +345,8 @@ static int snp_send_guest_request(struct snp_guest_dev *snp_dev, struct snp_gues u64 seqno; int rc; + guard(mutex)(&snp_cmd_mutex); + /* Get message sequence and verify that its a non-zero */ seqno = snp_get_msg_seqno(snp_dev); if (!seqno) @@ -419,8 +421,6 @@ static int get_report(struct snp_guest_dev *snp_dev, struct snp_guest_request_io struct snp_report_resp *report_resp; int rc, resp_len; - lockdep_assert_held(&snp_cmd_mutex); - if (!arg->req_data || !arg->resp_data) return -EINVAL; @@ -458,8 +458,6 @@ static int get_derived_key(struct snp_guest_dev *snp_dev, struct snp_guest_reque /* Response data is 64 bytes and max authsize for GCM is 16 bytes. */ u8 buf[64 + 16]; - lockdep_assert_held(&snp_cmd_mutex); - if (!arg->req_data || !arg->resp_data) return -EINVAL; @@ -501,8 +499,6 @@ static int get_ext_report(struct snp_guest_dev *snp_dev, struct snp_guest_reques int ret, npages = 0, resp_len; sockptr_t certs_address; - lockdep_assert_held(&snp_cmd_mutex); - if (sockptr_is_null(io->req_data) || sockptr_is_null(io->resp_data)) return -EINVAL; @@ -590,12 +586,9 @@ static long snp_guest_ioctl(struct file *file, unsigned int ioctl, unsigned long if (!input.msg_version) return -EINVAL; - mutex_lock(&snp_cmd_mutex); - /* Check if the VMPCK is not empty */ if (is_vmpck_empty(snp_dev)) { dev_err_ratelimited(snp_dev->dev, "VMPCK is disabled\n"); - mutex_unlock(&snp_cmd_mutex); return -ENOTTY; } @@ -620,8 +613,6 @@ static long snp_guest_ioctl(struct file *file, unsigned int ioctl, unsigned long break; } - mutex_unlock(&snp_cmd_mutex); - if (input.exitinfo2 && copy_to_user(argp, &input, sizeof(input))) return -EFAULT; @@ -736,8 +727,6 @@ static int sev_svsm_report_new(struct tsm_report *report, void *data) man_len = SZ_4K; certs_len = SEV_FW_BLOB_MAX_SIZE; - guard(mutex)(&snp_cmd_mutex); - if (guid_is_null(&desc->service_guid)) { call_id = SVSM_ATTEST_CALL(SVSM_ATTEST_SERVICES); } else { @@ -872,8 +861,6 @@ static int sev_report_new(struct tsm_report *report, void *data) if (!buf) return -ENOMEM; - guard(mutex)(&snp_cmd_mutex); - /* Check if the VMPCK is not empty */ if (is_vmpck_empty(snp_dev)) { dev_err_ratelimited(snp_dev->dev, "VMPCK is disabled\n");
The SNP command mutex is used to serialize access to the shared buffer, command handling, and message sequence number. All shared buffer, command handling, and message sequence updates are done within snp_send_guest_request(), so moving the mutex to this function is appropriate and maintains the critical section. Since the mutex is now taken at a later point in time, remove the lockdep checks that occur before taking the mutex. Signed-off-by: Nikunj A Dadhania <nikunj@amd.com> --- drivers/virt/coco/sev-guest/sev-guest.c | 17 ++--------------- 1 file changed, 2 insertions(+), 15 deletions(-)