Re: [RFC PATCH] virt: tdx-guest: Remove quote generation via ioctl

[Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com>]

On 1/23/24 8:07 AM, Nikolay Borisov wrote:
> When this driver got merged initially there was no widely agreed upon
> interface how the quote generation interface will work so having an
> ioctl made sense. However, there's now a vendor-neutral interface via
> configfs. Just remove the old ioctl interface and leave only the the
> configfs one.
>
> Signed-off-by: Nikolay Borisov <nik.borisov@suse.com>
> ---

This ABI allows the user to get the raw report which is further used
for Quote generation via vsock. AFAIK, some vendors (TDX users) and
DCAP user libraries are still using this ABI to support attestation over
vsock model.

Don't you think we should wait till there are no users before considering
removing it?

>
> I think it's better to remove the legacy interface now, before there's been a
> wide adoption of it, cementing it as an ABI of sorts.
>
>  drivers/virt/coco/tdx-guest/tdx-guest.c | 75 +------------------------
>  1 file changed, 2 insertions(+), 73 deletions(-)
>
> diff --git a/drivers/virt/coco/tdx-guest/tdx-guest.c b/drivers/virt/coco/tdx-guest/tdx-guest.c
> index 1253bf76b570..1642a0f70333 100644
> --- a/drivers/virt/coco/tdx-guest/tdx-guest.c
> +++ b/drivers/virt/coco/tdx-guest/tdx-guest.c
> @@ -66,41 +66,6 @@ static DEFINE_MUTEX(quote_lock);
>   */
>  static u32 getquote_timeout = 30;
>
> -static long tdx_get_report0(struct tdx_report_req __user *req)
> -{
> -	u8 *reportdata, *tdreport;
> -	long ret;
> -
> -	reportdata = kmalloc(TDX_REPORTDATA_LEN, GFP_KERNEL);
> -	if (!reportdata)
> -		return -ENOMEM;
> -
> -	tdreport = kzalloc(TDX_REPORT_LEN, GFP_KERNEL);
> -	if (!tdreport) {
> -		ret = -ENOMEM;
> -		goto out;
> -	}
> -
> -	if (copy_from_user(reportdata, req->reportdata, TDX_REPORTDATA_LEN)) {
> -		ret = -EFAULT;
> -		goto out;
> -	}
> -
> -	/* Generate TDREPORT0 using "TDG.MR.REPORT" TDCALL */
> -	ret = tdx_mcall_get_report0(reportdata, tdreport);
> -	if (ret)
> -		goto out;
> -
> -	if (copy_to_user(req->tdreport, tdreport, TDX_REPORT_LEN))
> -		ret = -EFAULT;
> -
> -out:
> -	kfree(reportdata);
> -	kfree(tdreport);
> -
> -	return ret;
> -}
> -
>  static void free_quote_buf(void *buf)
>  {
>  	size_t len = PAGE_ALIGN(GET_QUOTE_BUF_SIZE);
> @@ -249,29 +214,6 @@ static int tdx_report_new(struct tsm_report *report, void *data)
>  	return ret;
>  }
>
> -static long tdx_guest_ioctl(struct file *file, unsigned int cmd,
> -			    unsigned long arg)
> -{
> -	switch (cmd) {
> -	case TDX_CMD_GET_REPORT0:
> -		return tdx_get_report0((struct tdx_report_req __user *)arg);
> -	default:
> -		return -ENOTTY;
> -	}
> -}
> -
> -static const struct file_operations tdx_guest_fops = {
> -	.owner = THIS_MODULE,
> -	.unlocked_ioctl = tdx_guest_ioctl,
> -	.llseek = no_llseek,
> -};
> -
> -static struct miscdevice tdx_misc_dev = {
> -	.name = KBUILD_MODNAME,
> -	.minor = MISC_DYNAMIC_MINOR,
> -	.fops = &tdx_guest_fops,
> -};
> -
>  static const struct x86_cpu_id tdx_guest_ids[] = {
>  	X86_MATCH_FEATURE(X86_FEATURE_TDX_GUEST, NULL),
>  	{}
> @@ -290,27 +232,15 @@ static int __init tdx_guest_init(void)
>  	if (!x86_match_cpu(tdx_guest_ids))
>  		return -ENODEV;
>
> -	ret = misc_register(&tdx_misc_dev);
> -	if (ret)
> -		return ret;
> -
>  	quote_data = alloc_quote_buf();
>  	if (!quote_data) {
>  		pr_err("Failed to allocate Quote buffer\n");
> -		ret = -ENOMEM;
> -		goto free_misc;
> +		return -ENOMEM;
>  	}
>
>  	ret = tsm_register(&tdx_tsm_ops, NULL, NULL);
>  	if (ret)
> -		goto free_quote;
> -
> -	return 0;
> -
> -free_quote:
> -	free_quote_buf(quote_data);
> -free_misc:
> -	misc_deregister(&tdx_misc_dev);
> +		free_quote_buf(quote_data);
>
>  	return ret;
>  }
> @@ -320,7 +250,6 @@ static void __exit tdx_guest_exit(void)
>  {
>  	tsm_unregister(&tdx_tsm_ops);
>  	free_quote_buf(quote_data);
> -	misc_deregister(&tdx_misc_dev);
>  }
>  module_exit(tdx_guest_exit);
>
> --
> 2.34.1
>
>
-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer