Messages in this thread | | | From | Xujun Leng <> | Subject | Re: [PATCH] mm: fix potential invalid pointer dereference in kmemdup() | Date | Thu, 9 Mar 2023 18:04:15 +0800 |
| |
> 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@126.com> >>>> --- >>>> 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. -- Thanks,
Xujun Leng
| |