From patchwork Thu Jan 16 06:26:23 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Daniel Axtens X-Patchwork-Id: 11336203 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 3B41B13B4 for ; Thu, 16 Jan 2020 06:26:53 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 136C12081E for ; Thu, 16 Jan 2020 06:26:53 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="irQP6IWF"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=axtens.net header.i=@axtens.net header.b="C+UNMN/n" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 136C12081E Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=axtens.net Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Message-Id:Date:Subject:To:From:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=ymGlMf0NBw7ZgJ01/vxoKjMTZ3DuCH/zssxZYxvK9rU=; b=irQP6IWFQ7O/iG mxfDeJGm4146S4u2Np28n9OMBPX8+uT+JKdpm0kdOP8RtS0EMxnHSIIBIuZ/ewdnxOq18gFO4HMpy H3zbKbKoQuADnhi6ThWqU0Q36A/s2n55TA+yOBJv5ElfybFNFMkUZNVBC2lbxR5MYeOx0dGw2xA9n bfCjlf1B4HPDXxi/QUrFIvGEbBweUYqUJgWJiDaAEj/1Tmnh3uruFVIPYxmaHQwbFs2NwcCL8BLzT nATOYiFu160VjnBgbDmU24vCLrhB/UAO0Be3ehG2XLBVboKjHwxwh+uH5bEDvIWbFrJw8qZQJL9qB 1YsNiSaOkwROJ6UGVqpw==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1irycB-0001Rk-Oa; Thu, 16 Jan 2020 06:26:47 +0000 Received: from mail-pg1-x542.google.com ([2607:f8b0:4864:20::542]) by bombadil.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1iryc0-0001FU-2n for linux-arm-kernel@lists.infradead.org; Thu, 16 Jan 2020 06:26:39 +0000 Received: by mail-pg1-x542.google.com with SMTP id b137so9372302pga.6 for ; Wed, 15 Jan 2020 22:26:35 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=axtens.net; s=google; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=pcisanTgJm2Q3K0/rpShD/S/gMp/LV+S7q0SnsZ5eZA=; b=C+UNMN/n21Z+KKPWTfdkIPpgJbCp3mGvMCgNhHm92fuQmkPRxe8JFhLupRWnAl3BwY h4nyTZzX4mKEfs9QRe+aUDa/7XzgnYI3nzON1Ohb5efUL0C15DI/G5D7rqbRgrqYl5Ef h9i+GdntCOUA9SGQQCKWLxSGfDECudeCS5xHk= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=pcisanTgJm2Q3K0/rpShD/S/gMp/LV+S7q0SnsZ5eZA=; b=L2mA59yPMN1mK2PliNeP6FKfbmdQ7gUAvi1fQ5Wu2IQxGgtHoKIEkEz9YCIx2TEUpv OkTN4ZSukXEsX1e7MQhw3XJ6utHVhEfEcmO8O46wqSMMDMKyGjm8sIL+db1wchuCEnfA I3YYhANBbqtKlXOLOAPw5mjydtm3qI1Kd/5FHjrU49QmIpsElbqdY1cy9tbk+y77aL2R yRPm53irkzxz1xTR9Uth8o6nrxStt8D0ofr8AmqKvl17SXM9locdeBMNPFer5Ic1A29q bRTo5V9rPLtJQmCNaNyqHC8H/SX0bu1WfoIj5hUCeQtisI7hKzdmbRU4F7v03p8xTw2t 1JAw== X-Gm-Message-State: APjAAAUp74PT26qg7V5FBJIaBSQSjv2gwB/xQDhoUGVW1kusvDESVa6i BUivwLt7MotZhLhOfFWqt+zDng== X-Google-Smtp-Source: APXvYqzZNecZWkElAjCkhku46ZLAEt1HILfHwFxN9cP8hb0d/spt/ieWbkSQrR4axqy4NHudIk+/7A== X-Received: by 2002:a63:220b:: with SMTP id i11mr36948573pgi.50.1579155995165; Wed, 15 Jan 2020 22:26:35 -0800 (PST) Received: from localhost (2001-44b8-1113-6700-097c-7eed-afd4-cd15.static.ipv6.internode.on.net. [2001:44b8:1113:6700:97c:7eed:afd4:cd15]) by smtp.gmail.com with ESMTPSA id c68sm24184359pfc.156.2020.01.15.22.26.33 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 15 Jan 2020 22:26:34 -0800 (PST) From: Daniel Axtens To: linux-kernel@vger.kernel.org, linux-mm@kvack.org, kasan-dev@googlegroups.com Subject: [PATCH v2 1/3] kasan: stop tests being eliminated as dead code with FORTIFY_SOURCE Date: Thu, 16 Jan 2020 17:26:23 +1100 Message-Id: <20200116062625.32692-2-dja@axtens.net> X-Mailer: git-send-email 2.20.1 In-Reply-To: <20200116062625.32692-1-dja@axtens.net> References: <20200116062625.32692-1-dja@axtens.net> MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200115_222636_121266_D9D386D0 X-CRM114-Status: GOOD ( 16.33 ) X-Spam-Score: -0.2 (/) X-Spam-Report: SpamAssassin version 3.4.2 on bombadil.infradead.org summary: Content analysis details: (-0.2 points) pts rule name description ---- ---------------------- -------------------------------------------------- -0.0 RCVD_IN_DNSWL_NONE RBL: Sender listed at https://www.dnswl.org/, no trust [2607:f8b0:4864:20:0:0:0:542 listed in] [list.dnswl.org] -0.0 SPF_PASS SPF: sender matches SPF record 0.0 SPF_HELO_NONE SPF: HELO does not publish an SPF Record -0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's domain 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature -0.1 DKIM_VALID_EF Message has a valid DKIM or DK signature from envelope-from domain X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: christophe.leroy@c-s.fr, linux-s390@vger.kernel.org, linux-xtensa@linux-xtensa.org, x86@kernel.org, Daniel Micay , Alexander Potapenko , linux-arm-kernel@lists.infradead.org, Andrey Ryabinin , linuxppc-dev@lists.ozlabs.org, dvyukov@google.com, Daniel Axtens Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org 3 KASAN self-tests fail on a kernel with both KASAN and FORTIFY_SOURCE: memchr, memcmp and strlen. When FORTIFY_SOURCE is on, a number of functions are replaced with fortified versions, which attempt to check the sizes of the operands. However, these functions often directly invoke __builtin_foo() once they have performed the fortify check. The compiler can detect that the results of these functions are not used, and knows that they have no other side effects, and so can eliminate them as dead code. Why are only memchr, memcmp and strlen affected? ================================================ Of string and string-like functions, kasan_test tests: * strchr -> not affected, no fortified version * strrchr -> likewise * strcmp -> likewise * strncmp -> likewise * strnlen -> not affected, the fortify source implementation calls the underlying strnlen implementation which is instrumented, not a builtin * strlen -> affected, the fortify souce implementation calls a __builtin version which the compiler can determine is dead. * memchr -> likewise * memcmp -> likewise * memset -> not affected, the compiler knows that memset writes to its first argument and therefore is not dead. Why does this not affect the functions normally? ================================================ In string.h, these functions are not marked as __pure, so the compiler cannot know that they do not have side effects. If relevant functions are marked as __pure in string.h, we see the following warnings and the functions are elided: lib/test_kasan.c: In function ‘kasan_memchr’: lib/test_kasan.c:606:2: warning: statement with no effect [-Wunused-value] memchr(ptr, '1', size + 1); ^~~~~~~~~~~~~~~~~~~~~~~~~~ lib/test_kasan.c: In function ‘kasan_memcmp’: lib/test_kasan.c:622:2: warning: statement with no effect [-Wunused-value] memcmp(ptr, arr, size+1); ^~~~~~~~~~~~~~~~~~~~~~~~ lib/test_kasan.c: In function ‘kasan_strings’: lib/test_kasan.c:645:2: warning: statement with no effect [-Wunused-value] strchr(ptr, '1'); ^~~~~~~~~~~~~~~~ ... This annotation would make sense to add and could be added at any point, so the behaviour of test_kasan.c should change. The fix ======= Make all the functions that are pure write their results to a global, which makes them live. The strlen and memchr tests now pass. The memcmp test still fails to trigger, which is addressed in the next patch. Cc: Daniel Micay Cc: Andrey Ryabinin Cc: Alexander Potapenko Cc: Dmitry Vyukov Fixes: 0c96350a2d2f ("lib/test_kasan.c: add tests for several string/memory API functions") Reviewed-by: Dmitry Vyukov Signed-off-by: Daniel Axtens --- v2: rename variables to have kasan_ prefixes --- lib/test_kasan.c | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/lib/test_kasan.c b/lib/test_kasan.c index 328d33beae36..a130d75b9385 100644 --- a/lib/test_kasan.c +++ b/lib/test_kasan.c @@ -23,6 +23,14 @@ #include +/* + * We assign some test results to these globals to make sure the tests + * are not eliminated as dead code. + */ + +int kasan_int_result; +void *kasan_ptr_result; + /* * Note: test functions are marked noinline so that their names appear in * reports. @@ -603,7 +611,7 @@ static noinline void __init kasan_memchr(void) if (!ptr) return; - memchr(ptr, '1', size + 1); + kasan_ptr_result = memchr(ptr, '1', size + 1); kfree(ptr); } @@ -618,8 +626,7 @@ static noinline void __init kasan_memcmp(void) if (!ptr) return; - memset(arr, 0, sizeof(arr)); - memcmp(ptr, arr, size+1); + kasan_int_result = memcmp(ptr, arr, size + 1); kfree(ptr); } @@ -642,22 +649,22 @@ static noinline void __init kasan_strings(void) * will likely point to zeroed byte. */ ptr += 16; - strchr(ptr, '1'); + kasan_ptr_result = strchr(ptr, '1'); pr_info("use-after-free in strrchr\n"); - strrchr(ptr, '1'); + kasan_ptr_result = strrchr(ptr, '1'); pr_info("use-after-free in strcmp\n"); - strcmp(ptr, "2"); + kasan_int_result = strcmp(ptr, "2"); pr_info("use-after-free in strncmp\n"); - strncmp(ptr, "2", 1); + kasan_int_result = strncmp(ptr, "2", 1); pr_info("use-after-free in strlen\n"); - strlen(ptr); + kasan_int_result = strlen(ptr); pr_info("use-after-free in strnlen\n"); - strnlen(ptr, 1); + kasan_int_result = strnlen(ptr, 1); } static noinline void __init kasan_bitops(void) @@ -724,11 +731,12 @@ static noinline void __init kasan_bitops(void) __test_and_change_bit(BITS_PER_LONG + BITS_PER_BYTE, bits); pr_info("out-of-bounds in test_bit\n"); - (void)test_bit(BITS_PER_LONG + BITS_PER_BYTE, bits); + kasan_int_result = test_bit(BITS_PER_LONG + BITS_PER_BYTE, bits); #if defined(clear_bit_unlock_is_negative_byte) pr_info("out-of-bounds in clear_bit_unlock_is_negative_byte\n"); - clear_bit_unlock_is_negative_byte(BITS_PER_LONG + BITS_PER_BYTE, bits); + kasan_int_result = clear_bit_unlock_is_negative_byte(BITS_PER_LONG + + BITS_PER_BYTE, bits); #endif kfree(bits); } From patchwork Thu Jan 16 06:26:24 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Axtens X-Patchwork-Id: 11336205 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 105D1138D for ; Thu, 16 Jan 2020 06:27:07 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id DF4282075B for ; Thu, 16 Jan 2020 06:27:06 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="Squ/PzDz"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=axtens.net header.i=@axtens.net header.b="Uhihq+6r" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org DF4282075B Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=axtens.net Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Message-Id:Date:Subject:To:From:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=V98mWI7SXTZQvqSbAivwGGjn1SEuJZ2vojX4bZwfUrE=; b=Squ/PzDzKIHCJq cZ8wOEQ7KIBsjIkL53f3CbG3JH3a0bCfLVOkeA7rcPSIXdjTigQftnRG0wnJYgkEjCPTVaj+Ywi/O 82BiD33OS/ClOy4LlH09lf6SzX2H2yWQh0wn4F3WxlAMjDFzNJfbEub7dIGMyM/LFE84gAeZibet6 CWT90m3SBmst0U4PnGYPOB6k93dZLp9AmjUq5OVMrF6AD7vGXEKH6C3vVz6u9BsC2yx26IW2kymtb 5i/kwvqqIzsHcS+a5CDmhdKUnLP+GyPK0dtanzE2+ZIJPs9BT4TBugf1PHhACdVn4AimC3qRV5nav oLByvzRYzYYE0728h2vQ==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1irycT-0001i6-Bt; Thu, 16 Jan 2020 06:27:05 +0000 Received: from mail-pl1-x642.google.com ([2607:f8b0:4864:20::642]) by bombadil.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1iryc4-0001IS-If for linux-arm-kernel@lists.infradead.org; Thu, 16 Jan 2020 06:26:44 +0000 Received: by mail-pl1-x642.google.com with SMTP id b22so7885461pls.12 for ; Wed, 15 Jan 2020 22:26:40 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=axtens.net; s=google; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=ecViW2B6K6MgjtfxW3TT7mCoLEuRKAGSK4ebp685lGI=; b=Uhihq+6rfB5UyQl+9UDv+i3ThvZFs5H/8nw3PJ9UAbz3R2r3AMJzgomMFAzOXhQixb 9c4b8qrhyrPfdzEOkv05oUMvhz2JnmwO7kdW6N8LsZbFuuF4bvwkNyNZ4AOA+k+w3p4A vn7BPofvQmrLxHWpcX8lHyw7JZ1dUbkAXFCGQ= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=ecViW2B6K6MgjtfxW3TT7mCoLEuRKAGSK4ebp685lGI=; b=Syx9/L1s36qlRvAiJ8nz+ZvlckeV8igmrf2cguS+PGPxukeEyTgVf/mFkLc+xlLKQF NK7yp6P0DCj32C9iHjLTpiCwPm1hmqQqjA6fhDpp6uVGCBYkEmJzJeRmk6gKxhp9ozdN XJDcL44M/t52MpbSfZzbJ33cwJW5q4KF52fe3M1QtScFUAnCSio9U6pIswHrNfkC/B70 75CNBGx/E3ErGlMINdE8pQbF/Bo/sz/LUOW59NwyayqqN8Fo61AaCDNGUySwcaemtK3Y 1KJg43S5KxcqG59D+8V3OiMnIbTnd+J35z9i4d+q2253q0VGllFKCx8VvsQieDsN/Nbe e9Ow== X-Gm-Message-State: APjAAAVbI5U9a9q2puBx7YGCPfna1c90xrueL1eu4d+qy1ei0h15eb5C MIz6jgB3G/7kHcYw/fpt0P1v9A== X-Google-Smtp-Source: APXvYqw19UTxY8Hi9APh86aEtqUHLB8ch68t3FABEMRmpJHJbbxu4RZM4Mz69GnkzexI90XRmebJvQ== X-Received: by 2002:a17:90a:a608:: with SMTP id c8mr4785355pjq.36.1579155999384; Wed, 15 Jan 2020 22:26:39 -0800 (PST) Received: from localhost (2001-44b8-1113-6700-097c-7eed-afd4-cd15.static.ipv6.internode.on.net. [2001:44b8:1113:6700:97c:7eed:afd4:cd15]) by smtp.gmail.com with ESMTPSA id o16sm22735174pgl.58.2020.01.15.22.26.38 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 15 Jan 2020 22:26:38 -0800 (PST) From: Daniel Axtens To: linux-kernel@vger.kernel.org, linux-mm@kvack.org, kasan-dev@googlegroups.com Subject: [PATCH v2 2/3] string.h: fix incompatibility between FORTIFY_SOURCE and KASAN Date: Thu, 16 Jan 2020 17:26:24 +1100 Message-Id: <20200116062625.32692-3-dja@axtens.net> X-Mailer: git-send-email 2.20.1 In-Reply-To: <20200116062625.32692-1-dja@axtens.net> References: <20200116062625.32692-1-dja@axtens.net> MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200115_222640_676266_592C42CD X-CRM114-Status: GOOD ( 28.61 ) X-Spam-Score: -0.2 (/) X-Spam-Report: SpamAssassin version 3.4.2 on bombadil.infradead.org summary: Content analysis details: (-0.2 points) pts rule name description ---- ---------------------- -------------------------------------------------- -0.0 RCVD_IN_DNSWL_NONE RBL: Sender listed at https://www.dnswl.org/, no trust [2607:f8b0:4864:20:0:0:0:642 listed in] [list.dnswl.org] -0.0 SPF_PASS SPF: sender matches SPF record 0.0 SPF_HELO_NONE SPF: HELO does not publish an SPF Record -0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's domain 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature -0.1 DKIM_VALID_EF Message has a valid DKIM or DK signature from envelope-from domain X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: christophe.leroy@c-s.fr, linux-s390@vger.kernel.org, linux-xtensa@linux-xtensa.org, x86@kernel.org, Daniel Micay , Alexander Potapenko , linux-arm-kernel@lists.infradead.org, Andrey Ryabinin , linuxppc-dev@lists.ozlabs.org, dvyukov@google.com, Daniel Axtens Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org The memcmp KASAN self-test fails on a kernel with both KASAN and FORTIFY_SOURCE. When FORTIFY_SOURCE is on, a number of functions are replaced with fortified versions, which attempt to check the sizes of the operands. However, these functions often directly invoke __builtin_foo() once they have performed the fortify check. Using __builtins may bypass KASAN checks if the compiler decides to inline it's own implementation as sequence of instructions, rather than emit a function call that goes out to a KASAN-instrumented implementation. Why is only memcmp affected? ============================ Of the string and string-like functions that kasan_test tests, only memcmp is replaced by an inline sequence of instructions in my testing on x86 with gcc version 9.2.1 20191008 (Ubuntu 9.2.1-9ubuntu2). I believe this is due to compiler heuristics. For example, if I annotate kmalloc calls with the alloc_size annotation (and disable some fortify compile-time checking!), the compiler will replace every memset except the one in kmalloc_uaf_memset with inline instructions. (I have some WIP patches to add this annotation.) Does this affect other functions in string.h? ============================================= Yes. Anything that uses __builtin_* rather than __real_* could be affected. This looks like: - strncpy - strcat - strlen - strlcpy maybe, under some circumstances? - strncat under some circumstances - memset - memcpy - memmove - memcmp (as noted) - memchr - strcpy Whether a function call is emitted always depends on the compiler. Most bugs should get caught by FORTIFY_SOURCE, but the missed memcmp test shows that this is not always the case. Isn't FORTIFY_SOURCE disabled with KASAN? ========================================- The string headers on all arches supporting KASAN disable fortify with kasan, but only when address sanitisation is _also_ disabled. For example from x86: #if defined(CONFIG_KASAN) && !defined(__SANITIZE_ADDRESS__) /* * For files that are not instrumented (e.g. mm/slub.c) we * should use not instrumented version of mem* functions. */ #define memcpy(dst, src, len) __memcpy(dst, src, len) #define memmove(dst, src, len) __memmove(dst, src, len) #define memset(s, c, n) __memset(s, c, n) #ifndef __NO_FORTIFY #define __NO_FORTIFY /* FORTIFY_SOURCE uses __builtin_memcpy, etc. */ #endif #endif This comes from commit 6974f0c4555e ("include/linux/string.h: add the option of fortified string.h functions"), and doesn't work when KASAN is enabled and the file is supposed to be sanitised - as with test_kasan.c I'm pretty sure this is not wrong, but not as expansive it should be: * we shouldn't use __builtin_memcpy etc in files where we don't have instrumentation - it could devolve into a function call to memcpy, which will be instrumented. Rather, we should use __memcpy which by convention is not instrumented. * we also shouldn't be using __builtin_memcpy when we have a KASAN instrumented file, because it could be replaced with inline asm that will not be instrumented. What is correct behaviour? ========================== Firstly, there is some overlap between fortification and KASAN: both provide some level of _runtime_ checking. Only fortify provides compile-time checking. KASAN and fortify can pick up different things at runtime: - Some fortify functions, notably the string functions, could easily be modified to consider sub-object sizes (e.g. members within a struct), and I have some WIP patches to do this. KASAN cannot detect these because it cannot insert poision between members of a struct. - KASAN can detect many over-reads/over-writes when the sizes of both operands are unknown, which fortify cannot. So there are a couple of options: 1) Flip the test: disable fortify in santised files and enable it in unsanitised files. This at least stops us missing KASAN checking, but we lose the fortify checking. 2) Make the fortify code always call out to real versions. Do this only for KASAN, for fear of losing the inlining opportunities we get from __builtin_*. (We can't use kasan_check_{read,write}: because the fortify functions are _extern inline_, you can't include _static_ inline functions without a compiler warning. kasan_check_{read,write} are static inline so we can't use them even when they would otherwise be suitable.) Take approach 2 and call out to real versions when KASAN is enabled. Use __underlying_foo to distinguish from __real_foo: __real_foo always refers to the kernel's implementation of foo, __underlying_foo could be either the kernel implementation or the __builtin_foo implementation. This is sometimes enough to make the memcmp test succeed with FORTIFY_SOURCE enabled. It is at least enough to get the function call into the module. One more fix is needed to make it reliable: see the next patch. Cc: Daniel Micay Cc: Andrey Ryabinin Cc: Alexander Potapenko Cc: Dmitry Vyukov Fixes: 6974f0c4555e ("include/linux/string.h: add the option of fortified string.h functions") Signed-off-by: Daniel Axtens Reviewed-by: Dmitry Vyukov --- v2: add #undefs, do not drop arch code: Dmitry pointed out that we _do_ want to disable fortify in non-sanitised files because of how __builtin_memcpy might end up as a call to regular memcpy rather than __memcpy. Dmitry, this might cause a few new syzkaller splats - I first picked it up building from a syskaller config. Or it might not, it just depends what gets replaced with an inline sequence of instructions. checkpatch complains about some over-long lines, happy to change the format if anyone has better ideas for how to lay it out. --- include/linux/string.h | 60 +++++++++++++++++++++++++++++++++--------- 1 file changed, 48 insertions(+), 12 deletions(-) diff --git a/include/linux/string.h b/include/linux/string.h index 3b8e8b12dd37..18d3f7a4b2b9 100644 --- a/include/linux/string.h +++ b/include/linux/string.h @@ -317,6 +317,31 @@ void __read_overflow3(void) __compiletime_error("detected read beyond size of ob void __write_overflow(void) __compiletime_error("detected write beyond size of object passed as 1st parameter"); #if !defined(__NO_FORTIFY) && defined(__OPTIMIZE__) && defined(CONFIG_FORTIFY_SOURCE) + +#ifdef CONFIG_KASAN +extern void *__underlying_memchr(const void *p, int c, __kernel_size_t size) __RENAME(memchr); +extern int __underlying_memcmp(const void *p, const void *q, __kernel_size_t size) __RENAME(memcmp); +extern void *__underlying_memcpy(void *p, const void *q, __kernel_size_t size) __RENAME(memcpy); +extern void *__underlying_memmove(void *p, const void *q, __kernel_size_t size) __RENAME(memmove); +extern void *__underlying_memset(void *p, int c, __kernel_size_t size) __RENAME(memset); +extern char *__underlying_strcat(char *p, const char *q) __RENAME(strcat); +extern char *__underlying_strcpy(char *p, const char *q) __RENAME(strcpy); +extern __kernel_size_t __underlying_strlen(const char *p) __RENAME(strlen); +extern char *__underlying_strncat(char *p, const char *q, __kernel_size_t count) __RENAME(strncat); +extern char *__underlying_strncpy(char *p, const char *q, __kernel_size_t size) __RENAME(strncpy); +#else +#define __underlying_memchr __builtin_memchr +#define __underlying_memcmp __builtin_memcmp +#define __underlying_memcpy __builtin_memcpy +#define __underlying_memmove __builtin_memmove +#define __underlying_memset __builtin_memset +#define __underlying_strcat __builtin_strcat +#define __underlying_strcpy __builtin_strcpy +#define __underlying_strlen __builtin_strlen +#define __underlying_strncat __builtin_strncat +#define __underlying_strncpy __builtin_strncpy +#endif + __FORTIFY_INLINE char *strncpy(char *p, const char *q, __kernel_size_t size) { size_t p_size = __builtin_object_size(p, 0); @@ -324,14 +349,14 @@ __FORTIFY_INLINE char *strncpy(char *p, const char *q, __kernel_size_t size) __write_overflow(); if (p_size < size) fortify_panic(__func__); - return __builtin_strncpy(p, q, size); + return __underlying_strncpy(p, q, size); } __FORTIFY_INLINE char *strcat(char *p, const char *q) { size_t p_size = __builtin_object_size(p, 0); if (p_size == (size_t)-1) - return __builtin_strcat(p, q); + return __underlying_strcat(p, q); if (strlcat(p, q, p_size) >= p_size) fortify_panic(__func__); return p; @@ -345,7 +370,7 @@ __FORTIFY_INLINE __kernel_size_t strlen(const char *p) /* Work around gcc excess stack consumption issue */ if (p_size == (size_t)-1 || (__builtin_constant_p(p[p_size - 1]) && p[p_size - 1] == '\0')) - return __builtin_strlen(p); + return __underlying_strlen(p); ret = strnlen(p, p_size); if (p_size <= ret) fortify_panic(__func__); @@ -378,7 +403,7 @@ __FORTIFY_INLINE size_t strlcpy(char *p, const char *q, size_t size) __write_overflow(); if (len >= p_size) fortify_panic(__func__); - __builtin_memcpy(p, q, len); + __underlying_memcpy(p, q, len); p[len] = '\0'; } return ret; @@ -391,12 +416,12 @@ __FORTIFY_INLINE char *strncat(char *p, const char *q, __kernel_size_t count) size_t p_size = __builtin_object_size(p, 0); size_t q_size = __builtin_object_size(q, 0); if (p_size == (size_t)-1 && q_size == (size_t)-1) - return __builtin_strncat(p, q, count); + return __underlying_strncat(p, q, count); p_len = strlen(p); copy_len = strnlen(q, count); if (p_size < p_len + copy_len + 1) fortify_panic(__func__); - __builtin_memcpy(p + p_len, q, copy_len); + __underlying_memcpy(p + p_len, q, copy_len); p[p_len + copy_len] = '\0'; return p; } @@ -408,7 +433,7 @@ __FORTIFY_INLINE void *memset(void *p, int c, __kernel_size_t size) __write_overflow(); if (p_size < size) fortify_panic(__func__); - return __builtin_memset(p, c, size); + return __underlying_memset(p, c, size); } __FORTIFY_INLINE void *memcpy(void *p, const void *q, __kernel_size_t size) @@ -423,7 +448,7 @@ __FORTIFY_INLINE void *memcpy(void *p, const void *q, __kernel_size_t size) } if (p_size < size || q_size < size) fortify_panic(__func__); - return __builtin_memcpy(p, q, size); + return __underlying_memcpy(p, q, size); } __FORTIFY_INLINE void *memmove(void *p, const void *q, __kernel_size_t size) @@ -438,7 +463,7 @@ __FORTIFY_INLINE void *memmove(void *p, const void *q, __kernel_size_t size) } if (p_size < size || q_size < size) fortify_panic(__func__); - return __builtin_memmove(p, q, size); + return __underlying_memmove(p, q, size); } extern void *__real_memscan(void *, int, __kernel_size_t) __RENAME(memscan); @@ -464,7 +489,7 @@ __FORTIFY_INLINE int memcmp(const void *p, const void *q, __kernel_size_t size) } if (p_size < size || q_size < size) fortify_panic(__func__); - return __builtin_memcmp(p, q, size); + return __underlying_memcmp(p, q, size); } __FORTIFY_INLINE void *memchr(const void *p, int c, __kernel_size_t size) @@ -474,7 +499,7 @@ __FORTIFY_INLINE void *memchr(const void *p, int c, __kernel_size_t size) __read_overflow(); if (p_size < size) fortify_panic(__func__); - return __builtin_memchr(p, c, size); + return __underlying_memchr(p, c, size); } void *__real_memchr_inv(const void *s, int c, size_t n) __RENAME(memchr_inv); @@ -505,11 +530,22 @@ __FORTIFY_INLINE char *strcpy(char *p, const char *q) size_t p_size = __builtin_object_size(p, 0); size_t q_size = __builtin_object_size(q, 0); if (p_size == (size_t)-1 && q_size == (size_t)-1) - return __builtin_strcpy(p, q); + return __underlying_strcpy(p, q); memcpy(p, q, strlen(q) + 1); return p; } +/* Don't use these outside the FORITFY_SOURCE implementation */ +#undef __underlying_memchr +#undef __underlying_memcmp +#undef __underlying_memcpy +#undef __underlying_memmove +#undef __underlying_memset +#undef __underlying_strcat +#undef __underlying_strcpy +#undef __underlying_strlen +#undef __underlying_strncat +#undef __underlying_strncpy #endif /** From patchwork Thu Jan 16 06:26:25 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Daniel Axtens X-Patchwork-Id: 11336207 Return-Path: Received: from mail.kernel.org (pdx-korg-mail-1.web.codeaurora.org [172.30.200.123]) by pdx-korg-patchwork-2.web.codeaurora.org (Postfix) with ESMTP id 7632617EA for ; Thu, 16 Jan 2020 06:27:27 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 5160B2075B for ; Thu, 16 Jan 2020 06:27:27 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="l6SPy+aX"; dkim=fail reason="signature verification failed" (1024-bit key) header.d=axtens.net header.i=@axtens.net header.b="GfTRpKom" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 5160B2075B Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=axtens.net Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:In-Reply-To: Message-Id:Date:Subject:To:From:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=++6bFSCm4jzBozW/ebO+tGi8blRxKyibKY1FUoNTzog=; b=l6SPy+aXBltoNc UJeqNoNT81cCkSNnImIFxhW26Jez4pB61DzUdUDCQWThwuRB2/6gUeD95qmAGnTjr6Cq2sY3CiDmb v4oNNev8NL9lgvKp/0Aoa+JozCtoIf1vMHLXNIULfqbXgdVkmLfHgk4XSzwk89qt+MoH4XPIDJC9G VnRt0qpt2uESuakabzObJKYo8pVx1zWJBk4/We1nCgoBjUn9g/8Vtf7NSkifM3wwDJzrHYtlwogvi Mc+XKHvW2HgQRECQptufHRK0xoeOB3a0vmtKfMHhNYEK7AqhlvtSr65YmdTmpuWBXiKGNXTkoFAKm sPOfflmbkrejtFqPq0Rg==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1irycn-00021K-AA; Thu, 16 Jan 2020 06:27:25 +0000 Received: from mail-pj1-x1043.google.com ([2607:f8b0:4864:20::1043]) by bombadil.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1iryc8-0001Nb-I5 for linux-arm-kernel@lists.infradead.org; Thu, 16 Jan 2020 06:26:48 +0000 Received: by mail-pj1-x1043.google.com with SMTP id m13so1082888pjb.2 for ; Wed, 15 Jan 2020 22:26:44 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=axtens.net; s=google; h=from:to:cc:subject:date:message-id:in-reply-to:references :mime-version:content-transfer-encoding; bh=SI5glm9tTBFJlI56hJYs/doyA7zUbu5YUKwxBp+hpYY=; b=GfTRpKom0tPQq8D4aXQyI2foQczp9UuJXSAyw66CT6hkWQrJmpym0PT6cvNRyGY8Uk 0VvHyXu1/QQxb3+Jjk5FQ//6OW1fSF4+GEXglW8TGLZKjd4szY+kf1EU0mFv2H1ywD7M VbwSiZQaQuyCsyymCLbnA4sRvShcMEGUaENZ4= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:date:message-id:in-reply-to :references:mime-version:content-transfer-encoding; bh=SI5glm9tTBFJlI56hJYs/doyA7zUbu5YUKwxBp+hpYY=; b=ZcHsizOqbqDQjAAEyxXJ01zVPpANkiNXqmiJj6a3P0gc6QbYjd+vqIhKijAq39KLrN VFrx+YftlEZhDuyjAaHI+ZieEiFRikvRwuiWYG3JOVn9AXgamB4Vl4tZdjkYuabLuNuz rP6nfphzr0mSrOIWoG4yMtOOVxEWG2svDHHHmC/b+ablQY4XNYDpadxe8KQvOSvoqU6a dOF5yLZYPPckyHZ0xrfpOhCYyiaN0rNGTY7Q/2zZQweTCAPVjTHFoEnimQyU8e6WQfgU oTqOF1/n3na/oWFQ5DR+C+UeXhXb+KylInMzL4K70xUB/am3RNIHLdlhne/AR5kAyTJh Hi0g== X-Gm-Message-State: APjAAAXS9wrjmRuswszJJTt7l2ZvX5XNZdVc2CZYT3jYL91lSdR0ti6X xNl2wUeL+1LfFmVPYd/zuvOjEA== X-Google-Smtp-Source: APXvYqyt2Oz5lp6I4QYJC6msD4qYWnWAGuSCIA+DMfGiAHr8tDPwjSQJBDVs1A0BbM5XvepejAB4uQ== X-Received: by 2002:a17:902:6b8c:: with SMTP id p12mr29998912plk.290.1579156003508; Wed, 15 Jan 2020 22:26:43 -0800 (PST) Received: from localhost (2001-44b8-1113-6700-097c-7eed-afd4-cd15.static.ipv6.internode.on.net. [2001:44b8:1113:6700:97c:7eed:afd4:cd15]) by smtp.gmail.com with ESMTPSA id r3sm24681158pfg.145.2020.01.15.22.26.42 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 15 Jan 2020 22:26:42 -0800 (PST) From: Daniel Axtens To: linux-kernel@vger.kernel.org, linux-mm@kvack.org, kasan-dev@googlegroups.com Subject: [PATCH v2 3/3] kasan: initialise array in kasan_memcmp test Date: Thu, 16 Jan 2020 17:26:25 +1100 Message-Id: <20200116062625.32692-4-dja@axtens.net> X-Mailer: git-send-email 2.20.1 In-Reply-To: <20200116062625.32692-1-dja@axtens.net> References: <20200116062625.32692-1-dja@axtens.net> MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200115_222644_620599_8B55881A X-CRM114-Status: GOOD ( 12.83 ) X-Spam-Score: -0.2 (/) X-Spam-Report: SpamAssassin version 3.4.2 on bombadil.infradead.org summary: Content analysis details: (-0.2 points) pts rule name description ---- ---------------------- -------------------------------------------------- -0.0 SPF_PASS SPF: sender matches SPF record 0.0 SPF_HELO_NONE SPF: HELO does not publish an SPF Record -0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's domain 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature -0.1 DKIM_VALID_EF Message has a valid DKIM or DK signature from envelope-from domain X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: christophe.leroy@c-s.fr, linux-s390@vger.kernel.org, linux-xtensa@linux-xtensa.org, x86@kernel.org, Alexander Potapenko , linux-arm-kernel@lists.infradead.org, Andrey Ryabinin , linuxppc-dev@lists.ozlabs.org, dvyukov@google.com, Daniel Axtens Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+patchwork-linux-arm=patchwork.kernel.org@lists.infradead.org memcmp may bail out before accessing all the memory if the buffers contain differing bytes. kasan_memcmp calls memcmp with a stack array. Stack variables are not necessarily initialised (in the absence of a compiler plugin, at least). Sometimes this causes the memcpy to bail early thus fail to trigger kasan. Make sure the array initialised to zero in the code. No other test is dependent on the contents of an array on the stack. Cc: Andrey Ryabinin Cc: Alexander Potapenko Cc: Dmitry Vyukov Signed-off-by: Daniel Axtens Reviewed-by: Dmitry Vyukov --- lib/test_kasan.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/test_kasan.c b/lib/test_kasan.c index a130d75b9385..519b0f259e97 100644 --- a/lib/test_kasan.c +++ b/lib/test_kasan.c @@ -619,7 +619,7 @@ static noinline void __init kasan_memcmp(void) { char *ptr; size_t size = 24; - int arr[9]; + int arr[9] = {}; pr_info("out-of-bounds in memcmp\n"); ptr = kmalloc(size, GFP_KERNEL | __GFP_ZERO);