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

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

[Nikolay Borisov]

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>
---

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

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

[Kuppuswamy Sathyanarayanan]

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

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

[Nikolay Borisov]

On 23.01.24 г. 19:51 ч., Kuppuswamy Sathyanarayanan wrote:
> 
> 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?

Given that hw with TDX was just released I'd be surprised if there are 
any users? But then again, this is an RFC so let's get opinions :)


> 
>>
>> 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
>>
>>

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

[Dan Williams]

Nikolay Borisov wrote:
> 
> 
> On 23.01.24 г. 19:51 ч., Kuppuswamy Sathyanarayanan wrote:
> > 
> > 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?
> 
> Given that hw with TDX was just released I'd be surprised if there are 
> any users? But then again, this is an RFC so let's get opinions :)
> 

The assumption is that this tdx_guest_ioctl() ABI has never appeared in
an enterprise distro kernel. If that assumption is valid, it
significantly reduces the long term support exposure.

At a minimum, a build time option to disable the ioctl() path and a
runtime deprecation warning would be suitable. Certainly just removing a
never-been-enterprise-shipped ABI and see who screams is one way to
start the negotiation of the deprecation period. The current status of
no deprecation notification is difficult to justify.

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

[Daniel P. Berrangé]

On Tue, Jan 23, 2024 at 11:06:01AM -0800, Dan Williams wrote:
> Nikolay Borisov wrote:
> > 
> > 
> > On 23.01.24 г. 19:51 ч., Kuppuswamy Sathyanarayanan wrote:
> > > 
> > > 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?
> > 
> > Given that hw with TDX was just released I'd be surprised if there are 
> > any users? But then again, this is an RFC so let's get opinions :)
> > 
> 
> The assumption is that this tdx_guest_ioctl() ABI has never appeared in
> an enterprise distro kernel. If that assumption is valid, it
> significantly reduces the long term support exposure.

This ioctl is present in current RHEL-9 kernels

  https://gitlab.com/redhat/centos-stream/src/kernel/centos-stream-9/-/blob/main/drivers/virt/coco/tdx-guest/tdx-guest.c?ref_type=heads#L69

and this is exposed to users when running RHEL-9 guest on Azure
cloud with TDX support.

I've not directly checked, but I would assume it is also probably
included in Ubuntu LTS kernels too, since they also target TDX in
Azure.

No idea what the status of SLES is wrt TDX / Azure offhand.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

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

[Dan Williams]

Daniel P. Berrangé wrote:
> On Tue, Jan 23, 2024 at 11:06:01AM -0800, Dan Williams wrote:
> > Nikolay Borisov wrote:
> > > 
> > > 
> > > On 23.01.24 г. 19:51 ч., Kuppuswamy Sathyanarayanan wrote:
> > > > 
> > > > 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?
> > > 
> > > Given that hw with TDX was just released I'd be surprised if there are 
> > > any users? But then again, this is an RFC so let's get opinions :)
> > > 
> > 
> > The assumption is that this tdx_guest_ioctl() ABI has never appeared in
> > an enterprise distro kernel. If that assumption is valid, it
> > significantly reduces the long term support exposure.
> 
> This ioctl is present in current RHEL-9 kernels
> 
>   https://gitlab.com/redhat/centos-stream/src/kernel/centos-stream-9/-/blob/main/drivers/virt/coco/tdx-guest/tdx-guest.c?ref_type=heads#L69
> 
> and this is exposed to users when running RHEL-9 guest on Azure
> cloud with TDX support.

Ah, ok, thanks for looking that up. Also good to see the new interface
is also backported into that kernel.

> I've not directly checked, but I would assume it is also probably
> included in Ubuntu LTS kernels too, since they also target TDX in
> Azure.
> 
> No idea what the status of SLES is wrt TDX / Azure offhand.

Seems like this needs to follow the typical upstream deprecation of
notifying in Kconfig and maybe a runtime message, and then circle back
in a couple years to remove when distros stop enabling the legacy
interface.

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

[Jeremi Piotrowski]

On 23/01/2024 21:09, Dan Williams wrote:
> Daniel P. Berrangé wrote:
>> On Tue, Jan 23, 2024 at 11:06:01AM -0800, Dan Williams wrote:
>>> Nikolay Borisov wrote:
>>>>
>>>>
>>>> On 23.01.24 г. 19:51 ч., Kuppuswamy Sathyanarayanan wrote:
>>>>>
>>>>> 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?
>>>>
>>>> Given that hw with TDX was just released I'd be surprised if there are 
>>>> any users? But then again, this is an RFC so let's get opinions :)
>>>>
>>>
>>> The assumption is that this tdx_guest_ioctl() ABI has never appeared in
>>> an enterprise distro kernel. If that assumption is valid, it
>>> significantly reduces the long term support exposure.
>>
>> This ioctl is present in current RHEL-9 kernels
>>
>>   https://gitlab.com/redhat/centos-stream/src/kernel/centos-stream-9/-/blob/main/drivers/virt/coco/tdx-guest/tdx-guest.c?ref_type=heads#L69
>>
>> and this is exposed to users when running RHEL-9 guest on Azure
>> cloud with TDX support.
> 
> Ah, ok, thanks for looking that up. Also good to see the new interface
> is also backported into that kernel.

Azure TDX instances don't actually have access to that ioctl() interface because
they run with TD partitioning and that module call is not exposed to linux. The
TD report is used to chain trust to the TPM provided by the paravisor (TD L1),
the TD quote can be fetched through a TPM NVRAM index.

So I wouldn't block removal on this.

> 
>> I've not directly checked, but I would assume it is also probably
>> included in Ubuntu LTS kernels too, since they also target TDX in
>> Azure.
>>
>> No idea what the status of SLES is wrt TDX / Azure offhand.
> 
> Seems like this needs to follow the typical upstream deprecation of
> notifying in Kconfig and maybe a runtime message, and then circle back
> in a couple years to remove when distros stop enabling the legacy
> interface.

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

[Dionna Amalie Glaze]

On Tue, Jan 23, 2024 at 10:24 AM Nikolay Borisov <nik.borisov@suse.com> wrote:
>
>
>
> On 23.01.24 г. 19:51 ч., Kuppuswamy Sathyanarayanan wrote:
> >
> > 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?
>
> Given that hw with TDX was just released I'd be surprised if there are
> any users? But then again, this is an RFC so let's get opinions :)
>

There are users in private preview, so non-production testing, but still users.
Our (Google's) testing environment is already updated to use the new
configfs-tsm interface. I think with the technology being so new, we
ought to remove the old interface sooner rather than later to make the
early adopters the only ones who need to incur penalties for a
volatile interface.

>
> >
> >>
> >> 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
> >>
> >>
>


-- 
-Dionna Glaze, PhD (she/her)

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

[Dave Hansen]

On 1/23/24 09:51, Kuppuswamy Sathyanarayanan wrote:
> Don't you think we should wait till there are no users before considering
> removing it?

If an overwhelming majority of the folks to which Intel has provided
TDX-capable hardware say that they are not using and will never use the
ioctl() interface, we should definitely consider removing it.

How many kernels did this ioctl() show up in, btw?

Ideally, we'd get a warning into all those kernels.

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

[Kuppuswamy Sathyanarayanan]

On 1/23/24 11:23 AM, Dave Hansen wrote:
> On 1/23/24 09:51, Kuppuswamy Sathyanarayanan wrote:
>> Don't you think we should wait till there are no users before considering
>> removing it?
> If an overwhelming majority of the folks to which Intel has provided
> TDX-capable hardware say that they are not using and will never use the
> ioctl() interface, we should definitely consider removing it.

Got it.

> How many kernels did this ioctl() show up in, btw?

It got merged in v6.2. It should be part of LTS and regular kernel releases >= v6.2

Redhat and ubuntu also merged it in their LTS kernels.

>
> Ideally, we'd get a warning into all those kernels.
>

Agree.

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer

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

[Dan Middleton]

On 1/23/24 10: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.
>
There maybe some terminology collisions here.

The getQuote ioctl was never upstreamed. IIRC it died in patch v3.

This getReport ioctl predated it and has related but separate usages to the
quote.

The configfs-tsm API uses the word report, but that really means quote or
attestation.

In intel products and maybe elsewhere the attestation terminology is:
report: MAC'd evidence ~= local attestation
quote: Signed evidence ~= remote attestation

A report can be used for local attestation because the MAC is verifiable on
that machine by that hardware.
A quote can be remotely verified using the digital signature.

Other uses for a report include secure communication with service TDs.

Regards,
Dan Middleton