[PATCH] resource: use kstrdup_const to prevent wild pointer issues

[PATCH] resource: use kstrdup_const to prevent wild pointer issues

[kingdix10]

From: King Dix <kingdix10@qq.com>

When a stack string variable is passed during the request resource
operation, it causes an oops problem when executing cat /proc/iomem.

In the original code, in functions like __request_region_locked, the name
member of the resource structure was directly assigned the stack string
pointer without proper memory management.

This fix changes the assignment of res->name to use kstrdup_const for
string copying, ensuring the correct storage and release of the string
and thus avoiding potential memory errors and oops issues.

Signed-off-by: King Dix <kingdix10@qq.com>
---
 kernel/resource.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/kernel/resource.c b/kernel/resource.c
index b7c0e24d9398..87d22741c066 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -168,8 +168,10 @@ static void free_resource(struct resource *res)
 	 * buddy and trying to be smart and reusing them eventually in
 	 * alloc_resource() overcomplicates resource handling.
 	 */
-	if (res && PageSlab(virt_to_head_page(res)))
+	if (res && PageSlab(virt_to_head_page(res))) {
+		kfree_const(res->name);
 		kfree(res);
+	}
 }
 
 static struct resource *alloc_resource(gfp_t flags)
@@ -1098,7 +1100,7 @@ __reserve_region_with_split(struct resource *root, resource_size_t start,
 	if (!res)
 		return;
 
-	res->name = name;
+	res->name = kstrdup_const(name, GFP_ATOMIC);
 	res->start = start;
 	res->end = end;
 	res->flags = type | IORESOURCE_BUSY;
@@ -1133,7 +1135,7 @@ __reserve_region_with_split(struct resource *root, resource_size_t start,
 					free_resource(res);
 					break;
 				}
-				next_res->name = name;
+				next_res->name = kstrdup_const(name, GFP_ATOMIC);
 				next_res->start = conflict->end + 1;
 				next_res->end = end;
 				next_res->flags = type | IORESOURCE_BUSY;
@@ -1261,7 +1263,7 @@ static int __request_region_locked(struct resource *res, struct resource *parent
 {
 	DECLARE_WAITQUEUE(wait, current);
 
-	res->name = name;
+	res->name = kstrdup_const(name, GFP_KERNEL);
 	res->start = start;
 	res->end = start + n - 1;
 
@@ -1474,7 +1476,7 @@ void release_mem_region_adjustable(resource_size_t start, resource_size_t size)
 					goto retry;
 				}
 			}
-			new_res->name = res->name;
+			new_res->name = kstrdup_const(res->name, GFP_ATOMIC);
 			new_res->start = end + 1;
 			new_res->end = res->end;
 			new_res->flags = res->flags;
@@ -1978,7 +1980,7 @@ get_free_mem_region(struct device *dev, struct resource *base,
 		} else {
 			res->start = addr;
 			res->end = addr + size - 1;
-			res->name = name;
+			res->name = kstrdup_const(name, GFP_KERNEL);
 			res->desc = desc;
 			res->flags = IORESOURCE_MEM;
 
-- 
2.43.0

Re: [PATCH] resource: use kstrdup_const to prevent wild pointer issues

[Huang, Ying]

kingdix10@qq.com writes:

> From: King Dix <kingdix10@qq.com>
>
> When a stack string variable is passed during the request resource
> operation, it causes an oops problem when executing cat /proc/iomem.
>
> In the original code, in functions like __request_region_locked, the name
> member of the resource structure was directly assigned the stack string
> pointer without proper memory management.
>
> This fix changes the assignment of res->name to use kstrdup_const for
> string copying, ensuring the correct storage and release of the string
> and thus avoiding potential memory errors and oops issues.
>
> Signed-off-by: King Dix <kingdix10@qq.com>

In general, I think that it's good to improve the resource requesting
API.  However, it's not good to use so many GFP_ATOMIC too.  Why do you
need to call resource requesting API with stack variable?  If it's just
some programming bugs, we should add more checks instead of hiding the
bugs.  For example, if we only allows kernel rodata and slab memory to be
used in resource requesting.  We can add a VM_WARN_ON() to check that.

[snip]

---
Best Regards,
Huang, Ying

Re: [PATCH] resource: use kstrdup_const to prevent wild pointer issues

[Andrew Morton]

On Thu, 02 Jan 2025 09:59:26 +0800 "Huang, Ying" <ying.huang@linux.alibaba.com> wrote:

> > From: King Dix <kingdix10@qq.com>
> >
> > When a stack string variable is passed during the request resource
> > operation, it causes an oops problem when executing cat /proc/iomem.
> >
> > In the original code, in functions like __request_region_locked, the name
> > member of the resource structure was directly assigned the stack string
> > pointer without proper memory management.
> >
> > This fix changes the assignment of res->name to use kstrdup_const for
> > string copying, ensuring the correct storage and release of the string
> > and thus avoiding potential memory errors and oops issues.
> >
> > Signed-off-by: King Dix <kingdix10@qq.com>
> 
> In general, I think that it's good to improve the resource requesting
> API.  However, it's not good to use so many GFP_ATOMIC too.  Why do you
> need to call resource requesting API with stack variable?  If it's just
> some programming bugs, we should add more checks instead of hiding the
> bugs.  For example, if we only allows kernel rodata and slab memory to be
> used in resource requesting.  We can add a VM_WARN_ON() to check that.

I agree.  It may not be a very good idea, but request_region() requires
that the caller pass in a `name' string which is permanently available.

__request_region() kerneldoc doesn't document this, and it should.

Because of this present interface design, calling request_region() with
an on-stack string must be considered a bug in the calling code.

Re: [PATCH] resource: use kstrdup_const to prevent wild pointer issues

[kernel test robot]

Hello,

kernel test robot noticed "BUG:sleeping_function_called_from_invalid_context_at_include/linux/sched/mm.h" on:

commit: 85f3f9e1bca4488dcf0032c11c2d0b59352971a0 ("[PATCH] resource: use kstrdup_const to prevent wild pointer issues")
url: https://github.com/intel-lab-lkp/linux/commits/kingdix10-qq-com/resource-use-kstrdup_const-to-prevent-wild-pointer-issues/20250101-215639
base: https://git.kernel.org/cgit/linux/kernel/git/akpm/mm.git mm-everything
patch link: https://lore.kernel.org/all/tencent_3BD5C192A0E62121DE44D1A05A9C9ECB7C06@qq.com/
patch subject: [PATCH] resource: use kstrdup_const to prevent wild pointer issues

in testcase: boot

config: x86_64-randconfig-161-20250102
compiler: gcc-12
test machine: qemu-system-x86_64 -enable-kvm -cpu SandyBridge -smp 2 -m 16G

(please refer to attached dmesg/kmsg for entire log/backtrace)


+-------------------------------------------------------------------------------+------------+------------+
|                                                                               | d77847ea04 | 85f3f9e1bc |
+-------------------------------------------------------------------------------+------------+------------+
| BUG:sleeping_function_called_from_invalid_context_at_include/linux/sched/mm.h | 0          | 18         |
+-------------------------------------------------------------------------------+------------+------------+


If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <oliver.sang@intel.com>
| Closes: https://lore.kernel.org/oe-lkp/202501031658.51df654c-lkp@intel.com


[   24.398603][    T1] BUG: sleeping function called from invalid context at include/linux/sched/mm.h:321
[   24.399830][    T1] in_atomic(): 1, irqs_disabled(): 0, non_block: 0, pid: 1, name: swapper/0
[   24.400487][    T1] preempt_count: 1, expected: 0
[   24.400919][    T1] RCU nest depth: 0, expected: 0
[   24.401295][    T1] 2 locks held by swapper/0/1:
[ 24.401659][ T1] #0: ffff8881003600f0 (&dev->mutex){....}-{4:4}, at: __driver_attach (drivers/base/dd.c:1216 drivers/base/dd.c:1156)
[ 24.402502][ T1] #1: ffffffff95689c58 (resource_lock){++++}-{3:3}, at: __request_region (kernel/resource.c:1264 kernel/resource.c:1330)
[   24.403255][    T1] CPU: 0 UID: 0 PID: 1 Comm: swapper/0 Tainted: G        W       T  6.13.0-rc4-00416-g85f3f9e1bca4 #2
[   24.404161][    T1] Tainted: [W]=WARN, [T]=RANDSTRUCT
[   24.404599][    T1] Call Trace:
[   24.404862][    T1]  <TASK>
[ 24.404862][ T1] dump_stack_lvl (lib/dump_stack.c:123)
[ 24.404862][ T1] dump_stack (lib/dump_stack.c:130)
[ 24.404862][ T1] __might_resched (kernel/sched/core.c:8767)
[ 24.404862][ T1] slab_alloc_node+0x12d/0x1b0
[ 24.404862][ T1] ? kstrdup_const (mm/util.c:101)
[ 24.404862][ T1] __kmalloc_node_track_caller_noprof (mm/slub.c:4293 mm/slub.c:4313)
[ 24.404862][ T1] kstrdup (mm/util.c:61 mm/util.c:81)
[ 24.404862][ T1] kstrdup_const (mm/util.c:101)
[ 24.404862][ T1] __request_region (kernel/resource.c:1266 kernel/resource.c:1330)
[ 24.404862][ T1] ? device_register (drivers/base/core.c:3748)
[ 24.404862][ T1] ? wake_up_q (kernel/sched/core.c:7088)
[ 24.404862][ T1] __parport_pc_probe_port (drivers/parport/parport_pc.c:2073)
[ 24.404862][ T1] ? __dev_printk (drivers/base/core.c:4962)
[ 24.404862][ T1] ? _dev_info (drivers/base/core.c:5004)
[ 24.404862][ T1] ? pnp_device_probe (drivers/pnp/driver.c:95)
[ 24.404862][ T1] parport_pc_pnp_probe (drivers/parport/parport_pc.c:2917)
[ 24.404862][ T1] parport_pc_pnp_probe (drivers/parport/parport_pc.c:3039)
[ 24.404862][ T1] pnp_device_probe (drivers/pnp/driver.c:111)
[ 24.404862][ T1] really_probe (drivers/base/dd.c:579 drivers/base/dd.c:658)
[ 24.404862][ T1] __driver_probe_device (drivers/base/dd.c:800)
[ 24.404862][ T1] driver_probe_device (drivers/base/dd.c:830)
[ 24.404862][ T1] __driver_attach (drivers/base/dd.c:1217 drivers/base/dd.c:1156)
[ 24.404862][ T1] ? __device_attach_driver (drivers/base/dd.c:1157)
[ 24.404862][ T1] bus_for_each_dev (drivers/base/bus.c:369)
[ 24.404862][ T1] driver_attach (drivers/base/dd.c:1235)
[ 24.404862][ T1] bus_add_driver (drivers/base/bus.c:675)
[ 24.404862][ T1] driver_register (drivers/base/driver.c:246)
[ 24.404862][ T1] pnp_register_driver (drivers/pnp/driver.c:281)
[ 24.404862][ T1] parport_pc_find_ports (drivers/parport/parport_pc.c:3118)
[ 24.404862][ T1] ? driver_register (drivers/base/driver.c:258)
[ 24.404862][ T1] parport_pc_init (drivers/parport/parport_pc.c:3380)
[ 24.404862][ T1] ? parport_pc_find_ports (drivers/parport/parport_pc.c:3354)
[ 24.404862][ T1] do_one_initcall (init/main.c:1266)
[ 24.404862][ T1] ? rcu_is_watching (arch/x86/include/asm/atomic.h:23 include/linux/atomic/atomic-arch-fallback.h:457 include/linux/context_tracking.h:128 kernel/rcu/tree.c:737)
[ 24.404862][ T1] do_initcalls (init/main.c:1327 init/main.c:1344)
[ 24.404862][ T1] ? rdinit_setup (init/main.c:1314)
[ 24.404862][ T1] ? rest_init (init/main.c:1458)
[ 24.404862][ T1] kernel_init_freeable (init/main.c:1581)
[ 24.404862][ T1] kernel_init (init/main.c:1468)
[ 24.404862][ T1] ret_from_fork (arch/x86/kernel/process.c:153)
[ 24.404862][ T1] ? rest_init (init/main.c:1458)
[ 24.404862][ T1] ret_from_fork_asm (arch/x86/entry/entry_64.S:254)
[   24.404862][    T1]  </TASK>


The kernel config and materials to reproduce are available at:
https://download.01.org/0day-ci/archive/20250103/202501031658.51df654c-lkp@intel.com



-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki