Re: [PATCH RFC 17/20] attestation: Add CTLV operation handling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] core: stack buffer overflow in ctlv_attestation_mr_log_get_query()
- [Medium] core: ctlv_blob memory leak in attestation_evidence_decode_objects()
- [Medium] attestation: missing loop in ctlv_attestation_mr_log_get_query restricts pagination to one entry
--

commit e751f49f55526c0e053324e8d2f25a2d22385dc9
Author: Jiri Pirko <jiri@resnulli.us>

attestation: Add CTLV operation handling

Translate CTLV evidence, MR and journal operations to the provider core and
wire the attestation family into the kernel build.

> diff --git a/drivers/attestation/core/ctlv.c b/drivers/attestation/core/ctlv.c
> new file mode 100644
> index 0000000000000..ff19400843c2c
> --- /dev/null
> +++ b/drivers/attestation/core/ctlv.c
[ ... ]
> +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)
> +{
[ ... ]
> +	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;
> +		}

[Severity: Medium]
Will this return leak any successfully resolved blobs from earlier iterations?
On iterations where the index is greater than 0, previous objects have their
output resolved via
ctlv_attestation_evidence_get_object_evidence_data_resolve(). Because this
error path only calls kfree(objects), those earlier ctlv_blob references
seem to be leaked.

> +		for (previous = 0; previous < index; previous++)
> +			if (objects[previous].supported_object == supported) {
> +				kfree(objects);
> +				return -EINVAL;
> +			}

[Severity: Medium]
Does this face the same memory leak? If a duplicate format is found after some
blobs have already been resolved, it appears the accumulated references in
objects[0] to objects[index - 1] are not freed before returning here.

[ ... ]
> +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];
> +	size_t ids_count = 0, index;
> +	unsigned int start, end;
> +	unsigned long xa_index;
> +	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;
> +	}

[Severity: Critical]
Can this loop overflow the fixed-size ids[] stack array?
If a request contains more than 256 selections, it looks like ids_count
could exceed the size of ids[256] since there isn't a bounds check prior to
appending the id. Would it be possible to add a check to stop writing once
the array is full or return an error?

[ ... ]
> +	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);
> +}

[Severity: Medium]
Is it intentional that this function only encodes one entry per query?
Unlike ctlv_attestation_mr_dump_query() which has a loop over its entries,
this lacks a while (entry) loop wrapping the encode block. It serializes
exactly one entry and then returns ctlv_op_query_more() if more exist,
artificially restricting pagination to a single entry and causing multiple
requests to read the log. Should this iterate until the buffer fills up
(-EMSGSIZE)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015145053.3664509-1-jiri@resnulli.us?part=17