Re: [PATCH v2 18/18] spdm: Allow control of next requester nonce through sysfs

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Sun, 30 Jun 2024 21:53:00 +0200
Lukas Wunner <lukas@wunner.de> wrote:

> Remote attestation services may mistrust the kernel to always use a
> fresh nonce for SPDM authentication.
> 
> So allow user space to set the next requester nonce by writing to a
> sysfs attribute.
> 
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> Cc: James Bottomley <James.Bottomley@HansenPartnership.com>
> Cc: Jérôme Glisse <jglisse@google.com>
> Cc: Jason Gunthorpe <jgg@nvidia.com>
Why is the group visibility callback in this patch?


Otherwise looks fine to me,

Jonathan


> ---
>  Documentation/ABI/testing/sysfs-devices-spdm | 29 ++++++++++++++++
>  lib/spdm/core.c                              |  1 +
>  lib/spdm/req-authenticate.c                  |  8 ++++-
>  lib/spdm/req-sysfs.c                         | 35 ++++++++++++++++++++
>  lib/spdm/spdm.h                              |  4 +++
>  5 files changed, 76 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-devices-spdm b/Documentation/ABI/testing/sysfs-devices-spdm
> index 5ce34ce10b9c..d315b47b4af0 100644
> --- a/Documentation/ABI/testing/sysfs-devices-spdm
> +++ b/Documentation/ABI/testing/sysfs-devices-spdm
> @@ -216,3 +216,32 @@ Description:
>  		necessary to parse the SPDM messages in the transcript to find
>  		and extract the nonces, which is cumbersome.  That's why they
>  		are exposed as separate files.
> +
> +
> +What:		/sys/devices/.../signatures/next_requester_nonce
> +Date:		June 2024
> +Contact:	Lukas Wunner <lukas@wunner.de>
> +Description:
> +		If you do not trust the kernel to always use a fresh nonce,
> +		write 32 bytes to this file to set the requester nonce used
> +		in the next SPDM authentication sequence.
> +
> +		Meant for remote attestation services.  You are responsible
> +		for providing a nonce with sufficient entropy.  The kernel
> +		only uses the nonce once, so provide a new one every time
> +		you reauthenticate the device.  If you do not provide a
> +		nonce, the kernel generates a random one.
> +
> +		After the nonce has been consumed, it becomes readable as
> +		the newest [0-9]*_requester_nonce, which proves its usage::
> +
> +		 # dd if=/dev/random bs=32 count=1 | \
> +		   tee signatures/next_requester_nonce | hexdump
> +		 0000000 e0 77 91 54 bd 56 99 c2 ea 4f 0b 1a 7f ba 6e 59
> +		 0000010 8f ee f6 b2 26 82 58 34 9e e5 8c 8a 31 58 29 7e
> +
> +		 # echo re > authenticated
> +
> +		 # hexdump $(\ls -t signatures/[0-9]*_requester_nonce | head -1)
> +		 0000000 e0 77 91 54 bd 56 99 c2 ea 4f 0b 1a 7f ba 6e 59
> +		 0000010 8f ee f6 b2 26 82 58 34 9e e5 8c 8a 31 58 29 7e
> diff --git a/lib/spdm/core.c b/lib/spdm/core.c
> index b6a46bdbb2f9..7371adb7a52f 100644
> --- a/lib/spdm/core.c
> +++ b/lib/spdm/core.c
> @@ -434,6 +434,7 @@ void spdm_destroy(struct spdm_state *spdm_state)
>  	spdm_reset(spdm_state);
>  	spdm_destroy_log(spdm_state);
>  	mutex_destroy(&spdm_state->lock);
> +	kfree(spdm_state->next_nonce);
>  	kfree(spdm_state);
>  }
>  EXPORT_SYMBOL_GPL(spdm_destroy);
> diff --git a/lib/spdm/req-authenticate.c b/lib/spdm/req-authenticate.c
> index 7c977f5835c1..489fc88de74d 100644
> --- a/lib/spdm/req-authenticate.c
> +++ b/lib/spdm/req-authenticate.c
> @@ -626,7 +626,13 @@ static int spdm_challenge(struct spdm_state *spdm_state, u8 slot, bool verify)
>  	};
>  	int rc, length;
>  
> -	get_random_bytes(&req.nonce, sizeof(req.nonce));
> +	if (spdm_state->next_nonce) {
> +		memcpy(&req.nonce, spdm_state->next_nonce, sizeof(req.nonce));
> +		kfree(spdm_state->next_nonce);
> +		spdm_state->next_nonce = NULL;
> +	} else {
> +		get_random_bytes(&req.nonce, sizeof(req.nonce));
> +	}
>  
>  	if (spdm_state->version <= 0x12)
>  		req_sz = offsetofend(typeof(req), nonce);
> diff --git a/lib/spdm/req-sysfs.c b/lib/spdm/req-sysfs.c
> index c782054f8e18..232d4a00a510 100644
> --- a/lib/spdm/req-sysfs.c
> +++ b/lib/spdm/req-sysfs.c
> @@ -176,13 +176,48 @@ const struct attribute_group spdm_certificates_group = {
>  
>  /* signatures attributes */
>  
> +static umode_t spdm_signatures_are_visible(struct kobject *kobj,
> +					   struct bin_attribute *a, int n)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct spdm_state *spdm_state = dev_to_spdm_state(dev);
> +
> +	if (IS_ERR_OR_NULL(spdm_state))
> +		return SYSFS_GROUP_INVISIBLE;
> +
> +	return a->attr.mode;
> +}
> +
> +static ssize_t next_requester_nonce_write(struct file *file,
> +					  struct kobject *kobj,
> +					  struct bin_attribute *attr,
> +					  char *buf, loff_t off, size_t count)
> +{
> +	struct device *dev = kobj_to_dev(kobj);
> +	struct spdm_state *spdm_state = dev_to_spdm_state(dev);
> +
> +	guard(mutex)(&spdm_state->lock);
> +
> +	if (!spdm_state->next_nonce) {
> +		spdm_state->next_nonce = kmalloc(SPDM_NONCE_SZ, GFP_KERNEL);
> +		if (!spdm_state->next_nonce)
> +			return -ENOMEM;
> +	}
> +
> +	memcpy(spdm_state->next_nonce + off, buf, count);
> +	return count;
> +}
> +static BIN_ATTR_WO(next_requester_nonce, SPDM_NONCE_SZ);
> +
>  static struct bin_attribute *spdm_signatures_bin_attrs[] = {
> +	&bin_attr_next_requester_nonce,
>  	NULL
>  };
>  
>  const struct attribute_group spdm_signatures_group = {
>  	.name = "signatures",
>  	.bin_attrs = spdm_signatures_bin_attrs,
> +	.is_bin_visible = spdm_signatures_are_visible,
>  };
>  
>  static unsigned int spdm_max_log_sz = SZ_16M; /* per device */
> diff --git a/lib/spdm/spdm.h b/lib/spdm/spdm.h
> index 448107c92db7..aa36aa55e718 100644
> --- a/lib/spdm/spdm.h
> +++ b/lib/spdm/spdm.h
> @@ -475,6 +475,9 @@ struct spdm_error_rsp {
>   *	itself and the transcript with trailing signature.
>   * @log_counter: Number of generated log entries so far.  Will be prefixed to
>   *	the sysfs files of the next generated log entry.
> + * @next_nonce: Requester nonce to be used for the next authentication
> + *	sequence.  Populated from user space through sysfs.
> + *	If user space does not provide a nonce, the kernel uses a random one.
>   */
>  struct spdm_state {
>  	struct device *dev;
> @@ -521,6 +524,7 @@ struct spdm_state {
>  	struct list_head log;
>  	size_t log_sz;
>  	u32 log_counter;
> +	u8 *next_nonce;
>  };
>  
>  extern struct list_head spdm_state_list;