Re: [PATCH v2 15/18] PCI/CMA: Expose a log of received signatures in sysfs

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

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

> When authenticating a device with CMA-SPDM, the kernel verifies the
> challenge-response received from the device, but otherwise keeps it to
> itself.
> 
> James Bottomley contends that's not good enough because user space or a
> remote attestation service may want to re-verify the challenge-response:
> Either because it mistrusts the kernel or because the kernel is unaware
> of policy constraints that user space or the remote attestation service
> want to apply.
> 
> Facilitate such use cases by exposing a log in sysfs which consists of
> several files for each challenge-response event.  The files are prefixed
> with a monotonically increasing number, starting at 0:
> 
> /sys/devices/.../signatures/0_signature
> /sys/devices/.../signatures/0_transcript
> /sys/devices/.../signatures/0_requester_nonce
> /sys/devices/.../signatures/0_responder_nonce
> /sys/devices/.../signatures/0_hash_algorithm
> /sys/devices/.../signatures/0_combined_spdm_prefix
> /sys/devices/.../signatures/0_certificate_chain
> /sys/devices/.../signatures/0_type
> 
> The 0_signature is computed over the 0_transcript (a concatenation of
> all SPDM messages exchanged with the device).
> 
> To verify the signature, 0_transcript is hashed with 0_hash_algorithm
> (e.g. "sha384") and prefixed by 0_combined_spdm_prefix.
> 
> The public key to verify the signature against is the leaf certificate
> contained in 0_certificate_chain.
> 
> The nonces chosen by requester and responder are exposed as separate
> attributes to ease verification of their freshness.  They're already
> contained in the transcript but their offsets within the transcript are
> variable, so user space would otherwise have to parse the SPDM messages
> in the transcript to find the nonces.
> 
> The type attribute contains the event type:  Currently it is always
> "responder-challenge_auth signing".  In the future it may also contain
> "responder-measurements signing".
> 
> This custom log format was chosen for lack of a better alternative.
> Although the TCG PFP Specification defines DEVICE_SECURITY_EVENT_DATA
> structures, those structures do not store the transcript (which can be
> a few kBytes or up to several MBytes in size).  They do store nonces,
> hence at least allow for verification of nonce freshness.  But without
> the transcript, user space cannot verify the signature:
> 
> https://trustedcomputinggroup.org/resource/pc-client-specific-platform-firmware-profile-specification/
> 
> Exposing the transcript as an attribute of its own has the benefit that
> it can directly be fed into a protocol dissector for debugging purposes
> (think Wireshark).
> 
> 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>

Nice - particularly the thorough ABI docs.  A few trivial comments inline.
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>


> +/**
> + * spdm_create_log_entry() - Allocate log entry for one received SPDM signature
> + *
> + * @spdm_state: SPDM session state
> + * @spdm_context: SPDM context (needed to create combined_spdm_prefix)
> + * @slot: Slot which was used to generate the signature
> + *	(needed to create certificate_chain symlink)
> + * @req_nonce_off: Requester nonce offset within the transcript
> + * @rsp_nonce_off: Responder nonce offset within the transcript
> + *
> + * Allocate and populate a struct spdm_log_entry upon device authentication.
> + * Publish it in sysfs if the device has already been registered through
> + * device_add().
> + */
> +void spdm_create_log_entry(struct spdm_state *spdm_state,
> +			   const char *spdm_context, u8 slot,
> +			   size_t req_nonce_off, size_t rsp_nonce_off)
> +{
> +	struct spdm_log_entry *log = kmalloc(sizeof(*log), GFP_KERNEL);
> +	if (!log)
> +		return;
> +
> +	*log = (struct spdm_log_entry) {
> +		.slot		   = slot,
> +		.version	   = spdm_state->version,
> +		.counter	   = spdm_state->log_counter,
> +		.list		   = LIST_HEAD_INIT(log->list),
> +
> +		.sig = {
> +			.attr.name = log->sig_name,
> +			.attr.mode = 0444,
> +			.read	   = sysfs_bin_attr_simple_read,
> +			.private   = spdm_state->transcript_end -
> +				     spdm_state->sig_len,
> +			.size	   = spdm_state->sig_len },

We might set other bin_attr callbacks sometime in future, so I would
add the trailing comma and move the }, to the next line for these.

> +
> +		.req_nonce = {
> +			.attr.name = log->req_nonce_name,
> +			.attr.mode = 0444,
> +			.read	   = sysfs_bin_attr_simple_read,
> +			.private   = spdm_state->transcript + req_nonce_off,
> +			.size	   = SPDM_NONCE_SZ },
> +
> +		.rsp_nonce = {
> +			.attr.name = log->rsp_nonce_name,
> +			.attr.mode = 0444,
> +			.read	   = sysfs_bin_attr_simple_read,
> +			.private   = spdm_state->transcript + rsp_nonce_off,
> +			.size	   = SPDM_NONCE_SZ },
> +
> +		.transcript = {
> +			.attr.name = log->transcript_name,
> +			.attr.mode = 0444,
> +			.read	   = sysfs_bin_attr_simple_read,
> +			.private   = spdm_state->transcript,
> +			.size	   = spdm_state->transcript_end -
> +				     spdm_state->transcript -
> +				     spdm_state->sig_len },
> +
> +		.combined_prefix = {
> +			.attr.name = log->combined_prefix_name,
> +			.attr.mode = 0444,
> +			.read	   = spdm_read_combined_prefix,
> +			.private   = log,
> +			.size	   = spdm_state->version <= 0x11 ? 0 :
> +				     SPDM_COMBINED_PREFIX_SZ },
> +
> +		.spdm_context = {
> +			.attr.attr.name = log->spdm_context_name,
> +			.attr.attr.mode = 0444,
> +			.attr.show = device_show_string,
> +			.var	   = (char *)spdm_context },
> +
> +		.hash_alg = {
> +			.attr.attr.name = log->hash_alg_name,
> +			.attr.attr.mode = 0444,
> +			.attr.show = device_show_string,
> +			.var	   = (char *)spdm_state->base_hash_alg_name },
> +	};
> +
> +	snprintf(log->sig_name, sizeof(log->sig_name),
> +		 "%u_signature", spdm_state->log_counter);
> +	snprintf(log->req_nonce_name, sizeof(log->req_nonce_name),
> +		 "%u_requester_nonce", spdm_state->log_counter);
> +	snprintf(log->rsp_nonce_name, sizeof(log->rsp_nonce_name),
> +		 "%u_responder_nonce", spdm_state->log_counter);
> +	snprintf(log->transcript_name, sizeof(log->transcript_name),
> +		 "%u_transcript", spdm_state->log_counter);
> +	snprintf(log->combined_prefix_name, sizeof(log->combined_prefix_name),
> +		 "%u_combined_spdm_prefix", spdm_state->log_counter);
> +	snprintf(log->spdm_context_name, sizeof(log->spdm_context_name),
> +		 "%u_type", spdm_state->log_counter);
> +	snprintf(log->hash_alg_name, sizeof(log->hash_alg_name),
> +		 "%u_hash_algorithm", spdm_state->log_counter);
> +
> +	sysfs_bin_attr_init(&log->sig);
> +	sysfs_bin_attr_init(&log->req_nonce);
> +	sysfs_bin_attr_init(&log->rsp_nonce);
> +	sysfs_bin_attr_init(&log->transcript);
> +	sysfs_bin_attr_init(&log->combined_prefix);
> +	sysfs_attr_init(&log->spdm_context.attr.attr);
> +	sysfs_attr_init(&log->hash_alg.attr.attr);
> +
> +	list_add_tail(&log->list, &spdm_state->log);
> +	spdm_state->log_counter++;

Sanity check for roll over maybe? 

> +
> +	/* Steal transcript pointer ahead of spdm_free_transcript() */
> +	spdm_state->transcript = NULL;
> +
> +	if (device_is_registered(spdm_state->dev))
> +		spdm_publish_log_entry(&spdm_state->dev->kobj, log);
> +}
> +
> +/**
> + * spdm_publish_log() - Publish log of received SPDM signatures in sysfs
> + *
> + * @spdm_state: SPDM session state
> + *
> + * sysfs attributes representing received SPDM signatures are not static,
> + * but created dynamically upon authentication.  If a device was authenticated
> + * before it became visible in sysfs, the attributes could not be created.
> + * This function retroactively creates those attributes in sysfs after the
> + * device has become visible through device_add().
> + */
> +void spdm_publish_log(struct spdm_state *spdm_state)
> +{
> +	struct kobject *kobj = &spdm_state->dev->kobj;
> +	struct kernfs_node *grp_kn __free(kernfs_put);

As in previous reviews I'd keep constructor with destructor by declaring
these inline.

> +	struct spdm_log_entry *log;
> +
> +	grp_kn = kernfs_find_and_get(kobj->sd, spdm_signatures_group.name);
> +	if (WARN_ON(!grp_kn))
> +		return;
> +
> +	mutex_lock(&spdm_state->lock);

guard() perhaps.

> +	list_for_each_entry(log, &spdm_state->log, list) {
> +		struct kernfs_node *sig_kn __free(kernfs_put);
> +
> +		/*
> +		 * Skip over log entries created in-between device_add() and
> +		 * spdm_publish_log() as they've already been published.
> +		 */
> +		sig_kn = kernfs_find_and_get(grp_kn, log->sig_name);
> +		if (sig_kn)
> +			continue;
> +
> +		spdm_publish_log_entry(kobj, log);
> +	}
> +	mutex_unlock(&spdm_state->lock);
> +}
> +EXPORT_SYMBOL_GPL(spdm_publish_log);