On Sun, 30 Jun 2024 21:50:00 +0200 Lukas Wunner <lukas@xxxxxxxxx> 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@xxxxxxxxx> > Cc: James Bottomley <James.Bottomley@xxxxxxxxxxxxxxxxxxxxx> > Cc: Jérôme Glisse <jglisse@xxxxxxxxxx> > Cc: Jason Gunthorpe <jgg@xxxxxxxxxx> Nice - particularly the thorough ABI docs. A few trivial comments inline. Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> > +/** > + * 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);
