linux-kernel - Re: [PATCH v11 09/20] virt: sev-guest: Reduce the scope of SNP command mutex

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <60051636-35ea-d7a2-9204-037adea1d509@amd.com>
Date: Fri, 13 Sep 2024 09:06:17 -0500
From: Tom Lendacky <thomas.lendacky@....com>
To: "Nikunj A. Dadhania" <nikunj@....com>, linux-kernel@...r.kernel.org,
 bp@...en8.de, x86@...nel.org, kvm@...r.kernel.org
Cc: mingo@...hat.com, tglx@...utronix.de, dave.hansen@...ux.intel.com,
 pgonda@...gle.com, seanjc@...gle.com, pbonzini@...hat.com
Subject: Re: [PATCH v11 09/20] virt: sev-guest: Reduce the scope of SNP
 command mutex

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
>