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

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

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@linux.intel.com>
> ---
> 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.