Re: [PATCH v16 2/3] virt: Add TDX guest driver

[Sathyanarayanan Kuppuswamy]

Hi Greg

On 10/27/22 11:25 PM, Greg Kroah-Hartman wrote:
> On Thu, Oct 27, 2022 at 05:28:19PM -0700, Kuppuswamy Sathyanarayanan wrote:

>> +
>> +static long tdx_guest_ioctl(struct file *file, unsigned int cmd,
>> +			    unsigned long arg)
>> +{
>> +	switch (cmd) {
>> +	case TDX_CMD_GET_REPORT:
>> +		return tdx_get_report((void __user *)arg);
> 
> You know the type of this pointer here, why not cast it instead of
> having to cast it from void * again?

The only place we use arg pointer is in copy_from_user() function,
which expects void __user * pointer. So why cast it as struct
tdx_report_req * here?


>> +
>> +MODULE_AUTHOR("Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@xxxxxxxxxxxxxxx>");
>> +MODULE_DESCRIPTION("TDX Guest Driver");
>> +MODULE_LICENSE("GPL");
>> diff --git a/include/uapi/linux/tdx-guest.h b/include/uapi/linux/tdx-guest.h
>> new file mode 100644
>> index 000000000000..29453e6a7ced
>> --- /dev/null
>> +++ b/include/uapi/linux/tdx-guest.h
>> @@ -0,0 +1,55 @@
>> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
>> +/*
>> + * Userspace interface for TDX guest driver
>> + *
>> + * Copyright (C) 2022 Intel Corporation
>> + */
>> +
>> +#ifndef _UAPI_LINUX_TDX_GUEST_H_
>> +#define _UAPI_LINUX_TDX_GUEST_H_
>> +
>> +#include <linux/ioctl.h>
>> +#include <linux/types.h>
>> +
>> +/* Length of the REPORTDATA used in TDG.MR.REPORT TDCALL */
>> +#define TDX_REPORTDATA_LEN              64
>> +
>> +/* Length of TDREPORT used in TDG.MR.REPORT TDCALL */
>> +#define TDX_REPORT_LEN                  1024
> 
> As these are fixed values, why do you have to say them again in the
> structure?

These length recommendations are provided by the TDX Module, and there is
a slight possibility that the TDX Module will increase the maximum size
of the REPORTDATA and TDREPORT in the future. To handle such length
changes, rather than inventing a new IOCTL for it in the future, making
the current one flexible to handle such changes seems better. One less ABI
to maintain is always better, right? My initial design did use fixed size
buffers like you have recommended, but later changed it as per review
suggestion to make the ABI flexible.


> 
> If you ever change them, wonderful, make a new ioctl.  You can't change
> this one as userspace would have to also change, there is no way you
> could mix/match kernel versions and userspace and not have problems.


Removing the length based checks in the kernel (in tdx_get_report()) and
directly passing the user input to the TDX Module can also avoid the 
usespace/kernel version mix/match issues you have mentioned. Does such
a solution acceptable?

> 
> So just fix these values, like you have, and remove them from the
> structure definition as there's no way you can change them anyway.
> 

With above details, if you think it is still better to remove the length
params, I can make the change.


>> +
>> +/**
>> + * struct tdx_report_req - Request struct for TDX_CMD_GET_REPORT IOCTL.
>> + *
>> + * @reportdata: User-defined REPORTDATA to be included into TDREPORT.
>> + *              Typically it can be some nonce provided by attestation
>> + *              service, so the generated TDREPORT can be uniquely verified.
>> + * @tdreport: TDREPORT output from TDCALL[TDG.MR.REPORT].
> 
> These are userspace pointers, document them as such please.

Will following change do?

@reportdata: Address of the user buffer with user-defined REPORTDATA to be
             included into TDREPORT.
@tdreport: Address of the user buffer to store TDREPORT output from TDCALL[TDG.MR.REPORT]


> 
> thanks,
> 
> greg k-h

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer