Re: [PATCH 1/2] ubifs: add KUnit test scaffolding for scan.c

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Missing fatal assertion before list access causes out-of-bounds reads if memory allocation fails in KUnit test cases.
--

commit b83b8d90186f2d43276368aea89de87c7f3fd864
Author: lichen.wang <wanglichen9110@gmail.com>

ubifs: add KUnit test scaffolding for scan.c

This commit adds KUnit tests for the UBIFS scan path to validate node
parsing offline. It builds the tests into the ubifs module itself and adds
a new CONFIG_UBIFS_FS_KUNIT_TEST option to gate the build.

> diff --git a/fs/ubifs/tests/scan_kunit.c b/fs/ubifs/tests/scan_kunit.c
> new file mode 100644
> index 0000000000000..6871339cb899c
> --- /dev/null
> +++ b/fs/ubifs/tests/scan_kunit.c

[ ... ]

> +static void add_snod_pad_node_gets_invalid_key(struct kunit *test)
> +{
> +	struct ubifs_info *c = ubifs_test_info(test);
> +	struct ubifs_scan_leb *sleb;
> +	struct ubifs_scan_node *snod;
> +	struct ubifs_pad_node *pad;
> +
> +	KUNIT_ASSERT_NOT_NULL(test, c);
> +	sleb = ubifs_test_scan_leb(test);
> +	KUNIT_ASSERT_NOT_NULL(test, sleb);
> +	pad = ubifs_test_make_pad_node(test, 4);
> +	KUNIT_ASSERT_NOT_NULL(test, pad);
> +
> +	KUNIT_EXPECT_EQ(test, ubifs_add_snod(c, sleb, pad, 0x40), 0);
> +	KUNIT_EXPECT_EQ(test, sleb->nodes_cnt, 1);
> +
> +	snod = list_first_entry(&sleb->nodes, struct ubifs_scan_node, list);
> +	KUNIT_EXPECT_EQ(test, snod->type, UBIFS_PAD_NODE);

[Severity: Medium]
If ubifs_add_snod() fails to allocate a node with kmalloc(), it returns
-ENOMEM and leaves sleb->nodes empty. Since KUNIT_EXPECT_EQ() allows
execution to continue on failure, will list_first_entry() return a pointer
that aliases the list head inside the sleb structure?

Accessing snod->type could then cause an out-of-bounds read past the end
of the sleb allocation. Should this use KUNIT_ASSERT_EQ() instead to abort
the test early if the allocation fails?

> +	KUNIT_EXPECT_EQ(test, snod->key.u32[0], 0xDEADBEAF);
> +	KUNIT_EXPECT_EQ(test, snod->key.u32[1], UBIFS_INVALID_KEY);
> +
> +	free_scanned_nodes(sleb);
> +}
> +
> +static void add_snod_trun_node_gets_invalid_key(struct kunit *test)
> +{
> +	struct ubifs_info *c = ubifs_test_info(test);
> +	struct ubifs_scan_leb *sleb;
> +	struct ubifs_scan_node *snod;
> +	void *node;
> +
> +	KUNIT_ASSERT_NOT_NULL(test, c);
> +	sleb = ubifs_test_scan_leb(test);
> +	KUNIT_ASSERT_NOT_NULL(test, sleb);
> +	node = make_node(test, UBIFS_TRUN_NODE, UBIFS_TRUN_NODE_SZ);
> +	KUNIT_ASSERT_NOT_NULL(test, node);
> +
> +	KUNIT_EXPECT_EQ(test, ubifs_add_snod(c, sleb, node, 0), 0);
> +
> +	snod = list_first_entry(&sleb->nodes, struct ubifs_scan_node, list);
> +	KUNIT_EXPECT_EQ(test, snod->type, UBIFS_TRUN_NODE);

[Severity: Medium]
Similar to the padding test above, if ubifs_add_snod() fails, does this
cause an out-of-bounds read when accessing snod->type on the empty list?

> +	KUNIT_EXPECT_EQ(test, snod->key.u32[0], 0xDEADBEAF);
> +	KUNIT_EXPECT_EQ(test, snod->key.u32[1], UBIFS_INVALID_KEY);
> +
> +	free_scanned_nodes(sleb);
> +}
> +
> +static void add_snod_appends_nodes_in_order(struct kunit *test)
> +{
> +	struct ubifs_info *c = ubifs_test_info(test);
> +	struct ubifs_scan_leb *sleb;
> +	struct ubifs_scan_node *first, *second;
> +	void *node1, *node2;
> +
> +	KUNIT_ASSERT_NOT_NULL(test, c);
> +	sleb = ubifs_test_scan_leb(test);
> +	KUNIT_ASSERT_NOT_NULL(test, sleb);
> +	node1 = make_node(test, UBIFS_DATA_NODE, TEST_DATA_SZ);
> +	KUNIT_ASSERT_NOT_NULL(test, node1);
> +	node2 = make_node(test, UBIFS_INO_NODE, UBIFS_INO_NODE_SZ);
> +	KUNIT_ASSERT_NOT_NULL(test, node2);
> +
> +	KUNIT_EXPECT_EQ(test, ubifs_add_snod(c, sleb, node1, 0), 0);
> +	KUNIT_EXPECT_EQ(test, ubifs_add_snod(c, sleb, node2, 64), 0);
> +	KUNIT_EXPECT_EQ(test, sleb->nodes_cnt, 2);
> +
> +	first = list_first_entry(&sleb->nodes, struct ubifs_scan_node, list);
> +	second = list_next_entry(first, list);
> +	KUNIT_EXPECT_PTR_EQ(test, first->node, node1);
> +	KUNIT_EXPECT_PTR_EQ(test, second->node, node2);

[Severity: Medium]
If either call to ubifs_add_snod() fails, could fetching the node pointers
from the resulting invalid list entries result in out-of-bounds reads here
as well?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/178909365090.247910.11265261850560199708.p0-of-2@gmail.com?part=1