Re: [PATCH RFC v2 09/15] ctlv: Prepare VA blob handling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] ctlv: pages allocated before RLIMIT_MEMLOCK check in
  ctlv_blob_va_acquire()
- [Medium] ctlv: inconsistent record_offset on bad blob in
  ctlv_blob_resolve()
--

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

ctlv: Prepare VA blob handling

This commit adds task-bound VA blob resolution and CPU read/write access
helpers for CTLV families.

> diff --git a/lib/ctlv/blob.c b/lib/ctlv/blob.c
> new file mode 100644
> index 0000000000000..14cbbc4bcab1c
> --- /dev/null
> +++ b/lib/ctlv/blob.c
[ ... ]
> +static int ctlv_blob_va_range(u64 address, u64 len, unsigned long *first_page,
> +			      unsigned long *pages_count)
> +{
> +	unsigned long first, last, span, count;
> +	size_t bytes;
> +	u64 end;
> +
> +	if (!len)
> +		return -EINVAL;
> +	if (check_add_overflow(address, len, &end))
> +		return -EOVERFLOW;
> +	if (end > TASK_SIZE)
> +		return -EFAULT;
> +
> +	/* Bounded by an unsigned long above, so nothing below loses a bit. */
> +	first = (unsigned long)address & PAGE_MASK;
> +	last = ((unsigned long)end - 1) & PAGE_MASK;
> +	if (check_sub_overflow(last, first, &span) ||
> +	    check_add_overflow(span >> PAGE_SHIFT, 1UL, &count) ||
> +	    check_mul_overflow(count, sizeof(struct page *), &bytes))
> +		return -EOVERFLOW;
> +
> +	*first_page = first;
> +	*pages_count = count;
> +	return 0;
> +}
> +
> +static int ctlv_blob_va_acquire(struct ctlv_blob *blob, u64 address)
> +{
> +	unsigned long pinned = 0, first, pages_count;
> +	unsigned int gup_flags = 0;
> +	struct page **pages;
> +	int ret;
> +
> +	/* Before charging or allocating, both of which this sizes. */
> +	ret = ctlv_blob_va_range(address, blob->len, &first, &pages_count);
> +	if (ret)
> +		return ret;
> +
> +	pages = kvmalloc_array(pages_count, sizeof(*pages), GFP_KERNEL_ACCOUNT);
> +	if (!pages)
> +		return -ENOMEM;
> +
> +	if (ctlv_blob_writable(blob))
> +		gup_flags |= FOLL_WRITE;
> +	ret = ctlv_blob_pin_charge(current->mm, pages_count);
> +	if (ret)
> +		goto err_free_pages;

[Severity: Medium]
Should ctlv_blob_pin_charge() run before kvmalloc_array()?

An unprivileged caller with a small RLIMIT_MEMLOCK can pass a large blob
length to force a multi-megabyte vmalloc allocation that is immediately
freed when ctlv_blob_pin_charge() rejects the page count.  In addition,
check_mul_overflow() only bounds bytes to SIZE_MAX, so if a schema allows
a blob whose page array exceeds INT_MAX bytes, kvmalloc_array() without
__GFP_NOWARN will trigger the size warning in __kvmalloc_node_noprof().

[ ... ]

> +/*
> + * Which of an op's blobs was the unusable one, in the shape validation would
> + * have used. The errno the family returns says what went wrong.
> + */
> +static void ctlv_blob_context_fail(struct ctlv_op_ctx *ctx, u32 tlv_offset, u32 record_offset,
> +				   u64 required_length)
> +{
> +	if (ctx->error.reason)
> +		return;
> +	ctx->error.reason = CTLV_VALIDATION_REASON_BAD_BLOB_DESCRIPTOR;
> +	ctx->error.bad_tlv_offset = tlv_offset;
> +	ctx->error.record_offset = record_offset;
> +	ctx->error.required_length = required_length;
> +}
[ ... ]
> +struct ctlv_blob *ctlv_blob_resolve(struct ctlv_op_ctx *ctx, const struct ctlv_cursor *blob)
> +{
> +	const struct ctlv_hdr *bad = NULL;
> +	const struct ctlv_node_schema *node;
> +	u32 tlv_offset, record_offset;
> +	struct ctlv_blob_desc desc;
> +	struct ctlv_blob *acquired;
> +
> +	/* An absent blob, or a cursor from some other execution entirely. */
> +	if (WARN_ON_ONCE(blob->msg != &ctx->request || !ctlv_cursor_ok(blob)))
> +		return ERR_PTR(-EINVAL);
> +	node = &blob->tree->node_schemas[blob->node];
> +	if (WARN_ON_ONCE(node->kind != CTLV_SCHEMA_BLOB))
> +		return ERR_PTR(-EINVAL);
> +	if (WARN_ON_ONCE(!ctx->blobs_live || !ctx->submitter || ctx->submitter != current))
> +		return ERR_PTR(-EINVAL);
> +	/* Validated before the callback, so anything wrong with it is a bug. */
> +	if (WARN_ON_ONCE(ctlv_blob_desc_read(blob->hdr, &desc, &bad) ||
> +			 ctlv_blob_desc_check(&desc, node)))
> +		return ERR_PTR(-EINVAL);
> +
> +	tlv_offset = ctlv_message_offset(blob->msg, blob->hdr);
> +	record_offset = ctlv_message_offset(blob->msg, blob->parent);
> +	acquired = ctlv_blob_acquire(&desc, node->blob.direction);
> +	if (IS_ERR(acquired)) {
> +		ctlv_blob_context_fail(ctx, tlv_offset, record_offset, 0);
> +		return acquired;
> +	}

[Severity: Medium]
Does record_offset here match what schema validation reports for
CTLV_VALIDATION_REASON_BAD_BLOB_DESCRIPTOR?

ctlv_blob_resolve() sets record_offset to blob->parent, the record
containing the blob descriptor.  However, when ctlv_blob_desc_check()
fails during validation, ctlv_schema_check_blob() passes the blob TLV
itself as the record argument to ctlv_schema_bad():

lib/ctlv/schema.c:ctlv_schema_check_blob() {
    ...
	err = ctlv_blob_desc_check(&desc, node);
	if (err)
		ctlv_schema_bad(v, CTLV_VALIDATION_REASON_BAD_BLOB_DESCRIPTOR, blob, NULL);
	return err;
}

Because hdr is NULL there, ctlv_schema_bad() sets both bad_tlv_offset and
record_offset to the offset of blob, so userspace sees a different
record_offset depending on whether the blob failed validation or
resolution.

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