Re: [PATCH v17 06/17] iov_iter: Add a bvecq kunit test

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Low] iov_iter: bvecq_kunit_advance() bounds revert steps by left
  instead of pos
- [Low] iov_iter: unchecked kunit_add_action_or_reset() in kunit_bvecq.c
  causes UAF
--

commit 395487bf3dfe00b3cc632478de3e9afe0e1c97ad
Author: David Howells <dhowells@redhat.com>

iov_iter: Add a bvecq kunit test

This commit adds a KUnit test suite (CONFIG_TEST_BVECQ) to exercise
ITER_BVECQ iterator operations including advance, revert, npages, and
alignment across various bvecq chains and partial folio configurations.

> diff --git a/lib/tests/kunit_bvecq.c b/lib/tests/kunit_bvecq.c
> new file mode 100644
> index 0000000000000..19f3dc41eda78
> --- /dev/null
> +++ b/lib/tests/kunit_bvecq.c
> @@ -0,0 +1,599 @@

[ ... ]

> +static void iov_bvecq_alloc_folios(struct kunit *test)
> +{
> +	struct folio *folio, **folios = bvecq_kunit_folios;
> +	const int nr = ARRAY_SIZE(bvecq_kunit_folios);
> +
> +	bvecq_kunit_pagemap_nr = 0;
> +	memset(bvecq_kunit_pagemap, 0, sizeof(bvecq_kunit_pagemap));
> +	memset(bvecq_kunit_folios, 0, sizeof(bvecq_kunit_folios));
> +
> +	kunit_add_action_or_reset(test, bvecq_kunit_release_folios, NULL);
> +
> +	for (int i = 0; i < nr; i++) {
> +		folio = folio_alloc(GFP_KERNEL, bvecq_kunit_orders[i]);

[ ... ]

> +static struct bvecq *bvecq_kunit_load_chain(struct kunit *test,
> +					    const struct bvecq_test_recipe *rec,
> +					    const struct bvecq_test_partials *parts,
> +					    size_t *size)
> +{
> +	struct bvecq *head = NULL, *bq = NULL, *p;
> +	size_t pos = 0;
> +	int folio_index = 0;
> +
> +	*size = 0;
> +
> +	for (; rec->cmd != BVECQ_KUNIT_STOP; rec++) {
> +		if (!head)
> +			KUNIT_ASSERT_EQ(test, rec->cmd, BVECQ_KUNIT_ALLOC);
> +
> +		switch (rec->cmd) {
> +		case BVECQ_KUNIT_STOP:
> +			break;
> +
> +		case BVECQ_KUNIT_ALLOC:
> +			p = bvecq_alloc_one(rec->count, GFP_KERNEL, false);
> +			KUNIT_ASSERT_NOT_NULL(test, p);
> +
> +			if (!head) {
> +				head = p;
> +				kunit_add_action_or_reset(test, bvecq_kunit_bvecq_put, head);
> +				bq = p;
> +			} else {
> +				bvecq_append(bq, p);
> +				bq = p;
> +			}
> +			break;

[Severity: Low]
Does an allocation failure in kunit_add_action_or_reset() cause a
use-after-free on head in bvecq_kunit_load_chain() and leak the folios in
iov_bvecq_alloc_folios()?

When kunit_add_action() fails to allocate its action wrapper,
kunit_add_action_or_reset() immediately runs the cleanup callback and
returns an error without aborting the test.  In bvecq_kunit_load_chain(),
bvecq_kunit_bvecq_put() frees head via bvecq_put(), and the loop then
continues using the freed bq pointer and leaks any subsequent segments
appended to the chain.  Similarly, in iov_bvecq_alloc_folios(),
bvecq_kunit_release_folios() runs before the loop, leaving the 16 folios
allocated afterward without a cleanup action.  Asserting that
kunit_add_action_or_reset() returns 0 (and resetting bvecq_kunit_folios_nr
to 0 alongside the memset() in iov_bvecq_alloc_folios()) would avoid this.

[ ... ]

> +static void bvecq_kunit_advance(struct kunit *test,
> +				const struct bvecq_test_recipe *recipe,
> +				const struct bvecq_test_partials *parts)
> +{

[ ... ]

> +	npages = iov_iter_npages(&iter, INT_MAX);
> +	from = iov_iter_count(&iter);
> +	do {
> +		size_t left = from, part;
> +
> +		KUNIT_ASSERT_EQ(test, iov_iter_npages(&iter, INT_MAX),
> +				npages - bvecq_kunit_pos2page(test, pos));
> +
> +		/* 3 steps forward. */
> +		part = min(left, 3);
> +		iov_iter_advance(&iter, 3); /* May be short. */
> +		pos += part;
> +		left -= part;
> +		bvecq_kunit_check_next(test, &iter, &pos, &left, 3);
> +
> +		/* 2 steps back */
> +		part = umin(2, left);
> +		iov_iter_revert(&iter, part);
> +		pos -= part;
> +		left += part;
> +		bvecq_kunit_check_next(test, &iter, &pos, &left, 2);
> +
> +		/* 1 more step back */
> +		part = umin(1, left);
> +		iov_iter_revert(&iter, part);
> +		pos -= part;
> +		left += part;
> +		bvecq_kunit_check_next(test, &iter, &pos, &left, 1);

[Severity: Low]
Should the backward revert steps here be bounded by pos rather than left?

left is the number of unread bytes remaining ahead in the iterator, while
pos is the number of bytes already advanced from the start.  When the
forward advance and check reach the end of the iterator (left == 0 on the
last 6 iterations), umin(..., left) evaluates to 0, turning both
iov_iter_revert() calls into no-ops instead of testing reverts at the tail
of the bvecq chain.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111203034.2126314-1-dhowells@redhat.com?part=6