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

Jonathan Cameron
2024-07-18T15:56:31Z

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);