[PATCH v2] virt: tdx-guest: Handle GetQuote request error code

[PATCH v2] virt: tdx-guest: Handle GetQuote request error code

[Kuppuswamy Sathyanarayanan]

During the TDX guest attestation process, TSM ConfigFS ABI is used by
the user attestation agent to get the signed VM measurement data (a.k.a
Quote), which can be used by a remote verifier to validate the
trustworthiness of the guest. When a user requests for the Quote data
via the ConfigFS ABI, the TDX Quote generation handler
(tdx_report_new()) forwards the request to VMM (or QE) via a hypercall,
and then shares the output with the user.

Currently, when handling the Quote generation request, tdx_report_new()
handler only checks whether the VMM successfully processed the request
and if it is true it returns success and shares the output to the user
without actually validating the output data. Since the VMM can return
error even after processing the Quote request, always returning success
for the processed requests is incorrect and will create confusion to
the user. Although for the failed request, output buffer length will
be zero and can also be used by the user to identify the failure case,
it will be more clear to return error for all failed cases.

Validate the Quote data output status and return error code for all
failed cases.

Fixes: f4738f56d1dc ("virt: tdx-guest: Add Quote generation support using TSM_REPORTS")
Reported-by: Xiaoyao Li <xiaoyao.li@intel.com>
Closes: https://lore.kernel.org/linux-coco/6bdf569c-684a-4459-af7c-4430691804eb@linux.intel.com/T/#u
Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
---

Changes since v1:
 * Updated the commit log (Kirill)

 drivers/virt/coco/tdx-guest/tdx-guest.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/virt/coco/tdx-guest/tdx-guest.c b/drivers/virt/coco/tdx-guest/tdx-guest.c
index 1253bf76b570..61368318fa39 100644
--- a/drivers/virt/coco/tdx-guest/tdx-guest.c
+++ b/drivers/virt/coco/tdx-guest/tdx-guest.c
@@ -228,6 +228,12 @@ static int tdx_report_new(struct tsm_report *report, void *data)
 		goto done;
 	}
 
+	if (quote_buf->status != GET_QUOTE_SUCCESS) {
+		pr_err("GetQuote request failed, ret %llx\n", quote_buf->status);
+		ret = -EIO;
+		goto done;
+	}
+
 	buf = kvmemdup(quote_buf->data, quote_buf->out_len, GFP_KERNEL);
 	if (!buf) {
 		ret = -ENOMEM;
-- 
2.25.1

Re: [PATCH v2] virt: tdx-guest: Handle GetQuote request error code

[Kirill A . Shutemov]

On Thu, Jan 11, 2024 at 03:32:45AM +0000, Kuppuswamy Sathyanarayanan wrote:
> During the TDX guest attestation process, TSM ConfigFS ABI is used by
> the user attestation agent to get the signed VM measurement data (a.k.a
> Quote), which can be used by a remote verifier to validate the
> trustworthiness of the guest. When a user requests for the Quote data
> via the ConfigFS ABI, the TDX Quote generation handler
> (tdx_report_new()) forwards the request to VMM (or QE) via a hypercall,
> and then shares the output with the user.
> 
> Currently, when handling the Quote generation request, tdx_report_new()
> handler only checks whether the VMM successfully processed the request
> and if it is true it returns success and shares the output to the user
> without actually validating the output data. Since the VMM can return
> error even after processing the Quote request, always returning success
> for the processed requests is incorrect and will create confusion to
> the user. Although for the failed request, output buffer length will
> be zero and can also be used by the user to identify the failure case,
> it will be more clear to return error for all failed cases.
> 
> Validate the Quote data output status and return error code for all
> failed cases.
> 
> Fixes: f4738f56d1dc ("virt: tdx-guest: Add Quote generation support using TSM_REPORTS")
> Reported-by: Xiaoyao Li <xiaoyao.li@intel.com>
> Closes: https://lore.kernel.org/linux-coco/6bdf569c-684a-4459-af7c-4430691804eb@linux.intel.com/T/#u
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

Reviewed-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>

-- 
  Kiryl Shutsemau / Kirill A. Shutemov

Re: [PATCH v2] virt: tdx-guest: Handle GetQuote request error code

[Xiaoyao Li]

On 1/11/2024 11:32 AM, Kuppuswamy Sathyanarayanan wrote:
> During the TDX guest attestation process, TSM ConfigFS ABI is used by
> the user attestation agent to get the signed VM measurement data (a.k.a
> Quote), which can be used by a remote verifier to validate the
> trustworthiness of the guest. When a user requests for the Quote data
> via the ConfigFS ABI, the TDX Quote generation handler
> (tdx_report_new()) forwards the request to VMM (or QE) via a hypercall,
> and then shares the output with the user.
> 
> Currently, when handling the Quote generation request, tdx_report_new()
> handler only checks whether the VMM successfully processed the request
> and if it is true it returns success and shares the output to the user
> without actually validating the output data. Since the VMM can return
> error even after processing the Quote request, always returning success
> for the processed requests is incorrect and will create confusion to
> the user. Although for the failed request, output buffer length will
> be zero and can also be used by the user to identify the failure case,
> it will be more clear to return error for all failed cases.
> 
> Validate the Quote data output status and return error code for all
> failed cases.
> 
> Fixes: f4738f56d1dc ("virt: tdx-guest: Add Quote generation support using TSM_REPORTS")
> Reported-by: Xiaoyao Li <xiaoyao.li@intel.com>
> Closes: https://lore.kernel.org/linux-coco/6bdf569c-684a-4459-af7c-4430691804eb@linux.intel.com/T/#u
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>

Reviewed-by: Xiaoyao Li <xiaoyao.li@intel.com>

> ---
> 
> Changes since v1:
>   * Updated the commit log (Kirill)
> 
>   drivers/virt/coco/tdx-guest/tdx-guest.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/virt/coco/tdx-guest/tdx-guest.c b/drivers/virt/coco/tdx-guest/tdx-guest.c
> index 1253bf76b570..61368318fa39 100644
> --- a/drivers/virt/coco/tdx-guest/tdx-guest.c
> +++ b/drivers/virt/coco/tdx-guest/tdx-guest.c
> @@ -228,6 +228,12 @@ static int tdx_report_new(struct tsm_report *report, void *data)
>   		goto done;
>   	}
>   
> +	if (quote_buf->status != GET_QUOTE_SUCCESS) {
> +		pr_err("GetQuote request failed, ret %llx\n", quote_buf->status);
> +		ret = -EIO;
> +		goto done;
> +	}
> +
>   	buf = kvmemdup(quote_buf->data, quote_buf->out_len, GFP_KERNEL);
>   	if (!buf) {
>   		ret = -ENOMEM;

Re: [PATCH v2] virt: tdx-guest: Handle GetQuote request error code

[Huang, Kai]

Although for the failed request, output buffer length will
> be zero and can also be used by the user to identify the failure case,
> it will be more clear to return error for all failed cases.

Nit:

If I read correctly, kvmemdup() returns ZERO_SIZE_PTR if you pass the 0 size to
it, so w/o this patch it seems the kernel will report ZERO_SIZE_PTR as the
buffer to userspace.  Not sure whether this is an issue.

I guess what I want to say is, should we explicitly check quote_buf->out_len not
being 0 even the status shows success?  After all the out_len is set by the VMM.

Anyway:

Acked-by: Kai Huang <kai.huang@intel.com>


> 
> Validate the Quote data output status and return error code for all
> failed cases.
> 
> Fixes: f4738f56d1dc ("virt: tdx-guest: Add Quote generation support using TSM_REPORTS")
> Reported-by: Xiaoyao Li <xiaoyao.li@intel.com>
> Closes: https://lore.kernel.org/linux-coco/6bdf569c-684a-4459-af7c-4430691804eb@linux.intel.com/T/#u
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
> 
> Changes since v1:
>  * Updated the commit log (Kirill)
> 
>  drivers/virt/coco/tdx-guest/tdx-guest.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/virt/coco/tdx-guest/tdx-guest.c b/drivers/virt/coco/tdx-guest/tdx-guest.c
> index 1253bf76b570..61368318fa39 100644
> --- a/drivers/virt/coco/tdx-guest/tdx-guest.c
> +++ b/drivers/virt/coco/tdx-guest/tdx-guest.c
> @@ -228,6 +228,12 @@ static int tdx_report_new(struct tsm_report *report, void *data)
>  		goto done;
>  	}
>  
> +	if (quote_buf->status != GET_QUOTE_SUCCESS) {
> +		pr_err("GetQuote request failed, ret %llx\n", quote_buf->status);
> +		ret = -EIO;
> +		goto done;
> +	}
> +
>  	buf = kvmemdup(quote_buf->data, quote_buf->out_len, GFP_KERNEL);
>  	if (!buf) {
>  		ret = -ENOMEM;

Re: [PATCH v2] virt: tdx-guest: Handle GetQuote request error code

[Kuppuswamy Sathyanarayanan]

Hi x86 Maintainers,

On 1/10/24 7:32 PM, Kuppuswamy Sathyanarayanan wrote:
> During the TDX guest attestation process, TSM ConfigFS ABI is used by
> the user attestation agent to get the signed VM measurement data (a.k.a
> Quote), which can be used by a remote verifier to validate the
> trustworthiness of the guest. When a user requests for the Quote data
> via the ConfigFS ABI, the TDX Quote generation handler
> (tdx_report_new()) forwards the request to VMM (or QE) via a hypercall,
> and then shares the output with the user.
>
> Currently, when handling the Quote generation request, tdx_report_new()
> handler only checks whether the VMM successfully processed the request
> and if it is true it returns success and shares the output to the user
> without actually validating the output data. Since the VMM can return
> error even after processing the Quote request, always returning success
> for the processed requests is incorrect and will create confusion to
> the user. Although for the failed request, output buffer length will
> be zero and can also be used by the user to identify the failure case,
> it will be more clear to return error for all failed cases.
>
> Validate the Quote data output status and return error code for all
> failed cases.
>
> Fixes: f4738f56d1dc ("virt: tdx-guest: Add Quote generation support using TSM_REPORTS")
> Reported-by: Xiaoyao Li <xiaoyao.li@intel.com>
> Closes: https://lore.kernel.org/linux-coco/6bdf569c-684a-4459-af7c-4430691804eb@linux.intel.com/T/#u
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---

Can you consider merging this fix? It already got acks from Kirill, Kai and Li. Do
you want me rebase it and resend it with updated tags?

>
> Changes since v1:
>  * Updated the commit log (Kirill)
>
>  drivers/virt/coco/tdx-guest/tdx-guest.c | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/drivers/virt/coco/tdx-guest/tdx-guest.c b/drivers/virt/coco/tdx-guest/tdx-guest.c
> index 1253bf76b570..61368318fa39 100644
> --- a/drivers/virt/coco/tdx-guest/tdx-guest.c
> +++ b/drivers/virt/coco/tdx-guest/tdx-guest.c
> @@ -228,6 +228,12 @@ static int tdx_report_new(struct tsm_report *report, void *data)
>  		goto done;
>  	}
>  
> +	if (quote_buf->status != GET_QUOTE_SUCCESS) {
> +		pr_err("GetQuote request failed, ret %llx\n", quote_buf->status);
> +		ret = -EIO;
> +		goto done;
> +	}
> +
>  	buf = kvmemdup(quote_buf->data, quote_buf->out_len, GFP_KERNEL);
>  	if (!buf) {
>  		ret = -ENOMEM;

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

Re: [PATCH v2] virt: tdx-guest: Handle GetQuote request error code

[Dan Williams]

Kuppuswamy Sathyanarayanan wrote:
> During the TDX guest attestation process, TSM ConfigFS ABI is used by
> the user attestation agent to get the signed VM measurement data (a.k.a
> Quote), which can be used by a remote verifier to validate the
> trustworthiness of the guest. When a user requests for the Quote data
> via the ConfigFS ABI, the TDX Quote generation handler
> (tdx_report_new()) forwards the request to VMM (or QE) via a hypercall,
> and then shares the output with the user.
> 
> Currently, when handling the Quote generation request, tdx_report_new()
> handler only checks whether the VMM successfully processed the request
> and if it is true it returns success and shares the output to the user
> without actually validating the output data. Since the VMM can return
> error even after processing the Quote request, always returning success
> for the processed requests is incorrect and will create confusion to
> the user. Although for the failed request, output buffer length will
> be zero and can also be used by the user to identify the failure case,
> it will be more clear to return error for all failed cases.

This is a lot of text. More is not necessarily better.

---
The tdx-guest driver marshals requests via hypercall to have a quoting
enclave sign attestation evidence about the current state of the TD.
There are 2 possible failures, a transport failure (failure to
communicate with the quoting agent) and payload failure (a failed
quote). The driver only checks the former, update it to consider the
latter payload errors as well.
---


> 
> Validate the Quote data output status and return error code for all
> failed cases.
> 
> Fixes: f4738f56d1dc ("virt: tdx-guest: Add Quote generation support using TSM_REPORTS")
> Reported-by: Xiaoyao Li <xiaoyao.li@intel.com>
> Closes: https://lore.kernel.org/linux-coco/6bdf569c-684a-4459-af7c-4430691804eb@linux.intel.com/T/#u
> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
> ---
> 
> Changes since v1:
>  * Updated the commit log (Kirill)
> 
>  drivers/virt/coco/tdx-guest/tdx-guest.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/virt/coco/tdx-guest/tdx-guest.c b/drivers/virt/coco/tdx-guest/tdx-guest.c
> index 1253bf76b570..61368318fa39 100644
> --- a/drivers/virt/coco/tdx-guest/tdx-guest.c
> +++ b/drivers/virt/coco/tdx-guest/tdx-guest.c
> @@ -228,6 +228,12 @@ static int tdx_report_new(struct tsm_report *report, void *data)
>  		goto done;
>  	}
>  
> +	if (quote_buf->status != GET_QUOTE_SUCCESS) {
> +		pr_err("GetQuote request failed, ret %llx\n", quote_buf->status);

Do you really want to spam the log on every error? I would expect
pr_err() for events that are fatal to driver operation that might
indicate conditions where maybe the TD should give up on the host.

Yes, there are other pr_err() in this function and I am kicking myself
for not scrutinizing those more closely. It is likely enough to
distinguish transport errors vs payload / quote errors with ENXIO and
EIO.

Otherwise if there is an exceedingly good reason to keep this driver
chirping into the kernel log then these likely also want to be
rate-limited. If they are "just in case" debug messages, then move them
to pr_debug().

Re: [PATCH v2] virt: tdx-guest: Handle GetQuote request error code

[Kuppuswamy Sathyanarayanan]

On 2/22/24 9:48 PM, Dan Williams wrote:
> Kuppuswamy Sathyanarayanan wrote:
>> During the TDX guest attestation process, TSM ConfigFS ABI is used by
>> the user attestation agent to get the signed VM measurement data (a.k.a
>> Quote), which can be used by a remote verifier to validate the
>> trustworthiness of the guest. When a user requests for the Quote data
>> via the ConfigFS ABI, the TDX Quote generation handler
>> (tdx_report_new()) forwards the request to VMM (or QE) via a hypercall,
>> and then shares the output with the user.
>>
>> Currently, when handling the Quote generation request, tdx_report_new()
>> handler only checks whether the VMM successfully processed the request
>> and if it is true it returns success and shares the output to the user
>> without actually validating the output data. Since the VMM can return
>> error even after processing the Quote request, always returning success
>> for the processed requests is incorrect and will create confusion to
>> the user. Although for the failed request, output buffer length will
>> be zero and can also be used by the user to identify the failure case,
>> it will be more clear to return error for all failed cases.
> This is a lot of text. More is not necessarily better.
>
> ---
> The tdx-guest driver marshals requests via hypercall to have a quoting
> enclave sign attestation evidence about the current state of the TD.
> There are 2 possible failures, a transport failure (failure to
> communicate with the quoting agent) and payload failure (a failed
> quote). The driver only checks the former, update it to consider the
> latter payload errors as well.
> ---

Looks better. I will use it in next version.

>
>
>> Validate the Quote data output status and return error code for all
>> failed cases.
>>
>> Fixes: f4738f56d1dc ("virt: tdx-guest: Add Quote generation support using TSM_REPORTS")
>> Reported-by: Xiaoyao Li <xiaoyao.li@intel.com>
>> Closes: https://lore.kernel.org/linux-coco/6bdf569c-684a-4459-af7c-4430691804eb@linux.intel.com/T/#u
>> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>
>> ---
>>
>> Changes since v1:
>>  * Updated the commit log (Kirill)
>>
>>  drivers/virt/coco/tdx-guest/tdx-guest.c | 6 ++++++
>>  1 file changed, 6 insertions(+)
>>
>> diff --git a/drivers/virt/coco/tdx-guest/tdx-guest.c b/drivers/virt/coco/tdx-guest/tdx-guest.c
>> index 1253bf76b570..61368318fa39 100644
>> --- a/drivers/virt/coco/tdx-guest/tdx-guest.c
>> +++ b/drivers/virt/coco/tdx-guest/tdx-guest.c
>> @@ -228,6 +228,12 @@ static int tdx_report_new(struct tsm_report *report, void *data)
>>  		goto done;
>>  	}
>>  
>> +	if (quote_buf->status != GET_QUOTE_SUCCESS) {
>> +		pr_err("GetQuote request failed, ret %llx\n", quote_buf->status);
> Do you really want to spam the log on every error? I would expect
> pr_err() for events that are fatal to driver operation that might
> indicate conditions where maybe the TD should give up on the host.
>
> Yes, there are other pr_err() in this function and I am kicking myself
> for not scrutinizing those more closely. It is likely enough to
> distinguish transport errors vs payload / quote errors with ENXIO and
> EIO.
>
> Otherwise if there is an exceedingly good reason to keep this driver
> chirping into the kernel log then these likely also want to be
> rate-limited. If they are "just in case" debug messages, then move them
> to pr_debug().

Ok. Makes sense. I will convert it into a pr_debug.

I will submit a separate patch to fix other pr_err usage in this driver.
Expect Quote timeout, the rest of the failure does not affect the
usage of the driver.

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer