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
