Re: [PATCH v2 3/3] selftests/tdx: Test GetQuote TDX attestation feature

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

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

-- 
Sathyanarayanan Kuppuswamy
Linux Kernel Developer