Re: [PATCH v2 2/4] drm/xe/kunit: Extend GuC ID manager split-pool coverage

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Out-of-bounds memory write via `bitmap_clear()` if a resource allocation fails during KUnit tests.
--

--- Patch [2]: [PATCH v2 2/4] drm/xe/kunit: Extend GuC ID manager split-pool coverage ---
commit de07034d7b51f00db6617697a552f2ac15c74f32
Author: Piotr Piórkowski <[email protected]>

drm/xe/kunit: Extend GuC ID manager split-pool coverage

Expand the GuC ID manager KUnit coverage to exercise the new
usable/shareable pool model.

> diff --git a/drivers/gpu/drm/xe/tests/xe_guc_id_mgr_test.c b/drivers/gpu/drm/xe/tests/xe_guc_id_mgr_test.c
> index 103ea82d278e3..295c17dbc21fb 100644
> --- a/drivers/gpu/drm/xe/tests/xe_guc_id_mgr_test.c
> +++ b/drivers/gpu/drm/xe/tests/xe_guc_id_mgr_test.c

[ ... ]

> +static void check_used_usable(struct kunit *test)
> +{
> +	struct xe_guc_id_mgr *idm = test->priv;
> +	int id;
> +
> +	KUNIT_ASSERT_EQ(test, 0, idm_init(idm, 8, 8));
> +
> +	mutex_lock(idm_mutex(idm));
> +
> +	KUNIT_EXPECT_EQ(test, idm_used_usable(idm), 0);
> +	KUNIT_EXPECT_EQ(test, idm_used_shareable(idm), 0);
> +
> +	id = xe_guc_id_mgr_reserve_usable_locked(idm, 2);
> +	KUNIT_EXPECT_GE(test, id, 0);

[Severity: High]
The commit message notes that KUNIT_EXPECT is used instead of KUNIT_ASSERT to
avoid leaking the mutex on failure. However, if the reservation fails and
returns a negative error code, KUNIT_EXPECT_GE() will allow execution to
continue.

Since idm_release_chunk_locked() takes an unsigned int for the start position,
will passing a negative id result in an implicit cast to a huge unsigned
integer, causing an out-of-bounds write in bitmap_clear() when the chunk is
released below?

Would it be better to use a conditional check to handle the error and unlock
the mutex before returning, rather than letting the test continue with a
negative id?

> +
> +	KUNIT_EXPECT_EQ(test, idm_used_usable(idm), 2);
> +	KUNIT_EXPECT_EQ(test, idm_used_shareable(idm), 0);
> +	KUNIT_EXPECT_EQ(test, idm_used_total(idm), 2);
> +
> +	idm_release_chunk_locked(idm, id, 2);
> +	KUNIT_EXPECT_EQ(test, idm_used_total(idm), 0);
> +
> +	mutex_unlock(idm_mutex(idm));
> +}
> +
> +static void check_used_shareable(struct kunit *test)
> +{
> +	struct xe_guc_id_mgr *idm = test->priv;
> +	unsigned int shareable_start;
> +	int id;
> +
> +	KUNIT_ASSERT_EQ(test, 0, idm_init(idm, 8, 8));
> +
> +	mutex_lock(idm_mutex(idm));
> +	shareable_start = idm_shareable_start(idm);
> +
> +	KUNIT_EXPECT_EQ(test, idm_used_usable(idm), 0);
> +	KUNIT_EXPECT_EQ(test, idm_used_shareable(idm), 0);
> +
> +	id = xe_guc_id_mgr_reserve_shareable_locked(idm, 3, 0);
> +	KUNIT_EXPECT_EQ(test, id, shareable_start);

[Severity: High]
Could the same out-of-bounds write happen here if id is a negative error
code? KUNIT_EXPECT_EQ() does not abort the test, so idm_release_chunk_locked()
would process a negative id.

> +
> +	KUNIT_EXPECT_EQ(test, idm_used_usable(idm), 0);
> +	KUNIT_EXPECT_EQ(test, idm_used_shareable(idm), 3);
> +	KUNIT_EXPECT_EQ(test, idm_used_total(idm), 3);
> +
> +	idm_release_chunk_locked(idm, id, 3);
> +	KUNIT_EXPECT_EQ(test, idm_used_total(idm), 0);
> +
> +	mutex_unlock(idm_mutex(idm));
> +}

[ ... ]

> +static void check_shareable_spare(struct kunit *test)
> +{
> +	struct xe_guc_id_mgr *idm = test->priv;
> +	unsigned int pools_size = (GUC_ID_MAX / 3) * 2;
> +	unsigned int shareable_start;
> +	unsigned int overlap_size;
> +	unsigned int shared_tail;
> +	int id_tail, id_overlap;
> +
> +	KUNIT_ASSERT_EQ(test, 0, idm_init(idm, pools_size, pools_size));
> +
> +	mutex_lock(idm_mutex(idm));
> +	shareable_start = idm_shareable_start(idm);
> +	overlap_size = idm->usable - shareable_start;
> +	shared_tail = idm_total(idm) - idm->usable;
> +
> +	KUNIT_EXPECT_GT(test, overlap_size, 0);
> +	KUNIT_EXPECT_GT(test, shared_tail, 0);
> +
> +	id_tail = xe_guc_id_mgr_reserve_shareable_locked(idm, shared_tail, 1);
> +	KUNIT_EXPECT_EQ(test, id_tail, idm->usable);

[Severity: High]
Similar to the above, if id_tail is assigned a negative error code upon failure,
it will eventually be passed to idm_release_chunk_locked() since KUNIT_EXPECT_EQ()
does not halt execution.

> +	KUNIT_EXPECT_EQ(test, idm_used_shareable(idm), shared_tail);
> +
> +	id_overlap = xe_guc_id_mgr_reserve_shareable_locked(idm, overlap_size - 1, 1);
> +	KUNIT_EXPECT_EQ(test, id_overlap, shareable_start + 1);

[Severity: High]
If id_overlap is negative, will this also result in an out-of-bounds write
when calling idm_release_chunk_locked() later in the function?

> +	KUNIT_EXPECT_EQ(test, idm_used_shareable(idm), idm->shareable - 1);
> +
> +	KUNIT_EXPECT_EQ(test, xe_guc_id_mgr_reserve_shareable_locked(idm, 1, 1), -EDQUOT);
> +	KUNIT_EXPECT_EQ(test, idm_used_shareable(idm), idm->shareable - 1);
> +
> +	idm_release_chunk_locked(idm, id_tail, shared_tail);
> +	idm_release_chunk_locked(idm, id_overlap, overlap_size - 1);
> +	KUNIT_EXPECT_EQ(test, idm_used_shareable(idm), 0);
> +
> +	mutex_unlock(idm_mutex(idm));
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2