| Message ID | 20230413034108.1902712-4-sathyanarayanan.kuppuswamy@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | TDX Guest Quote generation support | expand |
On Wed, Apr 12, 2023 at 8:42 PM Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> wrote: > > In TDX guest, the second stage of the attestation process is Quote > generation. This process is required to convert the locally generated > TDREPORT into a remotely verifiable Quote. It involves sending the > TDREPORT data to a Quoting Enclave (QE) which will verify the > integerity of the TDREPORT and sign it with an attestation key. nit: integrity > > Intel's TDX attestation driver exposes TDX_CMD_GET_QUOTE IOCTL to > allow user agent get the TD Quote. nit: to get > > Add a kernel selftest module to verify the Quote generation feature. > > TD Quote generation involves following steps: > > * Get the TDREPORT data using TDX_CMD_GET_REPORT IOCTL. > * Embed the TDREPORT data in quote buffer and request for quote > generation via TDX_CMD_GET_QUOTE IOCTL request. > * Upon completion of the GetQuote request, check for non zero value > in the status field of Quote header to make sure the generated > quote is valid. > > Reviewed-by: Tony Luck <tony.luck@intel.com> > Reviewed-by: Andi Kleen <ak@linux.intel.com> > Reviewed-by: Shuah Khan <skhan@linuxfoundation.org> > Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com> > Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> > Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> > --- > tools/testing/selftests/tdx/tdx_guest_test.c | 68 ++++++++++++++++++-- > 1 file changed, 62 insertions(+), 6 deletions(-) > > diff --git a/tools/testing/selftests/tdx/tdx_guest_test.c b/tools/testing/selftests/tdx/tdx_guest_test.c > index 81d8cb88ea1a..2eccde54185b 100644 > --- a/tools/testing/selftests/tdx/tdx_guest_test.c > +++ b/tools/testing/selftests/tdx/tdx_guest_test.c > @@ -18,6 +18,7 @@ > #define TDX_GUEST_DEVNAME "/dev/tdx_guest" > #define HEX_DUMP_SIZE 8 > #define DEBUG 0 > +#define QUOTE_SIZE 8192 > > /** > * struct tdreport_type - Type header of TDREPORT_STRUCT. > @@ -128,21 +129,29 @@ static void print_array_hex(const char *title, const char *prefix_str, > printf("\n"); > } > > +/* Helper function to get TDREPORT */ > +long get_tdreport0(int devfd, struct tdx_report_req *req) > +{ > + int i; > + > + /* Generate sample report data */ > + for (i = 0; i < TDX_REPORTDATA_LEN; i++) > + req->reportdata[i] = i; > + Shouldn't req be zeroed before populating reportdata? We wouldn't want uninitialized memory to leave the guest. I know this is just a test, but it's best to model good practices for anyone that might copy/paste. > + return ioctl(devfd, TDX_CMD_GET_REPORT0, req); > +} > + > TEST(verify_report) > { > struct tdx_report_req req; > struct tdreport *tdreport; > - int devfd, i; > + int devfd; > > devfd = open(TDX_GUEST_DEVNAME, O_RDWR | O_SYNC); > ASSERT_LT(0, devfd); > > - /* Generate sample report data */ > - for (i = 0; i < TDX_REPORTDATA_LEN; i++) > - req.reportdata[i] = i; > - > /* Get TDREPORT */ > - ASSERT_EQ(0, ioctl(devfd, TDX_CMD_GET_REPORT0, &req)); > + ASSERT_EQ(0, get_tdreport0(devfd, &req)); > > if (DEBUG) { > print_array_hex("\n\t\tTDX report data\n", "", > @@ -160,4 +169,51 @@ TEST(verify_report) > ASSERT_EQ(0, close(devfd)); > } > > +TEST(verify_quote) > +{ > + struct tdx_quote_hdr *quote_hdr; > + struct tdx_report_req rep_req; > + struct tdx_quote_req req; > + __u64 quote_buf_size; > + __u8 *quote_buf; > + int devfd; > + > + /* Open attestation device */ > + devfd = open(TDX_GUEST_DEVNAME, O_RDWR | O_SYNC); > + > + ASSERT_LT(0, devfd); > + > + /* Add size for quote header */ > + quote_buf_size = sizeof(*quote_hdr) + QUOTE_SIZE; > + > + /* Allocate quote buffer */ > + quote_buf = malloc(quote_buf_size); > + ASSERT_NE(NULL, quote_buf); > + > + quote_hdr = (struct tdx_quote_hdr *)quote_buf; > + > + /* Initialize GetQuote header */ > + quote_hdr->version = 1; > + quote_hdr->status = GET_QUOTE_SUCCESS; > + quote_hdr->in_len = TDX_REPORT_LEN; > + quote_hdr->out_len = 0; > + > + /* Get TDREPORT data */ > + ASSERT_EQ(0, get_tdreport0(devfd, &rep_req)); > + > + /* Fill GetQuote request */ > + memcpy(quote_hdr->data, rep_req.tdreport, TDX_REPORT_LEN); > + req.buf = (__u64)quote_buf; > + req.len = quote_buf_size; > + > + ASSERT_EQ(0, ioctl(devfd, TDX_CMD_GET_QUOTE, &req)); > + > + /* Check whether GetQuote request is successful */ > + EXPECT_EQ(0, quote_hdr->status); > + > + free(quote_buf); > + > + ASSERT_EQ(0, close(devfd)); > +} > + > TEST_HARNESS_MAIN > -- > 2.34.1 >
Hi, On 4/26/23 8:47 AM, Dionna Amalie Glaze wrote: > On Wed, Apr 12, 2023 at 8:42 PM Kuppuswamy Sathyanarayanan > <sathyanarayanan.kuppuswamy@linux.intel.com> wrote: >> >> In TDX guest, the second stage of the attestation process is Quote >> generation. This process is required to convert the locally generated >> TDREPORT into a remotely verifiable Quote. It involves sending the >> TDREPORT data to a Quoting Enclave (QE) which will verify the >> integerity of the TDREPORT and sign it with an attestation key. > > nit: integrity > >> >> Intel's TDX attestation driver exposes TDX_CMD_GET_QUOTE IOCTL to >> allow user agent get the TD Quote. > > nit: to get >> >> Add a kernel selftest module to verify the Quote generation feature. >> >> TD Quote generation involves following steps: >> >> * Get the TDREPORT data using TDX_CMD_GET_REPORT IOCTL. >> * Embed the TDREPORT data in quote buffer and request for quote >> generation via TDX_CMD_GET_QUOTE IOCTL request. >> * Upon completion of the GetQuote request, check for non zero value >> in the status field of Quote header to make sure the generated >> quote is valid. >> >> Reviewed-by: Tony Luck <tony.luck@intel.com> >> Reviewed-by: Andi Kleen <ak@linux.intel.com> >> Reviewed-by: Shuah Khan <skhan@linuxfoundation.org> >> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com> >> Acked-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com> >> Signed-off-by: Kuppuswamy Sathyanarayanan <sathyanarayanan.kuppuswamy@linux.intel.com> >> --- >> tools/testing/selftests/tdx/tdx_guest_test.c | 68 ++++++++++++++++++-- >> 1 file changed, 62 insertions(+), 6 deletions(-) >> >> diff --git a/tools/testing/selftests/tdx/tdx_guest_test.c b/tools/testing/selftests/tdx/tdx_guest_test.c >> index 81d8cb88ea1a..2eccde54185b 100644 >> --- a/tools/testing/selftests/tdx/tdx_guest_test.c >> +++ b/tools/testing/selftests/tdx/tdx_guest_test.c >> @@ -18,6 +18,7 @@ >> #define TDX_GUEST_DEVNAME "/dev/tdx_guest" >> #define HEX_DUMP_SIZE 8 >> #define DEBUG 0 >> +#define QUOTE_SIZE 8192 >> >> /** >> * struct tdreport_type - Type header of TDREPORT_STRUCT. >> @@ -128,21 +129,29 @@ static void print_array_hex(const char *title, const char *prefix_str, >> printf("\n"); >> } >> >> +/* Helper function to get TDREPORT */ >> +long get_tdreport0(int devfd, struct tdx_report_req *req) >> +{ >> + int i; >> + >> + /* Generate sample report data */ >> + for (i = 0; i < TDX_REPORTDATA_LEN; i++) >> + req->reportdata[i] = i; >> + > > Shouldn't req be zeroed before populating reportdata? We wouldn't want > uninitialized memory to leave the guest. I know this is just a test, There are only two members in struct tdx_report_req (reportdata and tdreport). The reportdata has already been updated here, and the tdreport will be updated by the kernel on output. Since TDX_CMD_GET_REPORT0 IOCTL handler uses an intermediate kernel buffer to the TDREPORT and copies the generated report back to this user buffer, this uninitialized tdreport data never leaves the guest. IMO, we don't need to zero it. However, because it is harmless, I am fine with including it. > but it's best to model good practices for anyone that might > copy/paste. > >> + return ioctl(devfd, TDX_CMD_GET_REPORT0, req); >> +} >> + >> TEST(verify_report) >> { >> struct tdx_report_req req; >> struct tdreport *tdreport; >> - int devfd, i; >> + int devfd; >> >> devfd = open(TDX_GUEST_DEVNAME, O_RDWR | O_SYNC); >> ASSERT_LT(0, devfd); >> >> - /* Generate sample report data */ >> - for (i = 0; i < TDX_REPORTDATA_LEN; i++) >> - req.reportdata[i] = i; >> - >> /* Get TDREPORT */ >> - ASSERT_EQ(0, ioctl(devfd, TDX_CMD_GET_REPORT0, &req)); >> + ASSERT_EQ(0, get_tdreport0(devfd, &req)); >> >> if (DEBUG) { >> print_array_hex("\n\t\tTDX report data\n", "", >> @@ -160,4 +169,51 @@ TEST(verify_report) >> ASSERT_EQ(0, close(devfd)); >> } >> >> +TEST(verify_quote) >> +{ >> + struct tdx_quote_hdr *quote_hdr; >> + struct tdx_report_req rep_req; >> + struct tdx_quote_req req; >> + __u64 quote_buf_size; >> + __u8 *quote_buf; >> + int devfd; >> + >> + /* Open attestation device */ >> + devfd = open(TDX_GUEST_DEVNAME, O_RDWR | O_SYNC); >> + >> + ASSERT_LT(0, devfd); >> + >> + /* Add size for quote header */ >> + quote_buf_size = sizeof(*quote_hdr) + QUOTE_SIZE; >> + >> + /* Allocate quote buffer */ >> + quote_buf = malloc(quote_buf_size); >> + ASSERT_NE(NULL, quote_buf); >> + >> + quote_hdr = (struct tdx_quote_hdr *)quote_buf; >> + >> + /* Initialize GetQuote header */ >> + quote_hdr->version = 1; >> + quote_hdr->status = GET_QUOTE_SUCCESS; >> + quote_hdr->in_len = TDX_REPORT_LEN; >> + quote_hdr->out_len = 0; >> + >> + /* Get TDREPORT data */ >> + ASSERT_EQ(0, get_tdreport0(devfd, &rep_req)); >> + >> + /* Fill GetQuote request */ >> + memcpy(quote_hdr->data, rep_req.tdreport, TDX_REPORT_LEN); >> + req.buf = (__u64)quote_buf; >> + req.len = quote_buf_size; >> + >> + ASSERT_EQ(0, ioctl(devfd, TDX_CMD_GET_QUOTE, &req)); >> + >> + /* Check whether GetQuote request is successful */ >> + EXPECT_EQ(0, quote_hdr->status); >> + >> + free(quote_buf); >> + >> + ASSERT_EQ(0, close(devfd)); >> +} >> + >> TEST_HARNESS_MAIN >> -- >> 2.34.1 >> > >
On 4/26/23 08:47, Dionna Amalie Glaze wrote: >> +/* Helper function to get TDREPORT */ >> +long get_tdreport0(int devfd, struct tdx_report_req *req) >> +{ >> + int i; >> + >> + /* Generate sample report data */ >> + for (i = 0; i < TDX_REPORTDATA_LEN; i++) >> + req->reportdata[i] = i; >> + > Shouldn't req be zeroed before populating reportdata? We wouldn't want > uninitialized memory to leave the guest. I know this is just a test, > but it's best to model good practices for anyone that might > copy/paste. It's leaving the guest and going to the TDX module. What's the problem there?
On 4/27/23 12:10, Sathyanarayanan Kuppuswamy wrote: >> Shouldn't req be zeroed before populating reportdata? We wouldn't want >> uninitialized memory to leave the guest. I know this is just a test, > There are only two members in struct tdx_report_req (reportdata and tdreport). > The reportdata has already been updated here, and the tdreport will be updated > by the kernel on output. Since TDX_CMD_GET_REPORT0 IOCTL handler uses an > intermediate kernel buffer to the TDREPORT and copies the generated report back > to this user buffer, this uninitialized tdreport data never leaves the guest. Is that really even relevant? I mean, we could implement the whole thing with get_user_pages() and then just pass the physical address of the reportdata and tdreport down to the TDX module. It doesn't matter either way. The data is going from guest userspace to the guest kernel to the TDX module, all of which are trusted. It's a selftest. I'd just leave it alone.
diff --git a/tools/testing/selftests/tdx/tdx_guest_test.c b/tools/testing/selftests/tdx/tdx_guest_test.c index 81d8cb88ea1a..2eccde54185b 100644 --- a/tools/testing/selftests/tdx/tdx_guest_test.c +++ b/tools/testing/selftests/tdx/tdx_guest_test.c @@ -18,6 +18,7 @@ #define TDX_GUEST_DEVNAME "/dev/tdx_guest" #define HEX_DUMP_SIZE 8 #define DEBUG 0 +#define QUOTE_SIZE 8192 /** * struct tdreport_type - Type header of TDREPORT_STRUCT. @@ -128,21 +129,29 @@ static void print_array_hex(const char *title, const char *prefix_str, printf("\n"); } +/* Helper function to get TDREPORT */ +long get_tdreport0(int devfd, struct tdx_report_req *req) +{ + int i; + + /* Generate sample report data */ + for (i = 0; i < TDX_REPORTDATA_LEN; i++) + req->reportdata[i] = i; + + return ioctl(devfd, TDX_CMD_GET_REPORT0, req); +} + TEST(verify_report) { struct tdx_report_req req; struct tdreport *tdreport; - int devfd, i; + int devfd; devfd = open(TDX_GUEST_DEVNAME, O_RDWR | O_SYNC); ASSERT_LT(0, devfd); - /* Generate sample report data */ - for (i = 0; i < TDX_REPORTDATA_LEN; i++) - req.reportdata[i] = i; - /* Get TDREPORT */ - ASSERT_EQ(0, ioctl(devfd, TDX_CMD_GET_REPORT0, &req)); + ASSERT_EQ(0, get_tdreport0(devfd, &req)); if (DEBUG) { print_array_hex("\n\t\tTDX report data\n", "", @@ -160,4 +169,51 @@ TEST(verify_report) ASSERT_EQ(0, close(devfd)); } +TEST(verify_quote) +{ + struct tdx_quote_hdr *quote_hdr; + struct tdx_report_req rep_req; + struct tdx_quote_req req; + __u64 quote_buf_size; + __u8 *quote_buf; + int devfd; + + /* Open attestation device */ + devfd = open(TDX_GUEST_DEVNAME, O_RDWR | O_SYNC); + + ASSERT_LT(0, devfd); + + /* Add size for quote header */ + quote_buf_size = sizeof(*quote_hdr) + QUOTE_SIZE; + + /* Allocate quote buffer */ + quote_buf = malloc(quote_buf_size); + ASSERT_NE(NULL, quote_buf); + + quote_hdr = (struct tdx_quote_hdr *)quote_buf; + + /* Initialize GetQuote header */ + quote_hdr->version = 1; + quote_hdr->status = GET_QUOTE_SUCCESS; + quote_hdr->in_len = TDX_REPORT_LEN; + quote_hdr->out_len = 0; + + /* Get TDREPORT data */ + ASSERT_EQ(0, get_tdreport0(devfd, &rep_req)); + + /* Fill GetQuote request */ + memcpy(quote_hdr->data, rep_req.tdreport, TDX_REPORT_LEN); + req.buf = (__u64)quote_buf; + req.len = quote_buf_size; + + ASSERT_EQ(0, ioctl(devfd, TDX_CMD_GET_QUOTE, &req)); + + /* Check whether GetQuote request is successful */ + EXPECT_EQ(0, quote_hdr->status); + + free(quote_buf); + + ASSERT_EQ(0, close(devfd)); +} + TEST_HARNESS_MAIN