| Message ID | 20220928215535.26527-2-sathyanarayanan.kuppuswamy@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | Add TDX Guest Attestation support | expand |
On Wed, Sep 28, 2022 at 02:55:33PM -0700, Kuppuswamy Sathyanarayanan wrote: > To support TDX attestation, the TDX guest user interface driver must > use the __tdx module_call() function in the driver to allow the user to > obtain the TDREPORT. > > So export the __tdx_module_call() and move the TDX Module IDs to > asm/tdx.h. The functions with the __ prefix are usually lower-level interfaces which should be internal. Usually. Why aren't you exporting the tdx_module_call() one instead?
On 10/12/22 3:18 AM, Borislav Petkov wrote: > On Wed, Sep 28, 2022 at 02:55:33PM -0700, Kuppuswamy Sathyanarayanan wrote: >> To support TDX attestation, the TDX guest user interface driver must >> use the __tdx module_call() function in the driver to allow the user to >> obtain the TDREPORT. >> >> So export the __tdx_module_call() and move the TDX Module IDs to >> asm/tdx.h. > > The functions with the __ prefix are usually lower-level interfaces > which should be internal. Usually. > > Why aren't you exporting the tdx_module_call() one instead? tdx_module_call() calls panic() on a non-zero error value. So it is only used for cases where failure is fatal to the guest. But in the case of TDG.MR.REPORT TDCALL, there are valid cases for failure (like invalid param or busy condition) and the failure is non-fatal. So we should create a new wrapper for this use case or use __tdx_module_call() which is already exposed in asm/tdx.h. >
On Wed, Oct 12, 2022 at 06:35:56AM -0700, Sathyanarayanan Kuppuswamy wrote:
> So we should create a new wrapper for this use case or use
Yes, you got it - a new wrapper pls.
Thx.
On 10/12/22 7:27 AM, Borislav Petkov wrote: > On Wed, Oct 12, 2022 at 06:35:56AM -0700, Sathyanarayanan Kuppuswamy wrote: >> So we should create a new wrapper for this use case or use > > Yes, you got it - a new wrapper pls. Ok. I will add a new wrapper to get the TDREPORT. +/* + * Add a wrapper for TDG.MR.REPORT TDCALL. It is used in TDX guest + * driver module to get the TDREPORT. + */ +long tdx_mcall_get_report(void *reportdata, void *tdreport, u8 subtype) +{ + if (subtype || !reportdata || !tdreport) + return -EINVAL; + + /* + * Generate TDREPORT using "TDG.MR.REPORT" TDCALL. + * + * Get the TDREPORT using REPORTDATA as input. Refer to + * section 22.3.3 TDG.MR.REPORT leaf in the TDX Module 1.0 + * specification for detailed information. + */ + return __tdx_module_call(TDX_GET_REPORT, virt_to_phys(tdreport), + virt_to_phys(reportdata), subtype, 0, NULL); +} +EXPORT_SYMBOL_GPL(tdx_mcall_get_report); > > Thx. >
On Wed, Oct 12, 2022 at 08:44:04AM -0700, Sathyanarayanan Kuppuswamy wrote: > > > On 10/12/22 7:27 AM, Borislav Petkov wrote: > > On Wed, Oct 12, 2022 at 06:35:56AM -0700, Sathyanarayanan Kuppuswamy wrote: > >> So we should create a new wrapper for this use case or use > > > > Yes, you got it - a new wrapper pls. > > Ok. I will add a new wrapper to get the TDREPORT. > > +/* > > + * Add a wrapper for TDG.MR.REPORT TDCALL. It is used in TDX guest > > + * driver module to get the TDREPORT. > > + */ > > +long tdx_mcall_get_report(void *reportdata, void *tdreport, u8 subtype) Why "long"? Why void *? Don't you have real types for these? > > +{ > > + if (subtype || !reportdata || !tdreport) > > + return -EINVAL; How could that happen if you control all callers? > > + > > + /* > > + * Generate TDREPORT using "TDG.MR.REPORT" TDCALL. > > + * > > + * Get the TDREPORT using REPORTDATA as input. Refer to > > + * section 22.3.3 TDG.MR.REPORT leaf in the TDX Module 1.0 > > + * specification for detailed information. > > + */ > > + return __tdx_module_call(TDX_GET_REPORT, virt_to_phys(tdreport), > > + virt_to_phys(reportdata), subtype, 0, NULL); If you check for NULL, why are you not validating that these are valid pointers as well? You can't have it both ways. thanks, greg k-h
Hi, On 10/12/22 9:23 AM, Greg Kroah-Hartman wrote: > On Wed, Oct 12, 2022 at 08:44:04AM -0700, Sathyanarayanan Kuppuswamy wrote: >> >> >> On 10/12/22 7:27 AM, Borislav Petkov wrote: >>> On Wed, Oct 12, 2022 at 06:35:56AM -0700, Sathyanarayanan Kuppuswamy wrote: >>>> So we should create a new wrapper for this use case or use >>> >>> Yes, you got it - a new wrapper pls. >> >> Ok. I will add a new wrapper to get the TDREPORT. >> >> +/* >> >> + * Add a wrapper for TDG.MR.REPORT TDCALL. It is used in TDX guest >> >> + * driver module to get the TDREPORT. >> >> + */ >> >> +long tdx_mcall_get_report(void *reportdata, void *tdreport, u8 subtype) > > Why "long"? We used long because __tdx_module_call() call returns u64 value. Alternatively, we can also check for return value of __tdx_module_call() here and return 0/-EIO as return values. In this case we can change return value to int. > > Why void *? Don't you have real types for these? We use these buffers as an intermediary to transfer data between userspace and the TDX module. In the kernel we don't consume these datas. So we did not define the type of the data. > > > >> >> +{ >> >> + if (subtype || !reportdata || !tdreport) >> >> + return -EINVAL; > > How could that happen if you control all callers? I have added it as a safety check against any incorrect usage in future. I will remove it. > > thanks, > > greg k-h
On Wed, Oct 12, 2022 at 10:13:50AM -0700, Sathyanarayanan Kuppuswamy wrote: > Hi, > > On 10/12/22 9:23 AM, Greg Kroah-Hartman wrote: > > On Wed, Oct 12, 2022 at 08:44:04AM -0700, Sathyanarayanan Kuppuswamy wrote: > >> > >> > >> On 10/12/22 7:27 AM, Borislav Petkov wrote: > >>> On Wed, Oct 12, 2022 at 06:35:56AM -0700, Sathyanarayanan Kuppuswamy wrote: > >>>> So we should create a new wrapper for this use case or use > >>> > >>> Yes, you got it - a new wrapper pls. > >> > >> Ok. I will add a new wrapper to get the TDREPORT. > >> > >> +/* > >> > >> + * Add a wrapper for TDG.MR.REPORT TDCALL. It is used in TDX guest > >> > >> + * driver module to get the TDREPORT. > >> > >> + */ > >> > >> +long tdx_mcall_get_report(void *reportdata, void *tdreport, u8 subtype) > > > > Why "long"? > > We used long because __tdx_module_call() call returns u64 value. Great, then use u64 please. Or if you are returning negative errors, use s64 to be specific. > Alternatively, we can also check for return value of __tdx_module_call() here > and return 0/-EIO as return values. In this case we can change return value > to int. That would make more sense, right? > > > > Why void *? Don't you have real types for these? > > We use these buffers as an intermediary to transfer data between userspace and > the TDX module. In the kernel we don't consume these datas. So we did not define > the type of the data. Then these are userspace pointers? Why are they not marked as such? thanks, greg k-h
On 10/12/22 10:26 AM, Greg Kroah-Hartman wrote: > On Wed, Oct 12, 2022 at 10:13:50AM -0700, Sathyanarayanan Kuppuswamy wrote: >> Hi, >> >> On 10/12/22 9:23 AM, Greg Kroah-Hartman wrote: >>> On Wed, Oct 12, 2022 at 08:44:04AM -0700, Sathyanarayanan Kuppuswamy wrote: >>>> >>>> >>>> On 10/12/22 7:27 AM, Borislav Petkov wrote: >>>>> On Wed, Oct 12, 2022 at 06:35:56AM -0700, Sathyanarayanan Kuppuswamy wrote: >>>>>> So we should create a new wrapper for this use case or use >>>>> >>>>> Yes, you got it - a new wrapper pls. >>>> >>>> Ok. I will add a new wrapper to get the TDREPORT. >>>> >>>> +/* >>>> >>>> + * Add a wrapper for TDG.MR.REPORT TDCALL. It is used in TDX guest >>>> >>>> + * driver module to get the TDREPORT. >>>> >>>> + */ >>>> >>>> +long tdx_mcall_get_report(void *reportdata, void *tdreport, u8 subtype) >>> >>> Why "long"? >> >> We used long because __tdx_module_call() call returns u64 value. > > Great, then use u64 please. Or if you are returning negative errors, > use s64 to be specific. > >> Alternatively, we can also check for return value of __tdx_module_call() here >> and return 0/-EIO as return values. In this case we can change return value >> to int. > > That would make more sense, right? Yes. I will change it as mentioned above. > >>> >>> Why void *? Don't you have real types for these? >> >> We use these buffers as an intermediary to transfer data between userspace and >> the TDX module. In the kernel we don't consume these datas. So we did not define >> the type of the data. > > Then these are userspace pointers? Why are they not marked as such? They are not userspace pointers. Since we need to pass physical addresses of reportdata and tdreport buffers to the TDX Module, we cannot directly use userspace pointers. So we allocate these intermediary buffers in the TDX guest driver and use it to copy the data from/to user pointers. > > thanks, > > greg k-h
diff --git a/arch/x86/coco/tdx/tdcall.S b/arch/x86/coco/tdx/tdcall.S index f9eb1134f22d..47a1534946c8 100644 --- a/arch/x86/coco/tdx/tdcall.S +++ b/arch/x86/coco/tdx/tdcall.S @@ -3,6 +3,7 @@ #include <asm/asm.h> #include <asm/frame.h> #include <asm/unwind_hints.h> +#include <asm/export.h> #include <linux/linkage.h> #include <linux/bits.h> @@ -75,6 +76,7 @@ SYM_FUNC_START(__tdx_module_call) FRAME_END RET SYM_FUNC_END(__tdx_module_call) +EXPORT_SYMBOL_GPL(__tdx_module_call); /* * __tdx_hypercall() - Make hypercalls to a TDX VMM using TDVMCALL leaf diff --git a/arch/x86/coco/tdx/tdx.c b/arch/x86/coco/tdx/tdx.c index 928dcf7a20d9..2dcc6021aa43 100644 --- a/arch/x86/coco/tdx/tdx.c +++ b/arch/x86/coco/tdx/tdx.c @@ -12,11 +12,6 @@ #include <asm/insn-eval.h> #include <asm/pgtable.h> -/* TDX module Call Leaf IDs */ -#define TDX_GET_INFO 1 -#define TDX_GET_VEINFO 3 -#define TDX_ACCEPT_PAGE 6 - /* TDX hypercall Leaf IDs */ #define TDVMCALL_MAP_GPA 0x10001 diff --git a/arch/x86/include/asm/tdx.h b/arch/x86/include/asm/tdx.h index 020c81a7c729..34c00d8a5263 100644 --- a/arch/x86/include/asm/tdx.h +++ b/arch/x86/include/asm/tdx.h @@ -18,6 +18,11 @@ #define TDX_SW_ERROR (TDX_ERROR | GENMASK_ULL(47, 40)) #define TDX_SEAMCALL_VMFAILINVALID (TDX_SW_ERROR | _UL(0xFFFF0000)) +/* TDX module Call Leaf IDs */ +#define TDX_GET_INFO 1 +#define TDX_GET_VEINFO 3 +#define TDX_ACCEPT_PAGE 6 + #ifndef __ASSEMBLY__ /*