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
>
