> From 724d3bf216ffb094027daac33938ce5b8383e058 Mon Sep 17 00:00:00 2001 > From: Jiri Pirko > Date: Thu, 15 Oct 2026 16:50:50 +0200 > Subject: [PATCH 17/20] attestation: Add CTLV operation handling > > Translate CTLV evidence, MR and journal operations to the provider core and The commit message omits that `ctlv_attestation_op_pre`/`op_post` become exported symbols and that a Documentation sentence is added; neither is related to "translating operations". Say what is added and why. > wire the attestation family into the kernel build. > > Assisted-by: Cursor: GPT-5.6 Sol > Assisted-by: Cursor: Claude Opus 5.5 > Signed-off-by: Jiri Pirko > --- > Documentation/driver-api/attestation.rst | 4 + > drivers/Kconfig | 2 + > drivers/Makefile | 1 + > drivers/attestation/Makefile | 3 +- > drivers/attestation/core/core.c | 2 + > drivers/attestation/core/ctlv.c | 520 +++++++++++++++++++++++ > 6 files changed, 531 insertions(+), 1 deletion(-) > create mode 100644 drivers/attestation/core/ctlv.c > > diff --git a/Documentation/driver-api/attestation.rst b/Documentation/driver-api/attestation.rst > index 3cddd853aaa0..5ce121ded05f 100644 > --- a/Documentation/driver-api/attestation.rst > +++ b/Documentation/driver-api/attestation.rst > @@ -35,6 +35,10 @@ and event before invoking ``mr_extend``. A successful callback is followed > only by infallible publication, so userspace observes the same ordered > journal. > > +``MR_READ`` uses ``mr_read_generation`` only to validate query continuation; This documents a continuation that cannot happen: `ctlv_attestation_mr_read_query()` below never calls `ctlv_op_query_more()`, only `ctlv_op_query_done()`, so a non-zero cursor can never be presented and the generation check is dead (see the comment in `ctlv.c`). Either make `MR_READ` paginate or drop this text and `mr_read_generation`. The paragraph also belongs with the journal patch that introduced `mr_read_generation`, not here. > +the returned ``mr.generation`` is the journal boundary at which the values > +were read. > + > Teardown > ======== > > diff --git a/drivers/Kconfig b/drivers/Kconfig > index 9acc7f74c5c8..971db1f0192c 100644 > --- a/drivers/Kconfig > +++ b/drivers/Kconfig > @@ -27,6 +27,8 @@ source "drivers/connector/Kconfig" > > source "drivers/ctlv/Kconfig" > > +source "drivers/attestation/Kconfig" > + > source "drivers/firmware/Kconfig" > > source "drivers/fwctl/Kconfig" > diff --git a/drivers/Makefile b/drivers/Makefile > index f2c0c3a367ea..2b4dfc8e256b 100644 > --- a/drivers/Makefile > +++ b/drivers/Makefile > @@ -136,6 +136,7 @@ obj-$(CONFIG_INFINIBAND) += infiniband/ > obj-y += firmware/ > obj-$(CONFIG_FWCTL) += fwctl/ > obj-$(CONFIG_CTLV) += ctlv/ > +obj-$(CONFIG_ATTESTATION) += attestation/ > obj-$(CONFIG_CRYPTO) += crypto/ > obj-$(CONFIG_SUPERH) += sh/ > obj-y += clocksource/ > diff --git a/drivers/attestation/Makefile b/drivers/attestation/Makefile > index e7c9cb2dfec0..e520fee2e90d 100644 > --- a/drivers/attestation/Makefile > +++ b/drivers/attestation/Makefile > @@ -1,4 +1,5 @@ > # SPDX-License-Identifier: GPL-2.0 > obj-$(CONFIG_ATTESTATION) += attestation.o > > -attestation-y := core/core.o core/events.o core/mr.o > +attestation-y := core/core.o core/ctlv.o core/events.o core/mr.o \ > + generated/ctlv_attestation_gen.o > diff --git a/drivers/attestation/core/core.c b/drivers/attestation/core/core.c > index 6db8ed093e93..3fb58f344663 100644 > --- a/drivers/attestation/core/core.c > +++ b/drivers/attestation/core/core.c > @@ -326,6 +326,7 @@ int ctlv_attestation_op_pre(void *priv, struct ctlv_op_ctx *ctx) > } > return 0; > } > +EXPORT_SYMBOL_NS_GPL(ctlv_attestation_op_pre, "ATTESTATION"); Why export these? The only references are in generated/`ctlv_attestation_gen`.o, which is linked into the same attestation.o as core.o (see the Makefile hunk). Symbols nobody outside the module uses should not be exported (this also adds a namespace nothing imports). Drop both `EXPORT_SYMBOL_NS_GPL()`s or name the external user. > > void ctlv_attestation_op_post(void *priv, struct ctlv_op_ctx *ctx) > { > @@ -333,6 +334,7 @@ void ctlv_attestation_op_post(void *priv, struct ctlv_op_ctx *ctx) > > mutex_unlock(&att->lock); > } > +EXPORT_SYMBOL_NS_GPL(ctlv_attestation_op_post, "ATTESTATION"); > > static int __init attestation_module_init(void) > { > diff --git a/drivers/attestation/core/ctlv.c b/drivers/attestation/core/ctlv.c > new file mode 100644 > index 000000000000..ff19400843c2 > --- /dev/null > +++ b/drivers/attestation/core/ctlv.c > @@ -0,0 +1,520 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include "internal.h" > + > +static const struct attestation_evidence_supported_object * > +attestation_evidence_object_find(const struct attestation_evidence_caps *caps, const char *format) > +{ > + size_t index; > + > + for (index = 0; index < caps->objects_count; index++) { > + const struct attestation_evidence_supported_object *object = &caps->objects[index]; > + > + if (!strcmp(object->format, format)) > + return object; > + } > + return NULL; > +} > + > +static int attestation_device_caps_put(struct attestation *att, struct ctlv_cursor *reply) `blob_class`, representation and `max_output_length` below are single-use temporaries and 'flags' is declared at function scope for a single block; pass the constants directly. Also the function is inconsistent about error handling: some failed puts return `-EMSGSIZE` immediately (leaving nests un-ended), others accumulate into 'ok' and keep encoding into a reply that is going to be rejected anyway. > +{ > + const struct attestation_provider *provider = att->provider; > + struct ctlv_cursor caps_nest, group, object; > + bool ok = true; > + size_t index; > + u64 flags; > + > + if (!ctlv_attestation_device_caps_start(reply, &caps_nest)) > + return -EMSGSIZE; > + if (provider->evidence_caps) { > + const struct attestation_evidence_caps *caps = provider->evidence_caps; > + u64 blob_class = CTLV_ATTESTATION_EVIDENCE_BLOB_CLASS_PRIMARY; > + u64 representation = CTLV_ATTESTATION_EVIDENCE_REPRESENTATION_RAW; > + u64 max_output_length = caps->max_total_output_length; > + > + if (!ctlv_attestation_cap_evidence_start(&caps_nest, &group)) > + return -EMSGSIZE; > + ok &= ctlv_attestation_cap_min_challenge_length_put(&group, > + caps->min_challenge_length); > + ok &= ctlv_attestation_cap_max_challenge_length_put(&group, > + caps->max_challenge_length); > + ok &= ctlv_attestation_cap_max_total_output_length_put(&group, max_output_length); > + for (index = 0; index < caps->objects_count; index++) { > + const struct attestation_evidence_supported_object *cap = > + &caps->objects[index]; > + > + if (!ctlv_attestation_cap_object_start(&group, &object)) { > + ok = false; > + break; > + } > + ok &= ctlv_attestation_cap_blob_class_put(&object, blob_class); > + ok &= ctlv_attestation_cap_format_put(&object, cap->format); > + ok &= ctlv_attestation_cap_representation_put(&object, representation); > + ok &= ctlv_attestation_cap_max_length_put(&object, cap->max_length); > + ctlv_nest_end(&object); > + } > + ctlv_nest_end(&group); > + } > + if (provider->mrs_count) { > + if (!ctlv_attestation_cap_mr_start(&caps_nest, &group)) > + return -EMSGSIZE; > + flags = 0; > + if (provider->mr_read) > + flags |= CTLV_ATTESTATION_MR_DEVICE_CAPS_READ; > + if (provider->mr_extend) > + flags |= CTLV_ATTESTATION_MR_DEVICE_CAPS_EXTEND; > + ok &= ctlv_attestation_cap_mr_flags_put(&group, flags); > + ctlv_nest_end(&group); > + } > + ctlv_nest_end(&caps_nest); > + return ok ? 0 : -EMSGSIZE; > +} > + > +int ctlv_attestation_device_get_action(void *priv, struct ctlv_op_ctx *ctx, > + const struct ctlv_cursor *request, struct ctlv_cursor *reply) > +{ > + struct attestation *att = priv; > + struct ctlv_cursor info; > + int ret; > + > + ret = attestation_device_caps_put(att, reply); > + if (ret) > + return ret; > + if (!ctlv_attestation_device_info_start(reply, &info)) > + return -EMSGSIZE; > + ret = ctlv_stub_fill(ctx, request, &info); > + ctlv_nest_end(&info); > + return ret; > +} > + > +static int > +attestation_evidence_decode_objects(struct attestation *att, struct ctlv_op_ctx *ctx, > + const struct ctlv_cursor *request, > + struct attestation_evidence_request *decoded, > + struct attestation_evidence_object_request **objects_out) > +{ > + const struct attestation_evidence_caps *caps = att->provider->evidence_caps; > + struct attestation_evidence_object_request *objects; > + struct ctlv_cursor object; > + size_t count = 0, index = 0, previous; Resource leak in this function: blobs are resolved one by one in the loop below, but two of the error paths free only 'objects' and not the blobs already resolved for objects[0..index-1] (see the next two comments). `ctlv_blob_put()` is the release of a resolved blob; skipping it leaks the reference/pinned pages for every earlier object. > + > + ctlv_attestation_evidence_get_evidence_object_for_each(&object, request) count++; > + objects = kcalloc(count, sizeof(*objects), GFP_KERNEL); > + if (!objects) > + return -ENOMEM; > + ctlv_attestation_evidence_get_evidence_object_for_each(&object, request) > + { > + const struct attestation_evidence_supported_object *supported; > + const char *format; > + size_t total = 0; > + > + format = ctlv_attestation_evidence_get_object_evidence_format_get(&object); > + supported = attestation_evidence_object_find(caps, format); > + if (!supported) { > + kfree(objects); > + return -EOPNOTSUPP; Leak: unknown format with index > 0 returns after `kfree(objects)` without `ctlv_blob_put()` on objects[0..index-1].output. A user can trigger it with a request whose first object is valid and second names an unsupported format. Use a single error label that puts objects[0..index-1] and frees. > + } > + for (previous = 0; previous < index; previous++) > + if (objects[previous].supported_object == supported) { Same leak for duplicate formats (objects[0..index-1].output not put). Also the duplicate check is O(n^2) and is done after resolving the blob; do the cheap format lookup and duplicate check before `ctlv_`...`_evidence_data_resolve()`. > + kfree(objects); > + return -EINVAL; > + } > + objects[index].supported_object = supported; > + objects[index].output = > + ctlv_attestation_evidence_get_object_evidence_data_resolve(ctx, &object); > + if (IS_ERR(objects[index].output)) { > + int ret = PTR_ERR(objects[index].output); > + > + objects[index].output = NULL; > + while (index) > + ctlv_blob_put(objects[--index].output); > + kfree(objects); > + return ret; > + } > + if (ctlv_blob_len(objects[index].output) < supported->max_length) { Reading this literally: the caller must supply an output buffer of at least `max_length` for every requested object, else `-EMSGSIZE`. That is a surprising and strict contract (a client wanting a small format with a large declared `max_length` cannot ask for it) and it is not documented anywhere I can find in the series. If it is intended, document it in the spec. Note also that it makes the '`result.object_lengths[index]` > `ctlv_blob_len(...)`' test in `ctlv_attestation_evidence_get_action()` redundant (see there). > + for (previous = 0; previous <= index; previous++) > + ctlv_blob_put(objects[previous].output); > + kfree(objects); > + return -EMSGSIZE; > + } > + for (previous = 0; previous <= index; previous++) { > + if (check_add_overflow(total, ctlv_blob_len(objects[previous].output), 'total' is re-summed from scratch for all previous objects on every iteration (O(n^2)) although the running sum could just be carried in a variable declared outside the loop; also the inner 'for' reuses 'previous' as the loop variable for the cleanup loop inside the loop that uses it, which is confusing. `check_add_overflow()` on a `u64`/`size_t` pair: 'total' is `size_t`, `ctlv_blob_len()` is `u64`, so on 32-bit the addition is checked with a narrowing conversion into `size_t`. Use `u64` for total. > + &total) || > + total > caps->max_total_output_length) { > + for (previous = 0; previous <= index; previous++) > + ctlv_blob_put(objects[previous].output); > + kfree(objects); > + return -E2BIG; > + } > + } > + index++; > + } > + decoded->objects = objects; > + decoded->objects_count = count; `decoded->objects_count` and `objects_out` both carry the same information that the caller already has in 'decoded.objects'; the extra out parameter 'objects' of the two decode helpers exists only so the caller can free them after the main loop NULLs entries. decoded.objects would do and both helper signatures would shrink. > + *objects_out = objects; > + return 0; > +} > + > +static int attestation_evidence_request_decode(struct attestation *att, struct ctlv_op_ctx *ctx, > + const struct ctlv_cursor *request, > + struct attestation_evidence_request *decoded, > + struct attestation_evidence_object_request **objects) > +{ > + const struct attestation_evidence_caps *caps = att->provider->evidence_caps; > + u32 challenge_length; > + > + decoded->challenge = > + ctlv_attestation_evidence_get_evidence_challenge_get(request, &challenge_length); > + decoded->challenge_length = challenge_length; > + if (challenge_length < caps->min_challenge_length || > + challenge_length > caps->max_challenge_length) > + return -EINVAL; > + return attestation_evidence_decode_objects(att, ctx, request, decoded, objects); > +} > + > +static void attestation_evidence_request_free(struct attestation_evidence_object_request *objects, > + size_t count) > +{ > + while (count) { > + count--; > + if (objects[count].output) > + ctlv_blob_put(objects[count].output); > + } > + kfree(objects); > +} > + > +int ctlv_attestation_evidence_get_action(void *priv, struct ctlv_op_ctx *ctx, > + const struct ctlv_cursor *request, > + struct ctlv_cursor *reply) > +{ > + struct attestation_evidence_object_request *objects = NULL; > + struct attestation_evidence_request decoded = {}; > + struct attestation_evidence_result result = {}; > + struct attestation *att = priv; > + bool mr_dependent = false; > + size_t index; > + int put_ret; > + int ret; > + > + ret = attestation_evidence_request_decode(att, ctx, request, &decoded, &objects); > + if (ret) > + goto out; > + result.object_lengths = > + kcalloc(decoded.objects_count, sizeof(*result.object_lengths), GFP_KERNEL); > + if (!result.object_lengths) { > + ret = -ENOMEM; > + goto out; > + } > + for (index = 0; index < decoded.objects_count; index++) > + mr_dependent |= decoded.objects[index].supported_object->binds_mrs; > + ret = att->provider->evidence_get(att, &decoded, &result); `evidence_get()` is called with `att->lock` held (`op_pre`) and may be a slow TDX TDCALL/quote that sleeps up to a timeout; that blocks every other op, including pure `MR_GET`/DUMP and unregistration. Fine if intended, but document it. > + if (ret) > + goto out; > + for (index = 0; index < decoded.objects_count; index++) { > + if (result.object_lengths[index] > > + decoded.objects[index].supported_object->max_length || > + result.object_lengths[index] > ctlv_blob_len(decoded.objects[index].output)) { Redundant: decode already guaranteed `ctlv_blob_len(output)` >= `max_length`, so length <= `max_length` implies length <= `blob_len`. One of the two checks can go. Also: after the provider reports success, nothing here makes the blob contents beyond `object_lengths[]` defined; if the provider writes only `object_lengths` bytes, fine, but the unwritten remainder of a caller-provided buffer is not cleared (no leak since it is the caller's own buffer). > + ret = -E2BIG; > + goto out; > + } > + } > + for (index = 0; index < decoded.objects_count; index++) { > + put_ret = ctlv_blob_put(decoded.objects[index].output); Mixed use of 'decoded.objects' and 'objects' (same pointer) in adjacent lines; pick one. Also the put-error handling is odd: the first failing put sets ret, later puts still run, but nothing is reported to the provider that the result was not delivered, and a failed `blob_put` after the provider already did side effects (e.g. a Quote with generation increments) leaves the op failed. Probably fine, but the 'if (!ret) ret = `put_ret`' reads as if ret could be non-zero here; it cannot (all earlier errors goto out). > + objects[index].output = NULL; > + if (!ret) > + ret = put_ret; > + } > + if (ret) > + goto out; > + if (mr_dependent && !ctlv_attestation_mr_generation_put(reply, att->log->generation)) { The MR generation is sampled after the provider generated the evidence. Both are under `att->lock` and `mr_extend` runs under the same lock, so it is consistent; but a comment saying so would help since the invariant is non-obvious. `mr_dependent` is computed before `evidence_get()` for no reason; compute it where it is used. > + ret = -EMSGSIZE; > + goto out; > + } > + for (index = 0; index < decoded.objects_count; index++) > + if (!ctlv_attestation_evidence_object_length_put(reply, > + result.object_lengths[index])) { > + ret = -EMSGSIZE; > + goto out; > + } > +out: > + kfree(result.object_lengths); > + attestation_evidence_request_free(objects, decoded.objects_count); > + return ret; > +} > + > +int ctlv_attestation_mr_get_action(void *priv, struct ctlv_op_ctx *ctx, > + const struct ctlv_cursor *request, struct ctlv_cursor *reply) > +{ > + struct attestation *att = priv; > + struct attestation_mr *mr; > + u64 id = ctlv_attestation_mr_get_mr_id_get(request); > + > + if (id > UINT_MAX) > + return -ERANGE; > + mr = attestation_mr_lookup_live(att, id); > + return IS_ERR(mr) ? PTR_ERR(mr) : attestation_mr_props_encode(reply, mr, false); Style: this mixed ternary hides the error flow. Also 'ctx' and 'request' are unused in many of these handlers, which is fine for ops signatures, but the `u64->UINT_MAX` check/`-ERANGE` is repeated five times in this file (`mr_get`, `mr_read`, `mr_extend`, log selection); a small helper would remove the duplication. Why is an out-of-range id `-ERANGE` here but an unknown id (by lookup) presumably `-ENOENT`? An out-of-range id is simply unknown. > +} > + > +int ctlv_attestation_mr_dump_query(void *priv, struct ctlv_op_ctx *ctx, > + const struct ctlv_cursor *request, struct ctlv_cursor *reply) > +{ > + struct attestation *att = priv; > + struct attestation_mr *mr; > + struct ctlv_cursor nested; > + unsigned long id; > + u64 cursor = ctlv_op_query_cursor(ctx); > + unsigned int generation = att->log->generation; > + int ret; > + > + if (cursor && ctlv_op_query_generation(ctx) != generation) > + return -ESTALE; > + id = cursor ? cursor - 1 : 0; > + mr = xa_find(&att->mrs, &id, UINT_MAX, XA_PRESENT); `xa_find()` on `att->mrs`: is `att->mrs` protected by `att->lock` only (writers hold it) or does it need rcu/`xa_lock`? Holding `att->lock` via `op_pre` makes this OK, but then the pre-set cursor logic below should be documented: cursor is 'id + 1' so id 0 can be represented as cursor 1 while cursor 0 means start. Fine. One problem: if the very first nest does not fit (reply buffer too small), `ctlv_op_query_more()` is called with `next_cursor` == the current cursor (id + 1 == cursor when resuming at the same id) and `ctlv_op_query_more()` `WARN_ONCEs` and returns `-EINVAL` (`builder.c`: '`next_cursor` == `ctx->frag`.cursor' and `!ctx->frag.progress`). That is the general no-progress case and is presumably handled by the framework; just confirm it produces a sensible error instead of a WARN splat reachable from userspace with a tiny reply buffer. > + while (mr) { > + if (!ctlv_attestation_mr_item_start(reply, &nested)) > + return ctlv_op_query_more(ctx, generation, id + 1); > + ret = attestation_mr_props_encode(&nested, mr, true); > + ctlv_nest_end(&nested); > + if (ret) > + return ret; > + mr = xa_find_after(&att->mrs, &id, UINT_MAX, XA_PRESENT); > + } > + return ctlv_op_query_done(ctx, generation); > +} > + > +static int attestation_mr_values_put(struct ctlv_cursor *reply, > + const struct attestation_digest_set *values) > +{ > + struct ctlv_cursor nested; > + size_t index; > + bool ok; > + > + for (index = 0; index < values->values_count; index++) { > + const struct attestation_digest *value = &values->values[index]; > + enum hash_algo algorithm = value->algorithm; > + size_t length = hash_digest_size[algorithm]; > + > + if (!ctlv_attestation_mr_value_start(reply, &nested)) > + return -EMSGSIZE; > + ok = ctlv_attestation_mr_hash_algo_put(&nested, algorithm); > + ok &= ctlv_attestation_mr_data_put(&nested, value->data, length); > + ctlv_nest_end(&nested); > + if (!ok) > + return -EMSGSIZE; > + } > + return 0; > +} > + > +int ctlv_attestation_mr_read_query(void *priv, struct ctlv_op_ctx *ctx, > + const struct ctlv_cursor *request, struct ctlv_cursor *reply) > +{ > + struct attestation_digest_set values = {}; > + struct attestation *att = priv; > + struct attestation_mr *mr; > + u64 id; > + int ret; > + > + if (ctlv_op_query_cursor(ctx) && > + ctlv_op_query_generation(ctx) != att->mr_read_generation) Dead code: this function always finishes with `ctlv_op_query_done()`, never `ctlv_op_query_more()`, so `ctlv_op_query_cursor(ctx)` is never non-zero and this -ESTALE branch is unreachable. That makes `att->mr_read_generation`, its increment in `attestation_mr_changed()`, and the Documentation sentence all dead as well. Either implement pagination or remove them. > + return -ESTALE; > + id = ctlv_attestation_mr_read_mr_id_get(request); > + if (id > UINT_MAX) > + return -ERANGE; > + mr = attestation_mr_lookup_live(att, id); > + if (IS_ERR(mr)) > + return PTR_ERR(mr); > + if (!mr->props->readable) > + return -EOPNOTSUPP; > + values.values_count = mr->props->algorithms_count; > + values.values = kcalloc(values.values_count, sizeof(*values.values), GFP_KERNEL); > + if (!values.values) > + return -ENOMEM; > + for (id = 0; id < values.values_count; id++) Reusing 'id' (the MR id) as a loop counter is confusing, and after this loop 'id' no longer holds the MR id. Use a separate index variable. > + values.values[id].algorithm = mr->props->algorithms[id]; > + ret = att->provider->mr_read(att, mr->props->id, &values); > + if (!ret) > + ret = attestation_mr_values_validate(mr, &values); > + if (!ret && !ctlv_attestation_mr_generation_put(reply, att->log->generation)) > + ret = -EMSGSIZE; > + if (!ret) > + ret = attestation_mr_values_put(reply, &values); > + kfree(values.values); > + if (ret) > + return ret; > + return ctlv_op_query_done(ctx, att->mr_read_generation); Passing `mr_read_generation` here but reporting `att->log->generation` in the reply puts two different 'generation' numbers on the wire for one operation (the framework one and mr.generation). That is confusing for clients; the doc hunk above explains it but only by adding a third concept. Consider using the log generation for both and dropping `mr_read_generation`. > +} > + > +static int attestation_digest_compare(const void *left, const void *right) > +{ > + const struct attestation_digest *a = left; > + const struct attestation_digest *b = right; > + > + return (int)a->algorithm - (int)b->algorithm; Use `cmp_int()` or an explicit comparison, not subtraction (fine here only because enum values are small). Also: sorting by algorithm happens before validation; does `attestation_mr_values_validate()` reject duplicate algorithms? Duplicates are not rejected in the decode function above, so two entries with the same algorithm survive the sort. Please verify in `mr.c` that `validate()` catches this. > +} > + > +static int attestation_mr_extend_decode(const struct ctlv_cursor *request, > + struct attestation_digest_set *values) > +{ > + struct ctlv_cursor item; > + size_t count = 0, index = 0; > + > + ctlv_attestation_mr_extend_mr_value_for_each(&item, request) count++; > + values->values = kcalloc(count, sizeof(*values->values), GFP_KERNEL); > + if (!values->values) > + return -ENOMEM; > + values->values_count = count; > + ctlv_attestation_mr_extend_mr_value_for_each(&item, request) > + { > + struct attestation_digest *value = &values->values[index++]; 'count' comes from the first `for_each` pass and 'index' from the second; if the request could change between (it cannot, it is a kernel copy), the write would overflow. Bounded by schema? There is no explicit upper bound shown; a request with N `mr_value` attributes allocates N * `sizeof(digest)` (~72 bytes). Confirm the schema caps `mr_value` at a small max count. > + u32 length; > + const void *data; > + u64 algorithm; > + > + algorithm = > + ctlv_attestation_mr_extend_value_mr_hash_algo_get(&item); > + if (algorithm >= HASH_ALGO__LAST || !hash_digest_size[algorithm]) > + return -EINVAL; > + data = ctlv_attestation_mr_extend_value_mr_data_get(&item, &length); > + if (length != hash_digest_size[algorithm]) > + return -EINVAL; > + value->algorithm = algorithm; > + memcpy(value->data, data, length); > + } > + sort(values->values, values->values_count, sizeof(*values->values), > + attestation_digest_compare, NULL); > + return 0; > +} > + > +int ctlv_attestation_mr_extend_action(void *priv, struct ctlv_op_ctx *ctx, > + const struct ctlv_cursor *request, struct ctlv_cursor *reply) > +{ > + struct attestation_digest_set values = {}; > + struct attestation_mr_change *change; > + const struct attestation_mr_log_entry *entry; > + struct attestation *att = priv; > + struct attestation_mr *mr; > + u64 id; > + int ret; > + > + id = ctlv_attestation_mr_extend_mr_id_get(request); > + if (id > UINT_MAX) > + return -ERANGE; > + mr = attestation_mr_lookup_live(att, id); > + if (IS_ERR(mr)) > + return PTR_ERR(mr); > + if (!mr->props->extendable) > + return -EOPNOTSUPP; > + ret = attestation_mr_extend_decode(request, &values); > + if (ret) > + goto out_values; > + ret = attestation_mr_values_validate(mr, &values); > + if (ret) > + goto out_values; > + change = attestation_mr_change_prepare(att, mr, &values); > + if (IS_ERR(change)) { > + ret = PTR_ERR(change); > + goto out_values; > + } > + entry = attestation_mr_extend_locked(att, change); Atomicity problem: the extend is committed here (journal entry published, hardware register extended) and only then the reply is built. If `ctlv_attestation_mr_generation_put()` fails with `-EMSGSIZE` the op returns an error although the register was extended, and the client will typically retry and extend twice. Reserve/verify the reply space before committing, or make the put infallible by construction and WARN. > + if (IS_ERR(entry)) > + ret = PTR_ERR(entry); > + else if (!ctlv_attestation_mr_generation_put(reply, entry->mr_generation)) > + ret = -EMSGSIZE; > + attestation_mr_change_free(change); > +out_values: > + kfree(values.values); > + return ret; > +} > + > +static bool attestation_log_selected(const unsigned int *ids, size_t count, unsigned int id) > +{ > + size_t index; > + > + if (!count) > + return true; > + for (index = 0; index < count; index++) > + if (ids[index] == id) > + return true; > + return false; > +} > + > +int ctlv_attestation_mr_log_get_query(void *priv, struct ctlv_op_ctx *ctx, > + const struct ctlv_cursor *request, struct ctlv_cursor *reply) > +{ > + struct attestation *att = priv; > + struct attestation_mr_log_entry *entry; > + struct ctlv_cursor item, nested; > + unsigned int ids[256]; Open-coded 256 here duplicates the schema limit (generated file: `CTLV_NODE_SCHEMA_NEST(..._MR_SELECTION, 256, ...)`). If the generator enforces max 256 this is safe, but nothing ties the two numbers together; add a generated define or a `BUILD_BUG_ON`/WARN. If the limit is not enforced by the framework, `ids[ids_count++] = id` is a stack buffer overflow controlled by userspace. The duplicate check is O(n^2). > + size_t ids_count = 0, index; > + unsigned int start, end; > + unsigned long xa_index; 'boundary' is `u64` and 'start'/'end' are unsigned int, 'cursor' is `u64` and `xa_index` unsigned long: on 32-bit, cursor > `ULONG_MAX` is truncated into `xa_index`. Cursors are produced by the kernel from a 32-bit generation so only forged cursors are affected, but a forged cursor (e.g. 2^32 + 5) is then accepted as 5. Validate cursor <= `UINT_MAX`. > + u64 boundary; > + u64 cursor = ctlv_op_query_cursor(ctx); > + int ret; > + > + ctlv_attestation_mr_log_get_mr_selection_for_each(&item, request) > + { > + u64 id = ctlv_attestation_mr_log_get_selection_mr_id_get(&item); > + > + if (id > UINT_MAX) > + return -ERANGE; > + for (index = 0; index < ids_count; index++) > + if (ids[index] == id) > + return -EINVAL; > + ids[ids_count++] = id; Note: selection ids that name no MR are silently accepted (never validated against `att->mrs`), unlike `MR_GET`/READ/EXTEND which return an error for an unknown id. Inconsistent. > + } > + if (cursor) > + boundary = ctlv_op_query_generation(ctx); > + else > + boundary = att->log->generation; > + if (boundary > UINT_MAX || boundary > att->log->generation) The first condition is redundant with the second since `att->log->generation` is unsigned int. Also on a resumed query where the log can only grow, boundary > generation can only come from a forged generation token; -ESTALE is a strange error for it (`-EINVAL`). > + return -ESTALE; > + end = boundary; > + if (ctlv_attestation_mr_log_get_mr_end_generation_present(request)) { > + u64 requested_end = ctlv_attestation_mr_log_get_mr_end_generation_get(request); > + > + end = min_t(u64, end, requested_end); > + } > + start = 1; > + if (ctlv_attestation_mr_log_get_mr_start_generation_present(request)) { > + u64 value = ctlv_attestation_mr_log_get_mr_start_generation_get(request); > + > + if (value > UINT_MAX) > + return -EINVAL; > + start = value; > + } > + if (att->log->generation && start > end) > + return -EINVAL; > + if (!cursor && att->log->generation) { Behavior oddity: the first fragment (cursor == 0) carries only start/end generation and always ends in `ctlv_op_query_more()`, so a client pays an extra round trip with no entries; and when the journal is empty the reply has no start/end at all (the '`att->log->generation` &&' condition), which makes the reply shape depend on journal emptiness. Clients must handle the missing attributes. Spell this out in the spec or emit them unconditionally. > + if (!ctlv_attestation_mr_start_generation_put(reply, start) || > + !ctlv_attestation_mr_end_generation_put(reply, end)) > + return -EMSGSIZE; > + } > + xa_index = cursor ? cursor : start; > + entry = xa_find(&att->log->entries, &xa_index, end, XA_PRESENT); The 'find the next selected entry' loop is written out twice (here and at the end of the function). Factor into a helper returning the next selected entry. Also with one entry per fragment-continuation, a large journal costs one ctlv round per entry; the function emits a single entry per call (the cursor model) rather than filling the reply, which looks needlessly slow compared to `mr_dump_query` which loops. > + while (entry && !attestation_log_selected(ids, ids_count, entry->mr_id)) > + entry = xa_find_after(&att->log->entries, &xa_index, end, XA_PRESENT); > + if (!entry) > + return ctlv_op_query_done(ctx, boundary); > + if (!cursor) > + return ctlv_op_query_more(ctx, boundary, entry->mr_generation); > + if (!ctlv_attestation_log_entry_start(reply, &nested)) > + return -EMSGSIZE; > + ret = attestation_mr_log_entry_encode(&nested, entry); > + ctlv_nest_end(&nested); > + if (ret) > + return ret; > + entry = xa_find_after(&att->log->entries, &xa_index, end, XA_PRESENT); > + while (entry && !attestation_log_selected(ids, ids_count, entry->mr_id)) > + entry = xa_find_after(&att->log->entries, &xa_index, end, XA_PRESENT); > + if (entry) > + return ctlv_op_query_more(ctx, boundary, entry->mr_generation); > + return ctlv_op_query_done(ctx, boundary); > +} > -- > 2.55.0 > Summary of the most serious issues: - `ctlv_attestation_evidence_get_action()` decode path leaks resolved blobs (`ctlv_blob_put` never called) on the unknown-format and duplicate-format error paths. - `mr_read_query`'s ESTALE check, `att->mr_read_generation` and the new doc paragraph are dead: the op never paginates. - `MR_EXTEND` commits the extension and can then fail the reply with `-EMSGSIZE`, so the client sees an error for a completed extend. - Unneeded `EXPORT_SYMBOL_NS_GPL` of `op_pre`/`op_post` (same module). - Redundant blob length check, ids[256] only safe if the schema limit is enforced, 32-bit truncation of forged cursors in `log_get`, unvalidated selection ids, duplicated code (id range checks, next-entry loop). - Commit message does not describe exports/doc change.