Re: [PATCH v2 02/22] include/xen/slr-table.h: Secure Launch Resource Table definitions

[Jan Beulich <jbeulich@xxxxxxxx>]

On 13.05.2025 19:05, Sergii Dmytruk wrote:
> The file provides constants, structures and several helper functions for
> parsing SLRT.
> 
> Signed-off-by: Ross Philipson <ross.philipson@xxxxxxxxxx>
> Signed-off-by: Sergii Dmytruk <sergii.dmytruk@xxxxxxxxx>
> ---
>  xen/include/xen/slr-table.h | 268 ++++++++++++++++++++++++++++++++++++
>  1 file changed, 268 insertions(+)
>  create mode 100644 xen/include/xen/slr-table.h
> 
> --- /dev/null
> +++ b/xen/include/xen/slr-table.h
> @@ -0,0 +1,268 @@
> +/* SPDX-License-Identifier: GPL-2.0 */

GPL-2.0-only is, I think, the one to use for new code.

> +/*
> + *  Copyright (c) 2025 Apertus Solutions, LLC
> + *  Copyright (c) 2025 Oracle and/or its affiliates.
> + *  Copyright (c) 2025 3mdeb Sp. z o.o

I'm curious: Considering the (just) 2 S-o-b, where's the 3rd copyright
line coming from?

> + *  Secure Launch Resource Table definitions
> + */
> +
> +#ifndef XEN__SLR_TABLE_H
> +#define XEN__SLR_TABLE_H
> +
> +#include <xen/types.h>

Looks like xen/stdint.h would suffice?

> +#define UEFI_SLR_TABLE_GUID \
> +    { 0x877a9b2aU, 0x0385, 0x45d1, { 0xa0, 0x34, 0x9d, 0xac, 0x9c, 0x9e, 
> 0x56, 0x5f } }

I'm not sure this is a good place to put UEFI GUIDs. Considering e.g ...

> +/* SLR table header values */
> +#define SLR_TABLE_MAGIC         0x4452544d
> +#define SLR_TABLE_REVISION      1
> +
> +/* Current revisions for the policy and UEFI config */
> +#define SLR_POLICY_REVISION         1
> +#define SLR_UEFI_CONFIG_REVISION    1

... this, is the whole concept perhaps bound to UEFI? In which casethe
whole header may want to move to the efi/ subdir?

> +/* SLR defined architectures */
> +#define SLR_INTEL_TXT   1
> +#define SLR_AMD_SKINIT  2

These are both x86, yet the header is put in the common include dir?

> +/* SLR defined bootloaders */
> +#define SLR_BOOTLOADER_INVALID  0
> +#define SLR_BOOTLOADER_GRUB     1
> +
> +/* Log formats */
> +#define SLR_DRTM_TPM12_LOG      1
> +#define SLR_DRTM_TPM20_LOG      2
> +
> +/* DRTM Policy Entry Flags */
> +#define SLR_POLICY_FLAG_MEASURED    0x1
> +#define SLR_POLICY_IMPLICIT_SIZE    0x2
> +
> +/* Array Lengths */
> +#define TPM_EVENT_INFO_LENGTH       32
> +#define TXT_VARIABLE_MTRRS_LENGTH   32
> +
> +/* Tags */
> +#define SLR_ENTRY_INVALID       0x0000
> +#define SLR_ENTRY_DL_INFO       0x0001
> +#define SLR_ENTRY_LOG_INFO      0x0002
> +#define SLR_ENTRY_DRTM_POLICY   0x0003
> +#define SLR_ENTRY_INTEL_INFO    0x0004
> +#define SLR_ENTRY_AMD_INFO      0x0005
> +#define SLR_ENTRY_ARM_INFO      0x0006
> +#define SLR_ENTRY_UEFI_INFO     0x0007
> +#define SLR_ENTRY_UEFI_CONFIG   0x0008
> +#define SLR_ENTRY_END           0xffff
> +
> +/* Entity Types */
> +#define SLR_ET_UNSPECIFIED        0x0000
> +#define SLR_ET_SLRT               0x0001
> +#define SLR_ET_BOOT_PARAMS        0x0002
> +#define SLR_ET_SETUP_DATA         0x0003
> +#define SLR_ET_CMDLINE            0x0004
> +#define SLR_ET_UEFI_MEMMAP        0x0005
> +#define SLR_ET_RAMDISK            0x0006
> +#define SLR_ET_MULTIBOOT2_INFO    0x0007
> +#define SLR_ET_MULTIBOOT2_MODULE  0x0008
> +#define SLR_ET_TXT_OS2MLE         0x0010
> +#define SLR_ET_UNUSED             0xffff
> +
> +/*
> + * Primary SLR Table Header
> + */
> +struct slr_table
> +{
> +    uint32_t magic;
> +    uint16_t revision;
> +    uint16_t architecture;
> +    uint32_t size;
> +    uint32_t max_size;
> +    /* entries[] */
> +} __packed;

If x86-specific, the question on the need for some of the __packed arises
again.

> +/*
> + * Common SLRT Table Header
> + */
> +struct slr_entry_hdr
> +{
> +    uint32_t tag;
> +    uint32_t size;
> +} __packed;
> +
> +/*
> + * Boot loader context
> + */
> +struct slr_bl_context
> +{
> +    uint16_t bootloader;
> +    uint16_t reserved[3];
> +    uint64_t context;
> +} __packed;
> +
> +/*
> + * Prototype of a function pointed to by slr_entry_dl_info::dl_handler.
> + */
> +typedef void (*dl_handler_func)(struct slr_bl_context *bl_context);

It being an internal header, ...

> +/*
> + * DRTM Dynamic Launch Configuration
> + */
> +struct slr_entry_dl_info
> +{
> +    struct slr_entry_hdr hdr;
> +    uint64_t dce_size;
> +    uint64_t dce_base;
> +    uint64_t dlme_size;
> +    uint64_t dlme_base;
> +    uint64_t dlme_entry;
> +    struct slr_bl_context bl_context;
> +    uint64_t dl_handler;

... why can't this type be used here then? This would presumably avoid a
typecast later.

> +static inline void *
> +slr_end_of_entries(struct slr_table *table)
> +{
> +    return (uint8_t *)table + table->size;

Considering the function's return type, why not cast to void * (or perhaps
const void *, if the return type also can be such)?

> +}
> +
> +static inline struct slr_entry_hdr *
> +slr_next_entry(struct slr_table *table, struct slr_entry_hdr *curr)
> +{
> +    struct slr_entry_hdr *next = (struct slr_entry_hdr *)
> +                                 ((uint8_t *)curr + curr->size);
> +
> +    if ( (void *)next >= slr_end_of_entries(table) )
> +        return NULL;

Is this sufficient as a check? With it fulfilled, ...

> +    if ( next->tag == SLR_ENTRY_END )

... this member access may still be out of bounds. IOW the question is what
level of checking is really adequate here.

> +        return NULL;
> +
> +    return next;
> +}
> +
> +static inline struct slr_entry_hdr *
> +slr_next_entry_by_tag (struct slr_table *table,
> +                       struct slr_entry_hdr *entry,
> +                       uint16_t tag)
> +{
> +    if ( !entry ) /* Start from the beginning */
> +        entry = (struct slr_entry_hdr *)((uint8_t *)table + sizeof(*table));

Extending from the earlier comment - if the inner cast was to void * here,
the outer one could be dropped altogether.

Jan