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 4CB98C64EC4 for ; Thu, 9 Mar 2023 13:23:07 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id BF6756B0071; Thu, 9 Mar 2023 08:23:06 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id B7D9D6B0072; Thu, 9 Mar 2023 08:23:06 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id A01AE280001; Thu, 9 Mar 2023 08:23:06 -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 8DA416B0071 for ; Thu, 9 Mar 2023 08:23:06 -0500 (EST) Received: from smtpin21.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id 5D21F1606CA for ; Thu, 9 Mar 2023 13:23:06 +0000 (UTC) X-FDA: 80549425572.21.2690843 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by imf24.hostedemail.com (Postfix) with ESMTP id BC85418001B for ; Thu, 9 Mar 2023 13:23:03 +0000 (UTC) Authentication-Results: imf24.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=OMUAWS1m; dmarc=pass (policy=none) header.from=redhat.com; spf=pass (imf24.hostedemail.com: domain of david@redhat.com designates 170.10.133.124 as permitted sender) smtp.mailfrom=david@redhat.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1678368184; 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-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=Yhh05HEeTynj++ObYTl6qMBxPge98QWiHU+ULkQSrwM=; b=z19kthY3ReJrgYIVo6Q/REHVfebJ59HDQMxtnnOv9wlAXZzKmkTVPrVzaRvk0kZxihPZv8 01M+qerNMGlddNs5tlgjzDHD/OjDvZxHG6+PIg/CesJ7pPoPRfbX8p58dxy9rCbXnklnIm ubojX2P1IpvWQYcscPqk09lYR0EQqzs= ARC-Authentication-Results: i=1; imf24.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=OMUAWS1m; dmarc=pass (policy=none) header.from=redhat.com; spf=pass (imf24.hostedemail.com: domain of david@redhat.com designates 170.10.133.124 as permitted sender) smtp.mailfrom=david@redhat.com ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1678368184; a=rsa-sha256; cv=none; b=6nTiKrlfJTEQwBsL0cUCleXT8oJpNlSpOg9fYKVphOwP4oj6iDuebwnCAa9+1juQjEtTfq 7ovwYngTqgplEb2wP3PFPq3EIF3u9wBpU+QJ76e1m9xx8Hl3m67YWn53Zqkmwny3JevKhR sKgl3ox9f9d+883LaTzIrSwW41OMdVI= DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1678368183; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=Yhh05HEeTynj++ObYTl6qMBxPge98QWiHU+ULkQSrwM=; b=OMUAWS1myijnSlqVscc/f7eZkxM4jQgaO+NaNY+UEZkNV4CG6XPrtEK2WRFuGJFH5HfcfO KfzRvNukgfHHDBomSE0ATup/MkaNZazqQGvZ/tSO0hd8U7Jn85WF90rXXc50iHCoXXK/kD v7+brdrp87j7bGF1h3g0XjDue3SP/8E= Received: from mail-wm1-f72.google.com (mail-wm1-f72.google.com [209.85.128.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-22-hqpTllfxNh-2cY78PM6Vlw-1; Thu, 09 Mar 2023 08:23:00 -0500 X-MC-Unique: hqpTllfxNh-2cY78PM6Vlw-1 Received: by mail-wm1-f72.google.com with SMTP id s18-20020a7bc392000000b003deaf780ab6so781299wmj.4 for ; Thu, 09 Mar 2023 05:22:59 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1678368179; h=content-transfer-encoding:in-reply-to:organization:from:references :cc:to:content-language:subject:user-agent:mime-version:date :message-id:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=Yhh05HEeTynj++ObYTl6qMBxPge98QWiHU+ULkQSrwM=; b=2N7BNbk8EN4V121wJKCq3kuV3GSM4fyBCIOZjBPVEwoRA91FoyJ9Uw2HuQDSmYlC/S 08ddFrxY4XllMxUZTgvgaq6dgNVyweG6lHxiQp0zbjBvW1qOd8RArl15u6bJUJ0ucjk/ 5Qcy0CF2x2fKC2ye8equNfCNifPeXiEIQsjkayRmVm+hkq2JDCmN5BZfT3dZc8EB28sE 3Fi54BsfZNH9rKf264m6PYIJV5efcuhFeVY3eRQEeJuhwq0ZYeu226LmCVBcN6D+ze0C EM1Kw5BdfFROu+tJFYUCo/nYmNGeGzkvgr9YgyQrk+ewt6mb1VzT/8TllAPCEGMYfpfg gpVQ== X-Gm-Message-State: AO0yUKXoL0I5TkRmvYr+wPRyGc5vMlUAdsYrgmwAmyl7+fCU4ubAhJKY tGvEHhkK6RBlw0MNgO/oIHqZSbVEtFAPxfi3Trg9+UZXpOpKV6xyq++ulqmonkla/uE5PsUzZBY ldFAm5VIn1kg= X-Received: by 2002:a5d:6604:0:b0:2c7:d7e:4c6c with SMTP id n4-20020a5d6604000000b002c70d7e4c6cmr12864105wru.44.1678368178938; Thu, 09 Mar 2023 05:22:58 -0800 (PST) X-Google-Smtp-Source: AK7set/ZYO/c53IQo1unP9hzEw3MBcOJ/D9vBXdr4gD0cfHk7JyBlZHSLClP55Q8+cgLUagfjU8x/w== X-Received: by 2002:a5d:6604:0:b0:2c7:d7e:4c6c with SMTP id n4-20020a5d6604000000b002c70d7e4c6cmr12864095wru.44.1678368178535; Thu, 09 Mar 2023 05:22:58 -0800 (PST) Received: from ?IPV6:2003:cb:c702:5200:a73:3e7e:12c:b175? (p200300cbc70252000a733e7e012cb175.dip0.t-ipconnect.de. [2003:cb:c702:5200:a73:3e7e:12c:b175]) by smtp.gmail.com with ESMTPSA id h19-20020a05600c315300b003db0bb81b6asm2820877wmo.1.2023.03.09.05.22.57 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 09 Mar 2023 05:22:58 -0800 (PST) Message-ID: <42d864d6-6517-6601-c9c9-e06f7b731314@redhat.com> Date: Thu, 9 Mar 2023 14:22:57 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101 Thunderbird/102.8.0 Subject: Re: [PATCH] mm: fix potential invalid pointer dereference in kmemdup() To: Xujun Leng Cc: akpm@linux-foundation.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org References: <20230309100415.2382-1-lengxujun2007@126.com> From: David Hildenbrand Organization: Red Hat In-Reply-To: <20230309100415.2382-1-lengxujun2007@126.com> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-Rspam-User: X-Rspamd-Server: rspam02 X-Rspamd-Queue-Id: BC85418001B X-Stat-Signature: a9x4r7rywd1n9ujhpd7ng834ywuupumk X-HE-Tag: 1678368183-744141 X-HE-Meta: U2FsdGVkX1/kWXq3U6LIwlL+qLp6ZEYx9B1mzPFSmSysCAFlNxbkyfNfVsSj/6eFb6yzgZjEFNhCXjjxyYYWWDIkk31pIMq+jQjDW+MZfN+zZtekD8U9kOmvY1T1/FBrP/ZipOsVBvpMAX9DeTu/JdPIofMgiDHEqSUKN94DuPnRoY9k06fWIeRQ4JerzUNyrbMA+S7NYM/p17B60wf23SI++EnAg2aQHNG5ClOaOUUWp0MywKSNQ4F0LdnSLj/da/lFPFNU9SP6nfPvikPz8aGkbcyfDlZ/KlIiqUfsJ4nkUQrDdGnIEIydtdtaE3SucmPriYtyg7+nquoZwAgljviL5sq7GJxyPXzcJmfefwCgTbJp0s9I3j4JEVdA1AwvFqyB3h1r2YIj9lLz5uT50QVZVQq/V+jxXKWX5/0ipnEmJd6Cv/hDEus1EKoD4zqZqlUnj0LatKv17YzWu5J63GE21fEWLaPBdISrn1cnqdzPWcsad+FvcaPwB/WqmXaVdOs27WmmZiwvgd/wbWqf1F7UGTn+oiysFxBj7sU08uS+irJr+ScScSvS2x+OX8WzlLm0xrLM94657viIL6ZHvmahFRZXGo05qfdkFmPsK/UtxVfkRPEkIiUToSUlXEWh7K13hbisfWdsPZWGg/bz2gplMliMDzeYPJey7ekcZ8T8P10S7yOZZpX67x0i5KHyHeRHVcX9moTIfJHugSr+RFlynL1Lef2kNlM0v8lVAGs5urm28oqBVrwS8LoorJGmevcP5otnN1M/OmblQMQtehLbYZq/meif6EtE8x64BTNS/Y7FJcHzQlPGtTVeRWclc0aYeAYTYzsIBee8zy1utKDsI51SvhW1Bb4R7b9BXFNl8ziqLo0Rg9Vd+tdZ3+LjxDJ6TZU5HGA+tWwcciB5BDRhs5RvCVvAPu8kwAQLY9KJzR5GUJMWi+5R3EMLhk1N7iUsdHrVO+Fq6/KyUoJ /Ul5kndp m+7oc+CWLSftcRJxz/y9jWB3s2SicXiHI7tEphWutgvVRfg0bqIdKCT3NAr9NgkFxb9b6Mhv+Ohh4cxJOe9deYBpHPz2VF1JbY49fWy6Zo66u33MLBGJrvi35TBzSRqkgI173LEvOQHjgfX6i+1jpCCYbhVYlj18drPS//RPZuz1TsWCBvUGVM4x/0BiCK+amzhJO0ugobQU1ce+Ycy37bV3ZGSNxEev0HzngXUub9iOAQuzDm/TEks6YcQaARuHmw6ToehjOAegX20K9WiK0zScHna5Mzu6U7lTdExEadkLKj+rn915JV4xBLSKv7Gl81ya1u8KmfsgfH6/5YyQUswbpH7GLBG2MiG58bxFk0VnFOobpg1NgtecVajXeywJTgih1ib+z0f/e86VAXMiB1gJ1NisVXq2MzhceC1btuvMCnbo= 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 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 >>> #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. >> >> 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). -- Thanks, David / dhildenb