diff mbox series

[-v3,3/3] resource, kunit: Add test case for region_intersects()

Message ID 20240906030713.204292-4-ying.huang@intel.com (mailing list archive)
State New
Headers show
Series resource: Fix region_intersects() vs add_memory_driver_managed() | expand

Commit Message

Huang, Ying Sept. 6, 2024, 3:07 a.m. UTC 
region_intersects() is important because it's used for /dev/mem
permission checking.  To avoid possible bug of region_intersects() in
the future, a kunit test case for region_intersects() is added.

Signed-off-by: "Huang, Ying" <ying.huang@intel.com>
Cc: Dan Williams <dan.j.williams@intel.com>
Cc: David Hildenbrand <david@redhat.com>
Cc: Davidlohr Bueso <dave@stgolabs.net>
Cc: Jonathan Cameron <jonathan.cameron@huawei.com>
Cc: Dave Jiang <dave.jiang@intel.com>
Cc: Alison Schofield <alison.schofield@intel.com>
Cc: Vishal Verma <vishal.l.verma@intel.com>
Cc: Ira Weiny <ira.weiny@intel.com>
Cc: Alistair Popple <apopple@nvidia.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Bjorn Helgaas <bhelgaas@google.com>
Cc: Baoquan He <bhe@redhat.com>
---
 kernel/resource.c       |  20 ++++--
 kernel/resource_kunit.c | 143 ++++++++++++++++++++++++++++++++++++++++
 lib/Kconfig.debug       |   1 +
 3 files changed, 158 insertions(+), 6 deletions(-)

Comments

Kees Bakker Sept. 29, 2024, 7:45 p.m. UTC | #1 
Op 06-09-2024 om 05:07 schreef Huang Ying:
> [...]
> +static void resource_test_insert_resource(struct kunit *test, struct resource *parent,
> +					  resource_size_t start, resource_size_t size,
> +					  const char *name, unsigned long flags)
> +{
> +	struct resource *res;
> +
> +	res = kzalloc(sizeof(*res), GFP_KERNEL);
> +	KUNIT_ASSERT_NOT_NULL(test, res);
> +
> +	res->name = name;
> +	res->start = start;
> +	res->end = start + size - 1;
> +	res->flags = flags;
> +	if (insert_resource(parent, res)) {
> +		kfree(res);
> +		KUNIT_FAIL_AND_ABORT(test, "Fail to insert resource %pR\n", res);
Isn't this a user-after-free?
> +	}
> +
> +	kunit_add_action_or_reset(test, remove_free_resource, res);
> +}
>
Huang, Ying Sept. 30, 2024, 12:53 a.m. UTC | #2 
Kees Bakker <kees@ijzerbout.nl> writes:

> Op 06-09-2024 om 05:07 schreef Huang Ying:
>> [...]
>> +static void resource_test_insert_resource(struct kunit *test, struct resource *parent,
>> +					  resource_size_t start, resource_size_t size,
>> +					  const char *name, unsigned long flags)
>> +{
>> +	struct resource *res;
>> +
>> +	res = kzalloc(sizeof(*res), GFP_KERNEL);
>> +	KUNIT_ASSERT_NOT_NULL(test, res);
>> +
>> +	res->name = name;
>> +	res->start = start;
>> +	res->end = start + size - 1;
>> +	res->flags = flags;
>> +	if (insert_resource(parent, res)) {
>> +		kfree(res);
>> +		KUNIT_FAIL_AND_ABORT(test, "Fail to insert resource %pR\n", res);
> Isn't this a user-after-free?

Good catch!  Thanks for pointing this out.  I should be more careful for
the error path.  
>> +	}
>> +
>> +	kunit_add_action_or_reset(test, remove_free_resource, res);

This may cause use-after-free if failed to allocate memory for
add_action.  Will fix this too.

>> +}
>>

--
Best Regards,
Huang, Ying
Kees Bakker Sept. 30, 2024, 5:36 p.m. UTC | #3 
Op 30-09-2024 om 02:53 schreef Huang, Ying:
> Kees Bakker <kees@ijzerbout.nl> writes:
>
>> Op 06-09-2024 om 05:07 schreef Huang Ying:
>>> [...]
>>> +static void resource_test_insert_resource(struct kunit *test, struct resource *parent,
>>> +					  resource_size_t start, resource_size_t size,
>>> +					  const char *name, unsigned long flags)
>>> +{
>>> +	struct resource *res;
>>> +
>>> +	res = kzalloc(sizeof(*res), GFP_KERNEL);
>>> +	KUNIT_ASSERT_NOT_NULL(test, res);
>>> +
>>> +	res->name = name;
>>> +	res->start = start;
>>> +	res->end = start + size - 1;
>>> +	res->flags = flags;
>>> +	if (insert_resource(parent, res)) {
>>> +		kfree(res);
>>> +		KUNIT_FAIL_AND_ABORT(test, "Fail to insert resource %pR\n", res);
>> Isn't this a user-after-free?
> Good catch!  Thanks for pointing this out.  I should be more careful for
> the error path.
I have to honest, it wasn't me who found this. It was Coverity.
>>> +	}
>>> +
>>> +	kunit_add_action_or_reset(test, remove_free_resource, res);
> This may cause use-after-free if failed to allocate memory for
> add_action.  Will fix this too.
>
>>> +}
>>>
> --
> Best Regards,
> Huang, Ying
diff mbox series

Patch

diff --git a/kernel/resource.c b/kernel/resource.c
index 035ef16c1a66..e6d222fb0f84 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -1860,7 +1860,17 @@  EXPORT_SYMBOL(resource_list_free);
 #ifdef CONFIG_GET_FREE_REGION
 #define GFR_DESCENDING		(1UL << 0)
 #define GFR_REQUEST_REGION	(1UL << 1)
-#define GFR_DEFAULT_ALIGN (1UL << PA_SECTION_SHIFT)
+#ifdef PA_SECTION_SHIFT
+#define GFR_DEFAULT_ALIGN	(1UL << PA_SECTION_SHIFT)
+#else
+#define GFR_DEFAULT_ALIGN	PAGE_SIZE
+#endif
+
+#ifdef MAX_PHYSMEM_BITS
+#define MAX_PHYS_ADDR		((1ULL << MAX_PHYSMEM_BITS) - 1)
+#else
+#define MAX_PHYS_ADDR		(-1ULL)
+#endif
 
 static resource_size_t gfr_start(struct resource *base, resource_size_t size,
 				 resource_size_t align, unsigned long flags)
@@ -1868,8 +1878,7 @@  static resource_size_t gfr_start(struct resource *base, resource_size_t size,
 	if (flags & GFR_DESCENDING) {
 		resource_size_t end;
 
-		end = min_t(resource_size_t, base->end,
-			    (1ULL << MAX_PHYSMEM_BITS) - 1);
+		end = min_t(resource_size_t, base->end, MAX_PHYS_ADDR);
 		return end - size + 1;
 	}
 
@@ -1886,8 +1895,7 @@  static bool gfr_continue(struct resource *base, resource_size_t addr,
 	 * @size did not wrap 0.
 	 */
 	return addr > addr - size &&
-	       addr <= min_t(resource_size_t, base->end,
-			     (1ULL << MAX_PHYSMEM_BITS) - 1);
+		addr <= min_t(resource_size_t, base->end, MAX_PHYS_ADDR);
 }
 
 static resource_size_t gfr_next(resource_size_t addr, resource_size_t size,
@@ -2048,7 +2056,7 @@  struct resource *alloc_free_mem_region(struct resource *base,
 	return get_free_mem_region(NULL, base, size, align, name,
 				   IORES_DESC_NONE, flags);
 }
-EXPORT_SYMBOL_NS_GPL(alloc_free_mem_region, CXL);
+EXPORT_SYMBOL_GPL(alloc_free_mem_region);
 #endif /* CONFIG_GET_FREE_REGION */
 
 static int __init strict_iomem(char *str)
diff --git a/kernel/resource_kunit.c b/kernel/resource_kunit.c
index 0e509985a44a..42d2d8d20f5d 100644
--- a/kernel/resource_kunit.c
+++ b/kernel/resource_kunit.c
@@ -7,6 +7,8 @@ 
 #include <linux/ioport.h>
 #include <linux/kernel.h>
 #include <linux/string.h>
+#include <linux/sizes.h>
+#include <linux/mm.h>
 
 #define R0_START	0x0000
 #define R0_END		0xffff
@@ -137,9 +139,150 @@  static void resource_test_intersection(struct kunit *test)
 	} while (++i < ARRAY_SIZE(results_for_intersection));
 }
 
+/*
+ * The test resource tree for region_intersects() test:
+ *
+ * BASE-BASE+1M-1 : Test System RAM 0
+ *		  # hole 0 (BASE+1M-BASE+2M)
+ * BASE+2M-BASE+3M-1 : Test CXL Window 0
+ * BASE+3M-BASE+4M-1 : Test System RAM 1
+ * BASE+4M-BASE+7M-1 : Test CXL Window 1
+ *   BASE+4M-BASE+5M-1 : Test System RAM 2
+ *     BASE+4M+128K-BASE+4M+256K-1: Test Code
+ *   BASE+5M-BASE+6M-1 : Test System RAM 3
+ */
+#define RES_TEST_RAM0_OFFSET	0
+#define RES_TEST_RAM0_SIZE	SZ_1M
+#define RES_TEST_HOLE0_OFFSET	(RES_TEST_RAM0_OFFSET + RES_TEST_RAM0_SIZE)
+#define RES_TEST_HOLE0_SIZE	SZ_1M
+#define RES_TEST_WIN0_OFFSET	(RES_TEST_HOLE0_OFFSET + RES_TEST_HOLE0_SIZE)
+#define RES_TEST_WIN0_SIZE	SZ_1M
+#define RES_TEST_RAM1_OFFSET	(RES_TEST_WIN0_OFFSET + RES_TEST_WIN0_SIZE)
+#define RES_TEST_RAM1_SIZE	SZ_1M
+#define RES_TEST_WIN1_OFFSET	(RES_TEST_RAM1_OFFSET + RES_TEST_RAM1_SIZE)
+#define RES_TEST_WIN1_SIZE	(SZ_1M * 3)
+#define RES_TEST_RAM2_OFFSET	RES_TEST_WIN1_OFFSET
+#define RES_TEST_RAM2_SIZE	SZ_1M
+#define RES_TEST_CODE_OFFSET	(RES_TEST_RAM2_OFFSET + SZ_128K)
+#define RES_TEST_CODE_SIZE	SZ_128K
+#define RES_TEST_RAM3_OFFSET	(RES_TEST_RAM2_OFFSET + RES_TEST_RAM2_SIZE)
+#define RES_TEST_RAM3_SIZE	SZ_1M
+#define RES_TEST_TOTAL_SIZE	((RES_TEST_WIN1_OFFSET + RES_TEST_WIN1_SIZE))
+
+static void remove_free_resource(void *ctx)
+{
+	struct resource *res = (struct resource *)ctx;
+
+	remove_resource(res);
+	kfree(res);
+}
+
+static void resource_test_request_region(struct kunit *test, struct resource *parent,
+					 resource_size_t start, resource_size_t size,
+					 const char *name, unsigned long flags)
+{
+	struct resource *res;
+
+	res = __request_region(parent, start, size, name, flags);
+	KUNIT_ASSERT_NOT_NULL(test, res);
+	kunit_add_action_or_reset(test, remove_free_resource, res);
+}
+
+static void resource_test_insert_resource(struct kunit *test, struct resource *parent,
+					  resource_size_t start, resource_size_t size,
+					  const char *name, unsigned long flags)
+{
+	struct resource *res;
+
+	res = kzalloc(sizeof(*res), GFP_KERNEL);
+	KUNIT_ASSERT_NOT_NULL(test, res);
+
+	res->name = name;
+	res->start = start;
+	res->end = start + size - 1;
+	res->flags = flags;
+	if (insert_resource(parent, res)) {
+		kfree(res);
+		KUNIT_FAIL_AND_ABORT(test, "Fail to insert resource %pR\n", res);
+	}
+
+	kunit_add_action_or_reset(test, remove_free_resource, res);
+}
+
+static void resource_test_region_intersects(struct kunit *test)
+{
+	unsigned long flags =  IORESOURCE_SYSTEM_RAM | IORESOURCE_BUSY;
+	struct resource *parent;
+	resource_size_t start;
+
+	/* Find an iomem_resource hole to hold test resources */
+	parent = alloc_free_mem_region(&iomem_resource, RES_TEST_TOTAL_SIZE, SZ_1M,
+				       "test resources");
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, parent);
+	start = parent->start;
+	kunit_add_action_or_reset(test, remove_free_resource, parent);
+
+	resource_test_request_region(test, parent, start + RES_TEST_RAM0_OFFSET,
+				     RES_TEST_RAM0_SIZE, "Test System RAM 0", flags);
+	resource_test_insert_resource(test, parent, start + RES_TEST_WIN0_OFFSET,
+				      RES_TEST_WIN0_SIZE, "Test CXL Window 0",
+				      IORESOURCE_MEM);
+	resource_test_request_region(test, parent, start + RES_TEST_RAM1_OFFSET,
+				     RES_TEST_RAM1_SIZE, "Test System RAM 1", flags);
+	resource_test_insert_resource(test, parent, start + RES_TEST_WIN1_OFFSET,
+				      RES_TEST_WIN1_SIZE, "Test CXL Window 1",
+				      IORESOURCE_MEM);
+	resource_test_request_region(test, parent, start + RES_TEST_RAM2_OFFSET,
+				     RES_TEST_RAM2_SIZE, "Test System RAM 2", flags);
+	resource_test_insert_resource(test, parent, start + RES_TEST_CODE_OFFSET,
+				      RES_TEST_CODE_SIZE, "Test Code", flags);
+	resource_test_request_region(test, parent, start + RES_TEST_RAM3_OFFSET,
+				     RES_TEST_RAM3_SIZE, "Test System RAM 3", flags);
+	kunit_release_action(test, remove_free_resource, parent);
+
+	KUNIT_EXPECT_EQ(test, REGION_INTERSECTS,
+			region_intersects(start + RES_TEST_RAM0_OFFSET, PAGE_SIZE,
+					  IORESOURCE_SYSTEM_RAM, IORES_DESC_NONE));
+	KUNIT_EXPECT_EQ(test, REGION_INTERSECTS,
+			region_intersects(start + RES_TEST_RAM0_OFFSET +
+					  RES_TEST_RAM0_SIZE - PAGE_SIZE, 2 * PAGE_SIZE,
+					  IORESOURCE_SYSTEM_RAM, IORES_DESC_NONE));
+	KUNIT_EXPECT_EQ(test, REGION_DISJOINT,
+			region_intersects(start + RES_TEST_HOLE0_OFFSET, PAGE_SIZE,
+					  IORESOURCE_SYSTEM_RAM, IORES_DESC_NONE));
+	KUNIT_EXPECT_EQ(test, REGION_DISJOINT,
+			region_intersects(start + RES_TEST_HOLE0_OFFSET +
+					  RES_TEST_HOLE0_SIZE - PAGE_SIZE, 2 * PAGE_SIZE,
+					  IORESOURCE_SYSTEM_RAM, IORES_DESC_NONE));
+	KUNIT_EXPECT_EQ(test, REGION_MIXED,
+			region_intersects(start + RES_TEST_WIN0_OFFSET +
+					  RES_TEST_WIN0_SIZE - PAGE_SIZE, 2 * PAGE_SIZE,
+					  IORESOURCE_SYSTEM_RAM, IORES_DESC_NONE));
+	KUNIT_EXPECT_EQ(test, REGION_INTERSECTS,
+			region_intersects(start + RES_TEST_RAM1_OFFSET +
+					  RES_TEST_RAM1_SIZE - PAGE_SIZE, 2 * PAGE_SIZE,
+					  IORESOURCE_SYSTEM_RAM, IORES_DESC_NONE));
+	KUNIT_EXPECT_EQ(test, REGION_INTERSECTS,
+			region_intersects(start + RES_TEST_RAM2_OFFSET +
+					  RES_TEST_RAM2_SIZE - PAGE_SIZE, 2 * PAGE_SIZE,
+					  IORESOURCE_SYSTEM_RAM, IORES_DESC_NONE));
+	KUNIT_EXPECT_EQ(test, REGION_INTERSECTS,
+			region_intersects(start + RES_TEST_CODE_OFFSET, PAGE_SIZE,
+					  IORESOURCE_SYSTEM_RAM, IORES_DESC_NONE));
+	KUNIT_EXPECT_EQ(test, REGION_INTERSECTS,
+			region_intersects(start + RES_TEST_RAM2_OFFSET,
+					  RES_TEST_RAM2_SIZE + PAGE_SIZE,
+					  IORESOURCE_SYSTEM_RAM, IORES_DESC_NONE));
+	KUNIT_EXPECT_EQ(test, REGION_MIXED,
+			region_intersects(start + RES_TEST_RAM3_OFFSET,
+					  RES_TEST_RAM3_SIZE + PAGE_SIZE,
+					  IORESOURCE_SYSTEM_RAM, IORES_DESC_NONE));
+}
+
 static struct kunit_case resource_test_cases[] = {
 	KUNIT_CASE(resource_test_union),
 	KUNIT_CASE(resource_test_intersection),
+	KUNIT_CASE(resource_test_region_intersects),
 	{}
 };
 
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index a30c03a66172..383453cbf4af 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -2616,6 +2616,7 @@  config RESOURCE_KUNIT_TEST
 	tristate "KUnit test for resource API" if !KUNIT_ALL_TESTS
 	depends on KUNIT
 	default KUNIT_ALL_TESTS
+	select GET_FREE_REGION
 	help
 	  This builds the resource API unit test.
 	  Tests the logic of API provided by resource.c and ioport.h.