From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id AC5FCC61DA4 for ; Thu, 9 Mar 2023 06:46:45 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 0E86F6B0075; Thu, 9 Mar 2023 01:46:45 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 098DB280001; Thu, 9 Mar 2023 01:46:45 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id EA2796B007B; Thu, 9 Mar 2023 01:46:44 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0013.hostedemail.com [216.40.44.13]) by kanga.kvack.org (Postfix) with ESMTP id DAE756B0075 for ; Thu, 9 Mar 2023 01:46:44 -0500 (EST) Received: from smtpin18.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay02.hostedemail.com (Postfix) with ESMTP id 7D535120764 for ; Thu, 9 Mar 2023 06:46:44 +0000 (UTC) X-FDA: 80548426728.18.1CEBAF0 Received: from m126.mail.126.com (m126.mail.126.com [123.126.96.242]) by imf01.hostedemail.com (Postfix) with ESMTP id 8447940005 for ; Thu, 9 Mar 2023 06:46:41 +0000 (UTC) Authentication-Results: imf01.hostedemail.com; dkim=pass header.d=126.com header.s=s110527 header.b=EJp7LVKM; spf=pass (imf01.hostedemail.com: domain of lengxujun2007@126.com designates 123.126.96.242 as permitted sender) smtp.mailfrom=lengxujun2007@126.com; dmarc=pass (policy=none) header.from=126.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1678344402; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=ML8/8GyNwRH9llI8pfK1W8/ihqNzoTEF9cIJhETglCs=; b=MHnHl+UmaUmuekVXiqksaijAjSFSbJkQ5Soma/Pw1neFR2YNivzKkaJI7h0J/IaYT4gGTZ xt9oqKyMH1U5icR6TDFrFgBl0CMz2P/bQTJRT0A2pe/BsjDEOlO+v+dP88ZrFaqbEQw2Xs VoPca2DEPzj0cmynZ0IgVJkc2VC2WCA= ARC-Authentication-Results: i=1; imf01.hostedemail.com; dkim=pass header.d=126.com header.s=s110527 header.b=EJp7LVKM; spf=pass (imf01.hostedemail.com: domain of lengxujun2007@126.com designates 123.126.96.242 as permitted sender) smtp.mailfrom=lengxujun2007@126.com; dmarc=pass (policy=none) header.from=126.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1678344402; a=rsa-sha256; cv=none; b=kyEneLE9viLjyC8uhWfRc7HvzhT/v5Vu1F4mWRrUomkEQJzQUJ3Uo4SyxHkt5eUOMJaa2F pKVcuCslWUuRiMmXzRP+IO3yrgnlqECex+xC6KtH20k9r8xurBsrZpVJAhOIhnAyg24kvA aXoI8nGJIzUgC5O01KUAWdVjKOrGfH0= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=126.com; s=s110527; h=From:Subject:Date:Message-Id:MIME-Version; bh=ML8/8 GyNwRH9llI8pfK1W8/ihqNzoTEF9cIJhETglCs=; b=EJp7LVKMrp24Z9LGppUiQ OeDAkhDge/ZwWN46fWdtFJFP/SlR94UGJrP09e5iXfSPH93/ju9rcrGkbE6TC63A v746mbkgeNFBxspNZxhX+ewpsJ9GhpPb1wCbxv8CswMVmvGRFWKItfXNw6jJUMdm naGb56whF3mCejKX3nar3g= Received: from localhost.localdomain (unknown [113.91.40.179]) by smtp14 (Coremail) with SMTP id fuRpCgA3PvbMgAlkKslFBw--.28433S2; Thu, 09 Mar 2023 14:46:37 +0800 (CST) From: Xujun Leng To: david@redhat.com Cc: akpm@linux-foundation.org, lengxujun2007@126.com, linux-kernel@vger.kernel.org, linux-mm@kvack.org Subject: Re: [PATCH] mm: fix potential invalid pointer dereference in kmemdup() Date: Thu, 9 Mar 2023 14:46:33 +0800 Message-Id: <20230309064633.3617-1-lengxujun2007@126.com> X-Mailer: git-send-email 2.25.1 In-Reply-To: <4c223e9c-2d43-fb8f-7ac9-ad2121914170@redhat.com> References: <4c223e9c-2d43-fb8f-7ac9-ad2121914170@redhat.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-CM-TRANSID:fuRpCgA3PvbMgAlkKslFBw--.28433S2 X-Coremail-Antispam: 1Uf129KBjvJXoW3Wryktry3CF1xtw47Gr4xtFb_yoWfWr15p3 y5Jr43Kw4kXw17Jw4xJw10vr1Ivr1xAF4UGF1qy398uF4j9w18t3yxtay7trn8JrW8Xa4x JwnY9F43tF1UKaUanT9S1TB71UUUUU7qnTZGkaVYY2UrUUUUjbIjqfuFe4nvWSU5nxnvy2 9KBjDUYxBIdaVFxhVjvjDU0xZFpf9x0piFksPUUUUU= X-Originating-IP: [113.91.40.179] X-CM-SenderInfo: pohqw5hxmx0jqqqxqiyswou0bp/1tbiEw0td1pECfVY0wAAst X-Rspamd-Server: rspam05 X-Rspamd-Queue-Id: 8447940005 X-Stat-Signature: f3sezn7z3zmqog7ji8rqonfqux9u6nf6 X-Rspam-User: X-HE-Tag: 1678344401-732181 X-HE-Meta: U2FsdGVkX18hRn3bNNuAEgcIgG9DSYqBYfxq6jLwL8upIbLgc+hHfAGcl16Lp0lPUifhdDsP/7erd4UWQGxhtMjbc71QyjD85C5Sq9+ALAxUsU+TIco3IWJJDMxDapS15YmaJC6RUA8yB6MKSFPEfmIQApaPy3y/YuYfZy2ngDxyWk0Rr10Q/N69phtp/aGkWctyU0QxVZf4fphmI+vb4AsgTIBpQaai9ky0lAHvkLJTS8L5N+Lc2Lsh62FVjNC0LlEaig6DATq2aFOvW9xFv0PA0tBWXJdVL3NaBxxYLoGO4XNH9iS6PYnAcVpYwKcdxTjZIBQyxY1Q26dDLOh7rC6jAfg3V4z0nPmjkpSUTwcmvIw5Os+JcBruY5hKx3QFs82JmraYf86J+F7tLqc1PC0AicDVzYFxDuKR913owXUmj9yh2/rHQBgxt2DSX87pFfwQ0Rz1oYs3aK71y9shW+5dqnAVmOwP8LeGof22upadSbPg4y4ThDz2oSTq58nlf9yjx5CW3mrU6hMl/y0MWXV1IlIZWGH7p6p5ktbrQXe/O+ubVzKEzt+A5VeF3yBfjY2mxV82aXERHIKoxHOTNobe93PGdZyvOpSpBjPOyHPjZJ74seCs9c1PT3VDZ0XsgqsnsaY1gHvSR4uOAyY1/+hBZ++yNZzKTEYKJq6msW/fcT70zDqLNm8QNVzunawEEq+psAeGbiTu699ictF5/yl82zBArnA7IBiNoLMJx38lBb3huGopBLHoDjs/6iJ5/x4A/UgBbob6nWy2RXdMtN5UA+AUxInaW6PSxLLmZMmjw38sp6Os+MuvI2yYeYxg31yOCzLR5ByqKnkr/HhhQD5/U5O5lCw6HDqrCy/G8g5Bnyt3cEYFU+saQ6ZDGLKmjvFIwI/MUCjGMyaOe6CHXFRCBQEEweyr6NmJHeXPY42os9RngDp2duvxDSs04S+ZGk3YSi9IqXNFbbuSO/k 579U7s3c 9nAsJj0tYJJIgBUP3kLwJjmy3F6D1luiK1rdxFXnqKaJB/cSTncQrPTb1TsY8nt9wKy/9OvNvR9UHKCi+ePFBMFztG4QpU90AZhq5E9woOkSue0PUZSUdvcpDDWYx53irgSYaT/NMeK43ID8OiT8vPdJo9U941vhC4dvD6hRvoaF+4fSUQog1Gt+4758ooSnbu4ncrW5/iOpUReUf+/YuRwv3H2R2RiggPtabOevfnRKyLouBIfzEA6i09ywjOabkCIwkfQWln5Nwu5I6LXZKKE4Liw6Khcn1SDEYnt0lZCk8ctEMsA4Sjc+9uw== X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: > 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 #include #include #include #include /* * 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 > > --- > > 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. --