Re: [PATCH RFC 18/20] tdx-guest: Expose the native attestation service API

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] tdx: client attach failure aborts core driver initialization
- [Low] tdx: memory bloat per TSM report due to missing right-sizing
- [Low] Unreachable return statements after scoped_cond_guard
--

commit b1dd6a27af0398a52344b35c7786c4b3fbbf85c3
Author: Jiri Pirko <jiri@resnulli.us>

tdx-guest: Expose the native attestation service API

This patch shares TDREPORT, Quote and measurement-register operations with one
in-kernel client while preserving the existing misc, TSM report and TSM
MR interfaces. It ties clients to service initialization and removal.

> diff --git a/drivers/virt/coco/tdx-guest/tdx-guest.c b/drivers/virt/coco/tdx-guest/tdx-guest.c
> index d0303e31e8161..0d53170602463 100644
> --- a/drivers/virt/coco/tdx-guest/tdx-guest.c
> +++ b/drivers/virt/coco/tdx-guest/tdx-guest.c

[ ... ]

> +static int tdx_do_report(sockptr_t data, sockptr_t tdreport)
> +{
> +	scoped_cond_guard(mutex_intr, return -EINTR, &mr_lock)
> +		return __tdx_do_report(data, tdreport);
>  	return 0;
>  }

[Severity: Low]
Is the trailing return 0 unreachable? When scoped_cond_guard is used with a
single return statement as its body, both the success and failure branches
return immediately. The loop's internal break is never reached, preventing
execution from falling through to the statements following the macro.

[ ... ]

> +static int tdx_guest_quote_get(struct device *dev, const u8 *reportdata,
> +			       size_t length, void *out, size_t capacity,
> +			       size_t *out_length)
> +{
> +	if (!tdx_guest_device_valid(dev))
> +		return -ENODEV;
> +	scoped_cond_guard(mutex_intr, return -EINTR, &quote_lock)
> +		return __tdx_guest_quote_get(reportdata, length, out,
> +					     capacity, out_length);
> +	return 0;
> +}

[Severity: Low]
Is this trailing return 0 also unreachable for the same reason as in
tdx_do_report()?

[ ... ]

> +static int tdx_report_new_locked(struct tsm_report *report, void *data)
> +{
> +	struct tsm_report_desc *desc = &report->desc;
> +	size_t length;
> +	void *buf;
> +	int ret;
> +
> +	buf = kvmalloc(TDX_QUOTE_MAX_LEN, GFP_KERNEL);
> +	if (!buf)
> +		return -ENOMEM;
> +	ret = __tdx_guest_quote_get(desc->inblob, desc->inblob_len,
> +				    buf, TDX_QUOTE_MAX_LEN, &length);
> +	if (ret) {
> +		kvfree(buf);
> +		return ret;
> +	}
> +	report->outblob = buf;
> +	report->outblob_len = length;
> +	return 0;
> +}

[Severity: Low]
Does this code introduce persistent memory bloat for each TSM report?

The buffer is allocated at the maximum possible size of 128KB but is never
shrunk to the actual quote length returned in the length variable.

The previous implementation used kvmemdup() to size the buffer exactly to
the output length, which is typically much smaller than the 128KB maximum.
By leaving it at 128KB, could this increase the memory footprint for
concurrent reports?

[ ... ]

>  static int __init tdx_guest_init(void)
>  {
>  	int ret;

[ ... ]

>  	ret = tsm_report_register(&tdx_tsm_ops, NULL);
>  	if (ret)
>  		goto free_quote;
> +	scoped_guard(mutex, &client_lock) {
> +		tdx_service_ready = true;
> +		ret = tdx_guest_client_attach();
> +		if (ret)
> +			tdx_service_ready = false;
> +	}
> +	if (ret)
> +		goto unregister_tsm;
>  
>  	return 0;
>  
> +unregister_tsm:
> +	tsm_report_unregister(&tdx_tsm_ops);

[Severity: High]
Does a failure to attach a client here abort the entire core driver
initialization?

If a built-in client registers before tdx_guest_init() runs, a failure in
the client's add() callback will cause tdx_guest_client_attach() to fail.
This error is returned, causing a jump to unregister_tsm, which aborts the
initialization of the TDX guest driver and breaks other frontends like the
misc ioctl and configfs.

If the client is loaded after the driver, its failure does not affect the
core driver. Should the core provider ignore an optional downstream client's
failure and continue its initialization?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015145053.3664509-1-jiri@resnulli.us?part=18