mbox series

[SRU,K,0/1] new TDX attestation driver from Intel

Message ID 20230306092058.26718-1-andrea.righi@canonical.com
Headers show
Series new TDX attestation driver from Intel | expand

Message

Andrea Righi March 6, 2023, 9:20 a.m. UTC 
BugLink: https://bugs.launchpad.net/bugs/2009437

[Impact]

TDX guest attestation has been merged as SAUCE patches in the kinetic
kernel with the following commits:

https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/kinetic/commit/?h=master-next&id=285d6d8136ebadcee7fd6452b9e4223996a2a0af
https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/kinetic/commit/?h=master-next&id=0b78a71c7d7630ab7c3c8a03cbe4f78f1361fb45

However, Intel released a new TDX attestation driver that will be
submitted upstream. We should align with the new version that will
likely end upstream.

See also LP: #1971027

[Test case]

Testing this feature requires a special hardware in the host, special
firmware and special configuration of a guest.

Right now it can only be tested by Intel.

[Fix]

Apply the new driver provided by Intel in LP: #1971027.

[Regression potential]

The new driver can potentially break user-space applications that are
relying on the TDX attestation feature. This is because of this struct
(used in the user-space/kernel communication, via ioctl):

+ * Used in TDX_CMD_GET_REPORT IOCTL request.
+ */
+struct tdx_report_req {
+ __u8 subtype;
+ __u64 reportdata;
+ __u32 rpd_len;
+ __u64 tdreport;
+ __u32 tdr_len;
+};

The new patch changed the struct as following:

+struct tdx_report_req {
+ __u8 reportdata[TDX_REPORTDATA_LEN];
+ __u8 tdreport[TDX_REPORT_LEN];
+};

In general we should never apply changes that are breaking user-space
like this (especially for non-devel kernels), but realistically we can
probably say that nobody is using this feature yet, so nobody has any
user-space program that is relying on the old struct (and if they do,
they're probably in touch with Intel, so they're aware of this change).

In conclusion, this change should be considered pretty safe, despite the
potential user-space brekage.

----------------------------------------------------------------
Andrea Righi (3):
      Revert "UBUNTU: SAUCE: selftests: tdx: Test GetReport TDX attestation feature"
      Revert "UBUNTU: SAUCE: x86/tdx: Add TDX Guest attestation interface driver"
      UBUNTU: [Config] enable TDX attestation driver as module by default

Kuppuswamy Sathyanarayanan (3):
      UBUNTU: SAUCE: x86/tdx: Add a wrapper to get TDREPORT0 from the TDX Module
      UBUNTU: SAUCE: virt: Add TDX guest driver
      UBUNTU: SAUCE: selftests/tdx: Test TDX attestation GetReport support

 Documentation/virt/coco/tdx-guest.rst         |  52 ++++++++
 Documentation/virt/index.rst                  |   1 +
 Documentation/x86/tdx.rst                     |  43 +++++++
 arch/x86/coco/tdx/tdx.c                       | 151 ++++++------------------
 arch/x86/include/asm/tdx.h                    |   2 +
 arch/x86/include/uapi/asm/tdx.h               |  51 --------
 debian.master/config/annotations              |   3 +
 debian.master/config/config.common.ubuntu     |   1 +
 drivers/virt/Kconfig                          |   2 +
 drivers/virt/Makefile                         |   1 +
 drivers/virt/coco/tdx-guest/Kconfig           |  10 ++
 drivers/virt/coco/tdx-guest/Makefile          |   2 +
 drivers/virt/coco/tdx-guest/tdx-guest.c       | 102 ++++++++++++++++
 include/uapi/linux/tdx-guest.h                |  42 +++++++
 tools/arch/x86/include/uapi/asm/tdx.h         |  51 --------
 tools/testing/selftests/tdx/Makefile          |   8 +-
 tools/testing/selftests/tdx/config            |   2 +-
 tools/testing/selftests/tdx/tdx_attest_test.c | 156 ------------------------
 tools/testing/selftests/tdx/tdx_guest_test.c  | 163 ++++++++++++++++++++++++++
 19 files changed, 464 insertions(+), 379 deletions(-)
 create mode 100644 Documentation/virt/coco/tdx-guest.rst
 delete mode 100644 arch/x86/include/uapi/asm/tdx.h
 create mode 100644 drivers/virt/coco/tdx-guest/Kconfig
 create mode 100644 drivers/virt/coco/tdx-guest/Makefile
 create mode 100644 drivers/virt/coco/tdx-guest/tdx-guest.c
 create mode 100644 include/uapi/linux/tdx-guest.h
 delete mode 100644 tools/arch/x86/include/uapi/asm/tdx.h
 delete mode 100644 tools/testing/selftests/tdx/tdx_attest_test.c
 create mode 100644 tools/testing/selftests/tdx/tdx_guest_test.c

Comments

Tim Gardner March 6, 2023, 2:41 p.m. UTC | #1 
On 3/6/23 2:20 AM, Andrea Righi wrote:
> BugLink: https://bugs.launchpad.net/bugs/2009437
> 
> [Impact]
> 
> TDX guest attestation has been merged as SAUCE patches in the kinetic
> kernel with the following commits:
> 
> https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/kinetic/commit/?h=master-next&id=285d6d8136ebadcee7fd6452b9e4223996a2a0af
> https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/kinetic/commit/?h=master-next&id=0b78a71c7d7630ab7c3c8a03cbe4f78f1361fb45
> 
> However, Intel released a new TDX attestation driver that will be
> submitted upstream. We should align with the new version that will
> likely end upstream.
> 
> See also LP: #1971027
> 
> [Test case]
> 
> Testing this feature requires a special hardware in the host, special
> firmware and special configuration of a guest.
> 
> Right now it can only be tested by Intel.
> 
> [Fix]
> 
> Apply the new driver provided by Intel in LP: #1971027.
> 
> [Regression potential]
> 
> The new driver can potentially break user-space applications that are
> relying on the TDX attestation feature. This is because of this struct
> (used in the user-space/kernel communication, via ioctl):
> 
> + * Used in TDX_CMD_GET_REPORT IOCTL request.
> + */
> +struct tdx_report_req {
> + __u8 subtype;
> + __u64 reportdata;
> + __u32 rpd_len;
> + __u64 tdreport;
> + __u32 tdr_len;
> +};
> 
> The new patch changed the struct as following:
> 
> +struct tdx_report_req {
> + __u8 reportdata[TDX_REPORTDATA_LEN];
> + __u8 tdreport[TDX_REPORT_LEN];
> +};
> 
> In general we should never apply changes that are breaking user-space
> like this (especially for non-devel kernels), but realistically we can
> probably say that nobody is using this feature yet, so nobody has any
> user-space program that is relying on the old struct (and if they do,
> they're probably in touch with Intel, so they're aware of this change).
> 
> In conclusion, this change should be considered pretty safe, despite the
> potential user-space brekage.
> 
> ----------------------------------------------------------------
> Andrea Righi (3):
>        Revert "UBUNTU: SAUCE: selftests: tdx: Test GetReport TDX attestation feature"
>        Revert "UBUNTU: SAUCE: x86/tdx: Add TDX Guest attestation interface driver"
>        UBUNTU: [Config] enable TDX attestation driver as module by default
> 
> Kuppuswamy Sathyanarayanan (3):
>        UBUNTU: SAUCE: x86/tdx: Add a wrapper to get TDREPORT0 from the TDX Module
>        UBUNTU: SAUCE: virt: Add TDX guest driver
>        UBUNTU: SAUCE: selftests/tdx: Test TDX attestation GetReport support
> 
>   Documentation/virt/coco/tdx-guest.rst         |  52 ++++++++
>   Documentation/virt/index.rst                  |   1 +
>   Documentation/x86/tdx.rst                     |  43 +++++++
>   arch/x86/coco/tdx/tdx.c                       | 151 ++++++------------------
>   arch/x86/include/asm/tdx.h                    |   2 +
>   arch/x86/include/uapi/asm/tdx.h               |  51 --------
>   debian.master/config/annotations              |   3 +
>   debian.master/config/config.common.ubuntu     |   1 +
>   drivers/virt/Kconfig                          |   2 +
>   drivers/virt/Makefile                         |   1 +
>   drivers/virt/coco/tdx-guest/Kconfig           |  10 ++
>   drivers/virt/coco/tdx-guest/Makefile          |   2 +
>   drivers/virt/coco/tdx-guest/tdx-guest.c       | 102 ++++++++++++++++
>   include/uapi/linux/tdx-guest.h                |  42 +++++++
>   tools/arch/x86/include/uapi/asm/tdx.h         |  51 --------
>   tools/testing/selftests/tdx/Makefile          |   8 +-
>   tools/testing/selftests/tdx/config            |   2 +-
>   tools/testing/selftests/tdx/tdx_attest_test.c | 156 ------------------------
>   tools/testing/selftests/tdx/tdx_guest_test.c  | 163 ++++++++++++++++++++++++++
>   19 files changed, 464 insertions(+), 379 deletions(-)
>   create mode 100644 Documentation/virt/coco/tdx-guest.rst
>   delete mode 100644 arch/x86/include/uapi/asm/tdx.h
>   create mode 100644 drivers/virt/coco/tdx-guest/Kconfig
>   create mode 100644 drivers/virt/coco/tdx-guest/Makefile
>   create mode 100644 drivers/virt/coco/tdx-guest/tdx-guest.c
>   create mode 100644 include/uapi/linux/tdx-guest.h
>   delete mode 100644 tools/arch/x86/include/uapi/asm/tdx.h
>   delete mode 100644 tools/testing/selftests/tdx/tdx_attest_test.c
>   create mode 100644 tools/testing/selftests/tdx/tdx_guest_test.c
> 
> 

Patches 3-5 are upstream. Shouldn't they be cherry picks (or backports) ?
Andrea Righi March 6, 2023, 2:45 p.m. UTC | #2 
On Mon, Mar 06, 2023 at 07:41:20AM -0700, Tim Gardner wrote:
> On 3/6/23 2:20 AM, Andrea Righi wrote:
> > BugLink: https://bugs.launchpad.net/bugs/2009437
> > 
> > [Impact]
> > 
> > TDX guest attestation has been merged as SAUCE patches in the kinetic
> > kernel with the following commits:
> > 
> > https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/kinetic/commit/?h=master-next&id=285d6d8136ebadcee7fd6452b9e4223996a2a0af
> > https://git.launchpad.net/~ubuntu-kernel/ubuntu/+source/linux/+git/kinetic/commit/?h=master-next&id=0b78a71c7d7630ab7c3c8a03cbe4f78f1361fb45
> > 
> > However, Intel released a new TDX attestation driver that will be
> > submitted upstream. We should align with the new version that will
> > likely end upstream.
> > 
> > See also LP: #1971027
> > 
> > [Test case]
> > 
> > Testing this feature requires a special hardware in the host, special
> > firmware and special configuration of a guest.
> > 
> > Right now it can only be tested by Intel.
> > 
> > [Fix]
> > 
> > Apply the new driver provided by Intel in LP: #1971027.
> > 
> > [Regression potential]
> > 
> > The new driver can potentially break user-space applications that are
> > relying on the TDX attestation feature. This is because of this struct
> > (used in the user-space/kernel communication, via ioctl):
> > 
> > + * Used in TDX_CMD_GET_REPORT IOCTL request.
> > + */
> > +struct tdx_report_req {
> > + __u8 subtype;
> > + __u64 reportdata;
> > + __u32 rpd_len;
> > + __u64 tdreport;
> > + __u32 tdr_len;
> > +};
> > 
> > The new patch changed the struct as following:
> > 
> > +struct tdx_report_req {
> > + __u8 reportdata[TDX_REPORTDATA_LEN];
> > + __u8 tdreport[TDX_REPORT_LEN];
> > +};
> > 
> > In general we should never apply changes that are breaking user-space
> > like this (especially for non-devel kernels), but realistically we can
> > probably say that nobody is using this feature yet, so nobody has any
> > user-space program that is relying on the old struct (and if they do,
> > they're probably in touch with Intel, so they're aware of this change).
> > 
> > In conclusion, this change should be considered pretty safe, despite the
> > potential user-space brekage.
> > 
> > ----------------------------------------------------------------
> > Andrea Righi (3):
> >        Revert "UBUNTU: SAUCE: selftests: tdx: Test GetReport TDX attestation feature"
> >        Revert "UBUNTU: SAUCE: x86/tdx: Add TDX Guest attestation interface driver"
> >        UBUNTU: [Config] enable TDX attestation driver as module by default
> > 
> > Kuppuswamy Sathyanarayanan (3):
> >        UBUNTU: SAUCE: x86/tdx: Add a wrapper to get TDREPORT0 from the TDX Module
> >        UBUNTU: SAUCE: virt: Add TDX guest driver
> >        UBUNTU: SAUCE: selftests/tdx: Test TDX attestation GetReport support
> > 
> >   Documentation/virt/coco/tdx-guest.rst         |  52 ++++++++
> >   Documentation/virt/index.rst                  |   1 +
> >   Documentation/x86/tdx.rst                     |  43 +++++++
> >   arch/x86/coco/tdx/tdx.c                       | 151 ++++++------------------
> >   arch/x86/include/asm/tdx.h                    |   2 +
> >   arch/x86/include/uapi/asm/tdx.h               |  51 --------
> >   debian.master/config/annotations              |   3 +
> >   debian.master/config/config.common.ubuntu     |   1 +
> >   drivers/virt/Kconfig                          |   2 +
> >   drivers/virt/Makefile                         |   1 +
> >   drivers/virt/coco/tdx-guest/Kconfig           |  10 ++
> >   drivers/virt/coco/tdx-guest/Makefile          |   2 +
> >   drivers/virt/coco/tdx-guest/tdx-guest.c       | 102 ++++++++++++++++
> >   include/uapi/linux/tdx-guest.h                |  42 +++++++
> >   tools/arch/x86/include/uapi/asm/tdx.h         |  51 --------
> >   tools/testing/selftests/tdx/Makefile          |   8 +-
> >   tools/testing/selftests/tdx/config            |   2 +-
> >   tools/testing/selftests/tdx/tdx_attest_test.c | 156 ------------------------
> >   tools/testing/selftests/tdx/tdx_guest_test.c  | 163 ++++++++++++++++++++++++++
> >   19 files changed, 464 insertions(+), 379 deletions(-)
> >   create mode 100644 Documentation/virt/coco/tdx-guest.rst
> >   delete mode 100644 arch/x86/include/uapi/asm/tdx.h
> >   create mode 100644 drivers/virt/coco/tdx-guest/Kconfig
> >   create mode 100644 drivers/virt/coco/tdx-guest/Makefile
> >   create mode 100644 drivers/virt/coco/tdx-guest/tdx-guest.c
> >   create mode 100644 include/uapi/linux/tdx-guest.h
> >   delete mode 100644 tools/arch/x86/include/uapi/asm/tdx.h
> >   delete mode 100644 tools/testing/selftests/tdx/tdx_attest_test.c
> >   create mode 100644 tools/testing/selftests/tdx/tdx_guest_test.c
> > 
> > 
> 
> Patches 3-5 are upstream. Shouldn't they be cherry picks (or backports) ?

Definitely. Will send a v2 with the proper cherry pick / backport line.

Thanks,
-Andrea