Re: [PATCH V4 3/3] tools/arch/x86/intel_sdsi: Add attestation support

From: Ilpo Järvinen
Date: Sat Jul 06 2024 - 10:03:52 EST


On Fri, 7 Jun 2024, David E. Box wrote:

> Add support in the intel_sdsi tool to perform SPDM GET_DIGESTS and
> GET_CERTIFICATE commands. Output is sent to stdout.
>
> Example reading the certificate chain from socket 0:
>
> intel_sdsi -d 1 -attest get_certificate | openssl x509 -inform DER -nout -text
>
> Signed-off-by: David E. Box <david.e.box@xxxxxxxxxxxxxxx>
> ---
> V4 - No change
>
> V3 - No change
>
> V2 - Remove unnecessary struct packing
> - Remove newline from perror()
> - Add message options in --help output
> - Use new SDSI_SPDM_BUF_SIZE from uapi header
> - In spdm_get_certificate:
> - Initialize remainder length to the minimum of the actual size
> or the maximum buffer size.
> - Add old_remainder to test that the remaining certificate
> length is less than the previous length
>
> tools/arch/x86/intel_sdsi/Makefile | 11 +-
> tools/arch/x86/intel_sdsi/intel_sdsi.c | 72 +++-
> tools/arch/x86/intel_sdsi/spdm.c | 476 +++++++++++++++++++++++++
> tools/arch/x86/intel_sdsi/spdm.h | 13 +
> 4 files changed, 567 insertions(+), 5 deletions(-)
> create mode 100644 tools/arch/x86/intel_sdsi/spdm.c
> create mode 100644 tools/arch/x86/intel_sdsi/spdm.h
>

> +++ b/tools/arch/x86/intel_sdsi/spdm.c
> @@ -0,0 +1,476 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * spdm: Lightweight Security Protocol and Data Model (SPDM) specification
> + * support code for performing attestation commands using the Intel On
> + * Demand driver ioctl interface. Intel On Demand currently supports
> + * SPDM version 1.0
> + *
> + * See the SPDM v1.0 specification at:
> + * https://www.dmtf.org/sites/default/files/standards/documents/DSP0274_1.0.1.pdf
> + *
> + * Copyright (C) 2024 Intel Corporation. All rights reserved.
> + */
> +
> +#include<linux/bits.h>
> +
> +#include<fcntl.h>
> +#include<stdio.h>
> +#include<stdlib.h>
> +#include<stdint.h>
> +#include<string.h>
> +#include<unistd.h>
> +#include<sys/ioctl.h>

All missing spaces. :-(

> +static int sdsi_process_ioctl(int ioctl_no, void *info, uint8_t dev_no)
> +{
> + char pathname[14];
> + int fd, ret;
> +
> + ret = snprintf(pathname, 14, "%s%d", SDSI_DEV_PATH, dev_no);

sizeof(pathname)

> + remainder_length = size < SDSI_SPDM_BUF_SIZE ? size : SDSI_SPDM_BUF_SIZE;
> + old_remainder = remainder_length;
> +
> + while (remainder_length) {
> + uint16_t length;
> +
> + length = remainder_length < SDSI_SPDM_BUF_SIZE ?
> + remainder_length : SDSI_SPDM_BUF_SIZE;
> + offset += portion_length;

The way bound check interplay with old_remainder and remainder_length in
this code is quite convoluted and could contain some problems.

Would it work if old_remainder is set only here and the bound check
before the loop is replaced with a plain remainder_length = size
assignment?

> +
> + ret = get_certificate_portion(dev_no, offset, length,
> + &portion_length,
> + &remainder_length,
> + c->chain);
> + if (ret < 0)
> + goto free_cert_chain;
> +
> + if (!(remainder_length < old_remainder)) {
> + fprintf(stderr, "Bad GET_CERTIFICATE length\n");
> + ret = -1;
> + goto free_cert_chain;
> + }
> +
> + old_remainder = remainder_length;
> + }
> +
> + c->len = offset + portion_length;
> + return 0;
> +
> +free_cert_chain:
> + free(c->chain);
> + c->chain = NULL;
> + return ret;
> +}
> diff --git a/tools/arch/x86/intel_sdsi/spdm.h b/tools/arch/x86/intel_sdsi/spdm.h
> new file mode 100644
> index 000000000000..aa7e08ffb872
> --- /dev/null
> +++ b/tools/arch/x86/intel_sdsi/spdm.h
> @@ -0,0 +1,13 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#include <stdint.h>
> +
> +#define TPM_ALG_SHA_384_SIZE 48
> +
> +struct cert_chain {
> + void *chain;
> + size_t len;
> +};
> +
> +int spdm_get_digests(int dev_no, uint8_t digest[TPM_ALG_SHA_384_SIZE]);
> +int spdm_get_certificate(int dev_no, struct cert_chain *c);
> +

Trailing newline.

--
i.