Re: [PATCH] mm: fix potential invalid pointer dereference in kmemdup()

[Xujun Leng]

>On 09.03.23 11:04, Xujun Leng wrote:
>>> On 09.03.23 07:46, Xujun Leng wrote:
>>>>> On 07.03.23 10:03, Xujun Leng wrote:
>>>>>> If kmemdup() was called with src == NULL, then memcpy() source address
>>>>>> is fatal, and if kmemdup() was called with len == 0, kmalloc_track_caller()
>>>>>> will return ZERO_SIZE_PTR to variable p, then memcpy() destination address
>>>>>> is fatal. Both 2 cases will cause an invalid pointer dereference.
>>>>>>
>>>>
>>>>> "fix" in subject implies that there is actually a case broken. Is there,
>>>>> or is this rather a "sanitize" ?
>>>> Yes, I agree that word "sanitize" is a better choice.
>>>> And no, I don't find an actually case but in my test code as follow:
>>>>
>>>> #include <linux/module.h>
>>>> #include <linux/string.h>
>>>> #include <linux/slab.h>
>>>> #include <linux/printk.h>
>>>> #include <linux/err.h>
>>>>
>>>> /*
>>>>    * Test cases for kmemdup() and memdup_user().
>>>>    */
>>>> enum {
>>>> 	TC_KMEMDUP_ARG0_NULL, /* i.e. kmemdup(NULL, 5, GFP_KERNEL) */
>>>> 	TC_KMEMDUP_ARG1_ZERO, /* i.e. kmemdup("12345", 0, GFP_KERNEL) */
>>>>
>>>> 	TC_MEMDUP_USER_ARG0_NULL, /* i.e. memdup_user(NULL, 5) */
>>>> 	TC_MEMDUP_USER_ARG1_ZERO  /* i.e. memdup_user("12345", 0) */
>>>> };
>>>>
>>>> static int test_case;
>>>> static const char *test_func_name[] = {"kmemdup", "memdup_user"};
>>>> static void *ptr;
>>>>
>>>> module_param(test_case, int, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
>>>>
>>>> static void *kmemdup_arg0_null(void)
>>>> {
>>>> 	return kmemdup(NULL, 5, GFP_KERNEL);
>>>> }
>>>>
>>>> static void *kmemdup_arg1_zero(void)
>>>> {
>>>> 	return kmemdup("12345", 0, GFP_KERNEL);
>>>> }
>>>>
>>>> static void *memdup_user_arg0_null(void)
>>>> {
>>>> 	return memdup_user(NULL, 5);
>>>> }
>>>>
>>>> static void *memdup_user_arg1_zero(void)
>>>> {
>>>> 	return memdup_user("12345", 0);
>>>> }
>>>>
>>>> static int check_ptr(void)
>>>> {
>>>> 	if (ZERO_OR_NULL_PTR(ptr)) {
>>>> 		printk(KERN_ERR "test case %d: %s failed, PTR_ERR(ptr) = %ld\n",
>>>> 				test_case, test_func_name[test_case / 2], PTR_ERR(ptr));
>>>> 		return -EINVAL;
>>>> 	}
>>>> 	
>>>> 	if (IS_ERR(ptr)) {
>>>> 		printk(KERN_ERR "test case %d: %s failed, PTR_ERR(ptr) = %ld\n",
>>>> 				test_case, test_func_name[test_case / 2], PTR_ERR(ptr));
>>>> 		return PTR_ERR(ptr);
>>>> 	}
>>>>
>>>> 	printk(KERN_INFO "mm-util test module loaded.\n");
>>>>
>>>> 	return 0;
>>>> }
>>>>
>>>> static int __init memdup_user_test_init(void)
>>>> {
>>>> 	if (test_case < 0 || test_case > TC_MEMDUP_USER_ARG1_ZERO) {
>>>> 		printk(KERN_INFO "invalid test case %d\n", test_case);
>>>> 		return -EINVAL;
>>>> 	}
>>>>
>>>> 	printk(KERN_INFO "test case: %d\n", test_case);
>>>>
>>>> 	switch (test_case) {
>>>> 	case TC_KMEMDUP_ARG0_NULL:
>>>> 		ptr = kmemdup_arg0_null();
>>>> 		break;
>>>> 	case TC_KMEMDUP_ARG1_ZERO:
>>>> 		ptr = kmemdup_arg1_zero();
>>>> 		break;
>>>>
>>>> 	case TC_MEMDUP_USER_ARG0_NULL:
>>>> 		ptr = memdup_user_arg0_null();
>>>> 		break;
>>>>
>>>> 	case TC_MEMDUP_USER_ARG1_ZERO:
>>>> 		ptr = memdup_user_arg1_zero();
>>>> 		break;
>>>> 	
>>>> 	default:
>>>> 		/* should be never happend */
>>>> 		ptr = NULL;
>>>> 		break;
>>>> 	}
>>>>
>>>> 	return check_ptr();
>>>> }
>>>>
>>>> static void __exit memdup_user_test_exit(void)
>>>> {
>>>> 	if (ptr) {
>>>> 		kfree(ptr);
>>>> 		ptr = NULL;
>>>> 	}
>>>>
>>>> 	printk(KERN_INFO "mm-util test module exited.\n");
>>>> }
>>>>
>>>> module_init(memdup_user_test_init);
>>>> module_exit(memdup_user_test_exit);
>>>>
>>>> MODULE_LICENSE("GPL");
>>>>
>>>> Build the code as module, and run the module in QEMU ARM64, with different
>>>> test case(pass 0,1,2,3 to moddule parameter "test_case"), get follow the
>>>> results:
>>>>
>>>> root@qemu-ubuntu:~# modprobe memdup_kernel_user_test test_case=0
>>>> [  142.979506] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
>>>> [  142.983171] Mem abort info:
>>>> [  142.984049]   ESR = 0x0000000096000004
>>>> [  142.984556]   EC = 0x25: DABT (current EL), IL = 32 bits
>>>> [  142.985327]   SET = 0, FnV = 0
>>>> [  142.986867]   EA = 0, S1PTW = 0
>>>> [  142.987198]   FSC = 0x04: level 0 translation fault
>>>> [  142.987555] Data abort info:
>>>> [  142.987819]   ISV = 0, ISS = 0x00000004
>>>> [  142.988132]   CM = 0, WnR = 0
>>>> [  142.988540] user pgtable: 4k pages, 48-bit VAs, pgdp=0000000046168000
>>>> [  142.989715] [0000000000000000] pgd=0000000000000000, p4d=0000000000000000
>>>> [  142.992158] Internal error: Oops: 0000000096000004 [#1] PREEMPT SMP
>>>> [  142.993012] Modules linked in: memdup_kernel_user_test(+) drm ip_tables x_tables ipv6
>>>> [  142.996663] CPU: 0 PID: 133 Comm: modprobe Not tainted 6.3.0-rc1-next-20230307-dirty #1
>>>> [  143.002024] Hardware name: linux,dummy-virt (DT)
>>>> [  143.003370] pstate: 80000005 (Nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>>>> [  143.005461] pc : __memcpy+0x54/0x230
>>>> [  143.006833] lr : kmemdup+0x50/0x68
>>>> [  143.007208] sp : ffff80000aa53ae0
>>>> [  143.011440] x29: ffff80000aa53ae0 x28: ffff8000010c0378 x27: ffff8000010c0058
>>>> [  143.012386] x26: ffff80000a216fd8 x25: ffff80000aa53d00 x24: ffff8000010c0040
>>>> [  143.014183] x23: 0000000000000000 x22: ffff0000037d6580 x21: 0000000000000000
>>>> [  143.018590] x20: 0000000000000005 x19: ffff0000039a9100 x18: 0000000000000001
>>>> [  143.020166] x17: ffff80000aa75000 x16: ffff0000047bed91 x15: ffff0000037d69f8
>>>> [  143.021158] x14: 0000000000000147 x13: ffff0000037d69f8 x12: 00000000ffffffea
>>>> [  143.024978] x11: 00000000ffffefff x10: 00000000ffffefff x9 : ffff80000a1fb518
>>>> [  143.025800] x8 : 00000000ffffffff x7 : 00000000ffffffff x6 : ffff800036288000
>>>> [  143.026667] x5 : ffff0000039a9105 x4 : 0000000000000005 x3 : 0000000080200020
>>>> [  143.027257] x2 : 0000000000000005 x1 : 0000000000000000 x0 : ffff0000039a9100
>>>> [  143.028177] Call trace:
>>>> [  143.028833]  __memcpy+0x54/0x230
>>>> [  143.029424]  memdup_user_test_init+0xd8/0x1000 [memdup_kernel_user_test]
>>>> [  143.032466]  do_one_initcall+0x70/0x1b4
>>>> [  143.038282]  do_init_module+0x58/0x1e8
>>>> [  143.039354]  load_module+0x181c/0x1920
>>>> [  143.040919]  __do_sys_finit_module+0xb8/0x10c
>>>> [  143.041558]  __arm64_sys_finit_module+0x20/0x2c
>>>> [  143.044052]  invoke_syscall+0x44/0x104
>>>> [  143.044663]  el0_svc_common.constprop.0+0x44/0xec
>>>> [  143.045562]  do_el0_svc+0x38/0x98
>>>> [  143.047935]  el0_svc+0x2c/0x84
>>>> [  143.048175]  el0t_64_sync_handler+0xb8/0xbc
>>>> [  143.048295]  el0t_64_sync+0x190/0x194
>>>> [  143.049274] Code: f9000006 f81f80a7 d65f03c0 361000c2 (b9400026)
>>>> [  143.050933] ---[ end trace 0000000000000000 ]---
>>>> Segmentation fault
>>>>
>>>> root@qemu-ubuntu:~# modprobe memdup_kernel_user_test test_case=1
>>>> [   87.896982] test case 1: kmemdup failed, PTR_ERR(ptr) = 16
>>>> modprobe: ERROR: could not insert 'memdup_kernel_user_test': Invalid argument
>>>>
>>>> root@qemu-ubuntu:~# modprobe memdup_kernel_user_test test_case=2
>>>> [  124.032509] test case 2: memdup_user failed, PTR_ERR(ptr) = -14
>>>> modprobe: ERROR: could not insert 'memdup_kernel_user_test': Bad address
>>>>
>>>> root@qemu-ubuntu:~# modprobe memdup_kernel_user_test test_case=3
>>>> [  155.496285] test case 3: memdup_user failed, PTR_ERR(ptr) = 16
>>>> modprobe: ERROR: could not insert 'memdup_kernel_user_test': Invalid argument
>>>>
>>>> To sum it up, it is:
>>>> 1) If call kmemdup() with the src == NULL, a NULL pointer dereference
>>>>      fault happened.
>>>> 2) If call kmemdup() with the len == 0, an invalid address value
>>>>      ZERO_SIZE_PTR returned, consider that many existing code check
>>>>      kmemdup() return value like this:
>>>>      ptr = kmemdup();
>>>>      if (!ptr) {
>>>>      	/* allocation failed */
>>>>      }
>>>>      this could be a problem, but no fault happended, memcpy() will do
>>>>      nothing if copy length is zero, my previous statement is wrong.
>>>> 3) If call memdup_user() with src == NULL, -EFAULT returned. Because
>>>>      copy_from_user() takes care of the NULL pointer case, there is no
>>>>      fault to happend.
>>>> 4) If call memdup_user() with len == 0, an invalid address value
>>>>      ZERO_SIZE_PTR returned. The existing code uses IS_ERR() to check
>>>>      memdup_user() return value, unfortunately, the check range of the
>>>>      macro function doesn't contain ZERO_SIZE_PTR value.
>>>>
>>>> For 1), (2), we can add the following code to kmemdup() to eliminate:
>>>> if (!src || len == 0)
>>>> 	return NULL;
>>>>
>>>> For 4), we can change the statement if (!p) of memdup_user() to
>>>> if (ZERO_OR_NULL_PTR(s)) to solve that.
>>>>
>>>> BTW, the return values of kmemdup() and memdup_user() got a little
>>>> bit confused for now:
>>>> . kmemdup() can return ZERO_SIZE_PTR, NULL, and a valid memory allocation
>>>>     address, the caller should check those return values with ZERO_OR_NULL_PTR(),
>>>>     but many existing code don't follow this.
>>>> . memdup_user() can return ZERO_SIZE_PTR,-ENOMEM,-EFAULT,NULL, and a valid
>>>>     memory allocation address, the caller should check those return values with
>>>>     ZERO_OR_NULL_PTR() and IS_ERR() at the same time, but i can't find any code
>>>>     do things like this.
>>>>
>>>>>> Signed-off-by: Xujun Leng <lengxujun2007@xxxxxxx>
>>>>>> ---
>>>>>>     mm/util.c | 3 +++
>>>>>>     1 file changed, 3 insertions(+)
>>>>>>
>>>>>> diff --git a/mm/util.c b/mm/util.c
>>>>>> index dd12b9531ac4..d1a3b3d2988e 100644
>>>>>> --- a/mm/util.c
>>>>>> +++ b/mm/util.c
>>>>>> @@ -128,6 +128,9 @@ void *kmemdup(const void *src, size_t len, gfp_t gfp)
>>>>>>     {
>>>>>>     	void *p;
>>>>>>     
>>>>>> +	if (!src || len == 0)
>>>>>> +		return NULL;
>>>>>> +
>>>>>>     	p = kmalloc_track_caller(len, gfp);
>>>>>>     	if (p)
>>>>>>     		memcpy(p, src, len);
>>>>
>>>>> Why should we take care of kmemdup(), but not memdup_user() ? Shouldn't
>>>>> it suffer from similar problems?
>>>> By the foregoing, i think that both kmemdup() and memdup_user() need to
>>>> change.
>>>
>>> The issue is that you can call mostly any kernel function with
>>> unsupported arguments and trigger crashes. It all depends on with which
>>> parameters functions are expected to be called.
>>>
>>> If kmemdup() is not expected to be called with !src or !len, all is
>>> fine. And if there are no broken cases, existing code obeys these rules.
>>>
>>> Of course, we could improve the documentation or adjust the
>>> implementations, if there is real need to.
>>>
>>> But adjusting individual functions here while others are left with he
>>> same, theoretical (!) problems, is not a good approach IMHO.
>> 
>> Yes, you're right. The best way is to change kmalloc_slab(), let it always
>> return NULL on allocate failure, even for requested size == 0. And of course,
>> the detailed error message ZERO_SIZE_PTR will lost.
>> 
>> On the other hand, except kmemdup() and memdup_user(), the other functions
>> in mm/util.c, who called kmalloc_track_caller(), like kstrdup(), kstrndup(),
>> kmemdup_nul(), memdup_user_nul(), their all do argument check, and the
>> len >= 1(if ignore the wrap case). So if we need change kmemdup() and
>> memdup_user() a little, to let those all functions keep the same? If the
>> answer is NO, we can end the disscuss to save your time.
>
> It's not me to decide. :) I consider sanitizing the input of all of 
> these functions valuable, I just don't think the individual poking makes 
> sense. So I would certainly review a patch (series) that unifies the 
> error checks performed in these functions (e.g., be able to handle NULL 
> pointers or len=0).

Ok, got it.
However, this can be the responsibilty of the caller to handle these things. 
Things seemed to start to get meaningless, after all, so far, nothing bad had 
happended yet, so I'm not following up anymore, thanks anyway :).

--
Best regards,

Xujun Leng