From patchwork Tue Mar 21 12:25:11 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mark Rutland X-Patchwork-Id: 72844 Return-Path: Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:604a:0:0:0:0:0 with SMTP id j10csp1746411wrt; Tue, 21 Mar 2023 05:30:04 -0700 (PDT) X-Google-Smtp-Source: AK7set8q4QpNkaszG7z2axg7W8b1sAbJapFxL8cqgGSCLJMq/ToyqQsvs4VNdc/p0uEHGBAfn4qc X-Received: by 2002:a62:1dc7:0:b0:627:e577:4326 with SMTP id d190-20020a621dc7000000b00627e5774326mr1840765pfd.17.1679401803960; Tue, 21 Mar 2023 05:30:03 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1679401803; cv=none; d=google.com; s=arc-20160816; b=nS/L8OFX56LSIQ4g5ah5jgNYaswIXtJz6DZ3msAkX2iLrFL82PFPW9ugyZ7n9L6rxA mw6hcu+OkBLsqQZb2fy4QPgTAx8zf/No7K30CtU+E8E1nfNK/ld3yfzejw8QKZo/Pcjv NAnHwLGoeLmJBZ3gqtKstgON27GA9ZEezsF3aBEHHjTfTnG4QFoW3pIl8biVqviuYg6O 3An7BRb48LT6ggfIrULIhBuqZJrJvXXJBKay/5JcVzeKtv8su9g9jj4f3FCAPIuJKxza ptXEPzX5GwnhoWalo2Z/76kGapA2OgCk6rcDrl7nvlGCJuzW+BgQNNgiTCkiTmAs/bKz ilrw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from; bh=5tgs+DztmVMf14Loa+5surlpqyiOo7WxwEVBnNUEeVg=; b=w1BXbdJNgWANu84EYtC9/XM3WH02bYqfHINg7z7Pj/F9Q2QpMVqwI9CA+wP9nrnyQs U6GtY5/46VliBTH9AkFq+JTChAv+MEswUBBy5YShG6eSJO627DTReXCW2l01p6UixHbv dkljrFYpwv8xkcZKhr1zzm30hsrkgCQL3Rr5st5aw9YKOGsbFRta3C4i2pfnvfPOVdlJ /t88VEeP0oIuftRb8686X1aimMnx1peoUDhFT/3QqDxShCawNT3/6n+LymdPRE+ujLIL vOuc8Xi7QTnAB28ttmRRu+zBmxNiYy+jI2N6sqO8bbP9G2Tln0iEqfkijtJv0TUtyrUe Os0A== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=arm.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id q14-20020a056a00150e00b00627e1988c78si10762188pfu.143.2023.03.21.05.29.49; Tue, 21 Mar 2023 05:30:03 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=arm.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230303AbjCUMZl (ORCPT + 99 others); Tue, 21 Mar 2023 08:25:41 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:45996 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230110AbjCUMZe (ORCPT ); Tue, 21 Mar 2023 08:25:34 -0400 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 12A163AAD; Tue, 21 Mar 2023 05:25:28 -0700 (PDT) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id BB502C14; Tue, 21 Mar 2023 05:26:11 -0700 (PDT) Received: from lakrids.cambridge.arm.com (usa-sjc-imap-foss1.foss.arm.com [10.121.207.14]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPA id 62DB03F71E; Tue, 21 Mar 2023 05:25:25 -0700 (PDT) From: Mark Rutland To: linux-kernel@vger.kernel.org Cc: agordeev@linux.ibm.com, aou@eecs.berkeley.edu, bp@alien8.de, catalin.marinas@arm.com, dave.hansen@linux.intel.com, davem@davemloft.net, gor@linux.ibm.com, hca@linux.ibm.com, linux-arch@vger.kernel.org, linux@armlinux.org.uk, mark.rutland@arm.com, mingo@redhat.com, palmer@dabbelt.com, paul.walmsley@sifive.com, robin.murphy@arm.com, tglx@linutronix.de, torvalds@linux-foundation.org, viro@zeniv.linux.org.uk, will@kernel.org Subject: [PATCH v2 1/4] lib: test copy_{to,from}_user() Date: Tue, 21 Mar 2023 12:25:11 +0000 Message-Id: <20230321122514.1743889-2-mark.rutland@arm.com> X-Mailer: git-send-email 2.30.2 In-Reply-To: <20230321122514.1743889-1-mark.rutland@arm.com> References: <20230321122514.1743889-1-mark.rutland@arm.com> MIME-Version: 1.0 X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_MED, SPF_HELO_NONE,SPF_NONE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1760980426026569748?= X-GMAIL-MSGID: =?utf-8?q?1760980426026569748?= Historically, implementations of raw_copy_{to,from}_user() haven't correctly handled the requirements laid forth in , e.g. as Al spotted on arm64: https://lore.kernel.org/all/YNSyZaZtPTmTa5P8@zeniv-ca.linux.org.uk/ Similarly, we've had other issues where incorrect fault handling logic lead to memory corruption, e.g. https://lore.kernel.org/linux-arm-kernel/Y44gVm7IEMXqilef@FVFF77S0Q05N.cambridge.arm.com/ These issues are easy to introduce and hard to spot as we don't have any systematic tests for the faulting paths. This patch adds KUnit tests for raw_copy_{to,from}_user() which systematically test the faulting paths, allowing us to detect and avoid problems such as those above. The test checks copy sizes of 0 to 128 bytes at every possible alignment relative to leading/trailing page boundaries to exhaustively check faulting and success paths. I've given this a spin on a few architectures using the KUnit QEMU harness, and it looks like most get *something* wrong. From those initial runs: * arm64's copy_to_user() under-reports the number of bytes copied in some cases, e.g. | post-destination bytes modified (dst_page[4082]=0x1, offset=4081, size=16, ret=15) * arm's copy_to_user() under-reports the number of bytes copied in some cases, and both copy_to_user() and copy_from_user() don't guarantee that at least a single byte is copied when a partial copy is possible, e.g. | post-destination bytes modified (dst_page[4068]=0x1, offset=4067, size=33, ret=32) | too few bytes consumed (offset=4068, size=32, ret=32) * i386's copy_from_user does not guarantee that at least a single byte is copied when a partial copit is possible, e.g. | too few bytes consumed (offset=4093, size=8, ret=8) * riscv's copy_to_user() and copy_from_user() don't guarantee that at least a single byte is copied when a partial copy is possible, e.g. | too few bytes consumed (offset=4095, size=2, ret=2) * s390 passes all tests * sparc's copy_from_user() over-reports the number of bbytes copied in some caes, e.g. | copied bytes incorrect (dst_page[0+0]=0x0, src_page[4095+0]=0xff, offset=4095, size=2, ret=1 * x86_64 passes all tests Signed-off-by: Mark Rutland Cc: Al Viro Cc: Albert Ou Cc: Borislav Petkov Cc: Catalin Marinas Cc: Dave Hansen Cc: David S. Miller Cc: Ingo Molnar Cc: Linus Torvalds Cc: Palmer Dabbelt Cc: Paul Walmsley Cc: Robin Murphy Cc: Russell King Cc: Thomas Gleixner Cc: Will Deacon Cc: linux-arch@vger.kernel.org --- lib/Kconfig.debug | 9 + lib/Makefile | 1 + lib/usercopy_kunit.c | 431 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 441 insertions(+) create mode 100644 lib/usercopy_kunit.c diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug index c8b379e2e9adc..bd737db4ef06a 100644 --- a/lib/Kconfig.debug +++ b/lib/Kconfig.debug @@ -2647,6 +2647,15 @@ config SIPHASH_KUNIT_TEST This is intended to help people writing architecture-specific optimized versions. If unsure, say N. +config USERCOPY_KUNIT_TEST + bool "KUnit tests for usercopy functions" if !KUNIT_ALL_TESTS + depends on KUNIT=y + default KUNIT_ALL_TESTS + help + Tests for faulting behaviour of copy_{to,from}_user(). + + If unsure, say N. + config TEST_UDELAY tristate "udelay test driver" help diff --git a/lib/Makefile b/lib/Makefile index baf2821f7a00f..37fb438e6c433 100644 --- a/lib/Makefile +++ b/lib/Makefile @@ -391,6 +391,7 @@ CFLAGS_fortify_kunit.o += $(DISABLE_STRUCTLEAK_PLUGIN) obj-$(CONFIG_FORTIFY_KUNIT_TEST) += fortify_kunit.o obj-$(CONFIG_STRSCPY_KUNIT_TEST) += strscpy_kunit.o obj-$(CONFIG_SIPHASH_KUNIT_TEST) += siphash_kunit.o +obj-$(CONFIG_USERCOPY_KUNIT_TEST) += usercopy_kunit.o obj-$(CONFIG_GENERIC_LIB_DEVMEM_IS_ALLOWED) += devmem_is_allowed.o diff --git a/lib/usercopy_kunit.c b/lib/usercopy_kunit.c new file mode 100644 index 0000000000000..45983952cc079 --- /dev/null +++ b/lib/usercopy_kunit.c @@ -0,0 +1,431 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Test cases for usercopy faulting behaviour + */ + +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt + +#include + +#include +#include +#include +#include +#include +#include + +/* + * Arbitrarily chosen user address for the test page. + */ +#define UBUF_ADDR_BASE SZ_2M + +struct usercopy_env { + struct mm_struct *mm; + void *kbuf; + struct page *ubuf_page; + void *ubuf; +}; + +struct usercopy_params { + long offset; + unsigned long size; +}; + +static void *usercopy_env_alloc(void) +{ + struct usercopy_env *env = kzalloc(sizeof(*env), GFP_KERNEL); + if (!env) + return NULL; + + env->kbuf = vmalloc(PAGE_SIZE); + if (!env->kbuf) + goto out_free_env; + + return env; + +out_free_env: + kfree(env); + return NULL; +} + +static void usercopy_env_free(struct usercopy_env *env) +{ + vfree(env->kbuf); + kfree(env); +} + +static void *usercopy_mm_alloc(struct usercopy_env *env) +{ + struct mm_struct *mm; + struct vm_area_struct *vma; + mm = mm_alloc(); + if (!mm) + return NULL; + + if (mmap_write_lock_killable(mm)) + goto out_free; + + vma = vm_area_alloc(mm); + if (!vma) + goto out_unlock; + + vma_set_anonymous(vma); + vma->vm_start = UBUF_ADDR_BASE; + vma->vm_end = UBUF_ADDR_BASE + PAGE_SIZE; + vm_flags_init(vma, VM_READ | VM_MAYREAD | VM_WRITE | VM_MAYWRITE); + vma->vm_page_prot = vm_get_page_prot(vma->vm_flags); + + if (insert_vm_struct(mm, vma)) + goto out_free_vma; + + mmap_write_unlock(mm); + return mm; + +out_free_vma: + vm_area_free(vma); +out_unlock: + mmap_write_unlock(mm); +out_free: + mmput(mm); + return NULL; +} + +static void usercopy_mm_free(struct mm_struct *mm) +{ + mmput(mm); +} + +static struct page *usercopy_ubuf_pin(struct usercopy_env *env) +{ + struct page *p = NULL; + + kthread_use_mm(env->mm); + pin_user_pages_unlocked(UBUF_ADDR_BASE, 1, &p, FOLL_LONGTERM); + kthread_unuse_mm(env->mm); + + return p; +} + +static void usercopy_ubuf_unpin(struct usercopy_env *env) +{ + unpin_user_page(env->ubuf_page); +} + +static int usercopy_test_init(struct kunit *test) +{ + struct usercopy_env *env; + + env = usercopy_env_alloc(); + if (!env) + return -ENOMEM; + + env->mm = usercopy_mm_alloc(env); + if (!env->mm) + goto out_free_env; + + env->ubuf_page = usercopy_ubuf_pin(env); + if (!env->ubuf_page) + goto out_free_mm; + + env->ubuf = kmap(env->ubuf_page); + if (!env->ubuf) + goto out_unpin_ubuf; + + test->priv = env; + + return 0; + +out_unpin_ubuf: + usercopy_ubuf_unpin(env); +out_free_mm: + usercopy_mm_free(env->mm); +out_free_env: + usercopy_env_free(env); + return -ENOMEM; +} + +static void usercopy_test_exit(struct kunit *test) +{ + struct usercopy_env *env = test->priv; + + kunmap(env->ubuf); + + usercopy_ubuf_unpin(env); + usercopy_mm_free(env->mm); + usercopy_env_free(env); +} + +static char buf_pattern(int offset) +{ + return offset & 0xff; +} + +static void buf_init_pattern(char *buf) +{ + for (int i = 0; i < PAGE_SIZE; i++) + buf[i] = buf_pattern(i); +} + +static void buf_init_zero(char *buf) +{ + memset(buf, 0, PAGE_SIZE); +} + +static void assert_size_valid(struct kunit *test, + const struct usercopy_params *params, + unsigned long ret) +{ + const unsigned long size = params->size; + const unsigned long offset = params->offset; + + if (ret > size) { + KUNIT_ASSERT_FAILURE(test, + "return value is impossibly large (offset=%ld, size=%lu, ret=%lu)", + offset, size, ret); + } + + /* + * When the user buffer starts within a faulting page, no bytes can be + * copied, so ret must equal size. + */ + if (offset < 0 || offset >= PAGE_SIZE) { + if (ret == size) + return; + + KUNIT_ASSERT_FAILURE(test, + "impossible copy did not fail (offset=%ld, size=%lu, ret=%lu)", + offset, size, ret); + } + + /* + * When the user buffer fits entirely within a non-faulting page, all + * bytes must be copied, so ret must equal 0. + */ + if (offset + size <= PAGE_SIZE) { + if (ret == 0) + return; + + KUNIT_ASSERT_FAILURE(test, + "completely possible copy failed (offset=%ld, size=%lu, ret=%lu)", + offset, size, ret); + } + + /* + * The buffer starts in a non-faulting page, but continues into a + * faulting page. At least one byte must be copied, and at most all the + * non-faulting bytes may be copied. + */ + if (ret == size) { + KUNIT_ASSERT_FAILURE(test, + "too few bytes consumed (offset=%ld, size=%lu, ret=%lu)", + offset, size, ret); + } + + if (ret < (offset + size) - PAGE_SIZE) { + KUNIT_ASSERT_FAILURE(test, + "too many bytes consumed (offset=%ld, size=%lu, ret=%lu)", + offset, size, ret); + } +} + +static void assert_src_valid(struct kunit *test, + const struct usercopy_params *params, + const char *src, long src_offset, + unsigned long ret) +{ + const unsigned long size = params->size; + const unsigned long offset = params->offset; + + /* + * A usercopy MUST NOT modify the source buffer. + */ + for (int i = 0; i < PAGE_SIZE; i++) { + char val = src[i]; + + if (val == buf_pattern(i)) + continue; + + KUNIT_ASSERT_FAILURE(test, + "source bytes modified (src[%d]=0x%x, offset=%ld, size=%lu, ret=%lu)", + i, offset, size, ret); + } +} + +static void assert_dst_valid(struct kunit *test, + const struct usercopy_params *params, + const char *dst, long dst_offset, + unsigned long ret) +{ + const unsigned long size = params->size; + const unsigned long offset = params->offset; + + /* + * A usercopy MUST NOT modify any bytes before the destination buffer. + */ + for (int i = 0; i < dst_offset; i++) { + char val = dst[i]; + + if (val == 0) + continue; + + KUNIT_ASSERT_FAILURE(test, + "pre-destination bytes modified (dst_page[%d]=0x%x, offset=%ld, size=%lu, ret=%lu)", + i, val, offset, size, ret); + } + + /* + * A usercopy MUST NOT modify any bytes after the end of the destination + * buffer. + */ + for (int i = dst_offset + size - ret; i < PAGE_SIZE; i++) { + char val = dst[i]; + + if (val == 0) + continue; + + KUNIT_ASSERT_FAILURE(test, + "post-destination bytes modified (dst_page[%d]=0x%x, offset=%ld, size=%lu, ret=%lu)", + i, val, offset, size, ret); + } +} + +static void assert_copy_valid(struct kunit *test, + const struct usercopy_params *params, + const char *dst, long dst_offset, + const char *src, long src_offset, + unsigned long ret) +{ + const unsigned long size = params->size; + const unsigned long offset = params->offset; + + /* + * Have we actually copied the bytes we expected to? + */ + for (int i = 0; i < params->size - ret; i++) { + char dst_val = dst[dst_offset + i]; + char src_val = src[src_offset + i]; + + if (dst_val == src_val) + continue; + + KUNIT_ASSERT_FAILURE(test, + "copied bytes incorrect (dst_page[%ld+%d]=0x%x, src_page[%ld+%d]=0x%x, offset=%ld, size=%lu, ret=%lu", + dst_offset, i, dst_val, + src_offset, i, src_val, + offset, size, ret); + } +} + +static unsigned long do_copy_to_user(const struct usercopy_env *env, + const struct usercopy_params *params) +{ + void __user *uptr = (void __user *)UBUF_ADDR_BASE + params->offset; + void *kptr = env->kbuf; + unsigned long ret; + + kthread_use_mm(env->mm); + ret = raw_copy_to_user(uptr, kptr, params->size); + kthread_unuse_mm(env->mm); + + return ret; +} + +static unsigned long do_copy_from_user(const struct usercopy_env *env, + const struct usercopy_params *params) +{ + void __user *uptr = (void __user *)UBUF_ADDR_BASE + params->offset; + void *kptr = env->kbuf; + unsigned long ret; + + kthread_use_mm(env->mm); + ret = raw_copy_from_user(kptr, uptr, params->size); + kthread_unuse_mm(env->mm); + + return ret; +} + +/* + * Generate the size and offset combinations to test. + * + * Usercopies may have different paths for small/medium/large copies, but + * typically max out at 64 byte chunks. We test sizes 0 to 128 bytes to check + * all combinations of leading/trailing chunks and bulk copies. + * + * For each size, we test every offset relative to a leading and trailing page + * boundary (i.e. [size, 0] and [PAGE_SIZE - size, PAGE_SIZE]) to check every + * possible faulting boundary. + */ +#define for_each_size_offset(size, offset) \ + for (unsigned long size = 0; size <= 128; size++) \ + for (long offset = -size; \ + offset <= (long)PAGE_SIZE; \ + offset = (offset ? offset + 1: (PAGE_SIZE - size))) + +static void test_copy_to_user(struct kunit *test) +{ + const struct usercopy_env *env = test->priv; + + for_each_size_offset(size, offset) { + const struct usercopy_params params = { + .size = size, + .offset = offset, + }; + unsigned long ret; + + buf_init_zero(env->ubuf); + buf_init_pattern(env->kbuf); + + ret = do_copy_to_user(env, ¶ms); + + assert_size_valid(test, ¶ms, ret); + assert_src_valid(test, ¶ms, env->kbuf, 0, ret); + assert_dst_valid(test, ¶ms, env->ubuf, params.offset, ret); + assert_copy_valid(test, ¶ms, + env->ubuf, params.offset, + env->kbuf, 0, + ret); + } +} + +static void test_copy_from_user(struct kunit *test) +{ + const struct usercopy_env *env = test->priv; + + for_each_size_offset(size, offset) { + const struct usercopy_params params = { + .size = size, + .offset = offset, + }; + unsigned long ret; + + buf_init_zero(env->kbuf); + buf_init_pattern(env->ubuf); + + ret = do_copy_from_user(env, ¶ms); + + assert_size_valid(test, ¶ms, ret); + assert_src_valid(test, ¶ms, env->ubuf, params.offset, ret); + assert_dst_valid(test, ¶ms, env->kbuf, 0, ret); + assert_copy_valid(test, ¶ms, + env->kbuf, 0, + env->ubuf, params.offset, + ret); + } +} + +static struct kunit_case usercopy_cases[] = { + KUNIT_CASE(test_copy_to_user), + KUNIT_CASE(test_copy_from_user), + { /* sentinel */ } +}; + +static struct kunit_suite usercopy_suite = { + .name = "usercopy", + .init = usercopy_test_init, + .exit = usercopy_test_exit, + .test_cases = usercopy_cases, +}; +kunit_test_suites(&usercopy_suite); + +MODULE_AUTHOR("Mark Rutland "); +MODULE_LICENSE("GPL v2"); From patchwork Tue Mar 21 12:25:12 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mark Rutland X-Patchwork-Id: 72843 Return-Path: Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:604a:0:0:0:0:0 with SMTP id j10csp1746337wrt; Tue, 21 Mar 2023 05:29:53 -0700 (PDT) X-Google-Smtp-Source: AK7set/h245LpR3gduTL/zDQ4NmfCaGZAptqwmHvH/G5eGd820ZT0ViHbOOTEllMwMEhW8s69hIq X-Received: by 2002:a17:90b:1c8c:b0:23f:91e5:103d with SMTP id oo12-20020a17090b1c8c00b0023f91e5103dmr2609914pjb.36.1679401793428; Tue, 21 Mar 2023 05:29:53 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1679401793; cv=none; d=google.com; s=arc-20160816; b=AcgU5jJjQsmT16fRAbo9C+l/j9SyZ8LTOG60Hr31y5L5xVERr2i2fNmEHlsJMtdgX+ X1K+LyWSN7E1PIe/HwAqtxVOOb7iyzPvliSDxTKOplqcMHG280bf//trrcC2/lVvHN3f x/XEBS3ntmEq/ZpZ06QZBKErs9Er1WsTnBllsLxpw+ktlm6EZOsUSFH5mG4u9N1oOKdd nZrDLQ8m6CAo7KmYsGJ2s72u0cNMrJiSGa4EthXbll6ghpMM2avirGCqGi9QdCuqOTsi goAzHsSDxDSsRJ4LGaV3K/9sghX2hFcxhsZ9EtDHTFJUXotvFgJZRJWRjrjM7khO86H9 biaQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from; bh=Ff96fXAPNVECEvvVb58xySmz/zvgja6mffHsb2BAbS4=; b=MxW0qPMVYjbVFoX3aaFxF4QcJKeK4JXoMdhSjAxLrJgBVG8gGTjnJedmKqEZKQTL1S V2RKdDIu9Kd6YitJw/qNDcETaOwkphbgxPOsigWuCdsEEpcjHoT4SEN1NaIf5T57VmA5 00foZSgqpRIGsrP5zw4NcdtyeVBakLe1tlTwu3IxmfTVH3gFVaHBQyp3S+0sFQKmnGeW uJNIx7AG5qN0V5FrUu34UVTWK5jun+Aut+wHDbw8YHFIfqQnutCvhEdqGrOpcWjhor6d oF6S8tr1BaOzq4dJkaOiOeQwRJrw3VNQYJx7CtmX/IG4J02romR2ei6KquqLRXN9QUYH GtWA== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=arm.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id c1-20020a17090ad90100b002296a8ff568si12210094pjv.152.2023.03.21.05.29.39; Tue, 21 Mar 2023 05:29:53 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=arm.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230505AbjCUMZq (ORCPT + 99 others); Tue, 21 Mar 2023 08:25:46 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46006 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229987AbjCUMZg (ORCPT ); Tue, 21 Mar 2023 08:25:36 -0400 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 5533A32E59; Tue, 21 Mar 2023 05:25:31 -0700 (PDT) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 0F1EE1424; Tue, 21 Mar 2023 05:26:15 -0700 (PDT) Received: from lakrids.cambridge.arm.com (usa-sjc-imap-foss1.foss.arm.com [10.121.207.14]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPA id AAF203F71E; Tue, 21 Mar 2023 05:25:28 -0700 (PDT) From: Mark Rutland To: linux-kernel@vger.kernel.org Cc: agordeev@linux.ibm.com, aou@eecs.berkeley.edu, bp@alien8.de, catalin.marinas@arm.com, dave.hansen@linux.intel.com, davem@davemloft.net, gor@linux.ibm.com, hca@linux.ibm.com, linux-arch@vger.kernel.org, linux@armlinux.org.uk, mark.rutland@arm.com, mingo@redhat.com, palmer@dabbelt.com, paul.walmsley@sifive.com, robin.murphy@arm.com, tglx@linutronix.de, torvalds@linux-foundation.org, viro@zeniv.linux.org.uk, will@kernel.org Subject: [PATCH v2 2/4] lib: test clear_user() Date: Tue, 21 Mar 2023 12:25:12 +0000 Message-Id: <20230321122514.1743889-3-mark.rutland@arm.com> X-Mailer: git-send-email 2.30.2 In-Reply-To: <20230321122514.1743889-1-mark.rutland@arm.com> References: <20230321122514.1743889-1-mark.rutland@arm.com> MIME-Version: 1.0 X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_MED, SPF_HELO_NONE,SPF_NONE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1760980414867504938?= X-GMAIL-MSGID: =?utf-8?q?1760980414867504938?= The clear_user() function follows the same conventions as copy_{to,from}_user(), and presumably has identical requirements on the return value. Test it in the same way. I've given this a spin on a few architectures using the KUnit QEMU harness, and it looks like most get *something* wrong, or I've misunderstood and clear_user() doesn't have the same requirements as copy_{to,from}_user()). From those initial runs: * arm, arm64, i386, riscv, x86_64 don't ensure that at least 1 byte is zeroed when a partial zeroing is possible, e.g. | too few bytes consumed (offset=4095, size=2, ret=2) | too few bytes consumed (offset=4093, size=4, ret=4) | too few bytes consumed (offset=4089, size=8, ret=8) * s390 reports that some bytes have been zeroed even when they haven't, e.g. | zeroed bytes incorrect (dst_page[4031+64]=0xca, offset=4031, size=66, ret=1 * sparc passses all tests Signed-off-by: Mark Rutland Cc: Al Viro Cc: Albert Ou Cc: Alexander Gordeev Cc: Borislav Petkov Cc: Catalin Marinas Cc: Dave Hansen Cc: Heiko Carstens Cc: Ingo Molnar Cc: Linus Torvalds Cc: Palmer Dabbelt Cc: Paul Walmsley Cc: Robin Murphy Cc: Russell King Cc: Thomas Gleixner Cc: Vasily Gorbik Cc: Will Deacon Cc: linux-arch@vger.kernel.org Reviewed-by: Catalin Marinas --- lib/usercopy_kunit.c | 89 ++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 82 insertions(+), 7 deletions(-) diff --git a/lib/usercopy_kunit.c b/lib/usercopy_kunit.c index 45983952cc079..1ec0d5bbc179a 100644 --- a/lib/usercopy_kunit.c +++ b/lib/usercopy_kunit.c @@ -155,6 +155,11 @@ static void usercopy_test_exit(struct kunit *test) usercopy_env_free(env); } +static char buf_zero(int offset) +{ + return 0; +} + static char buf_pattern(int offset) { return offset & 0xff; @@ -230,6 +235,7 @@ static void assert_size_valid(struct kunit *test, static void assert_src_valid(struct kunit *test, const struct usercopy_params *params, + char (*buf_expected)(int), const char *src, long src_offset, unsigned long ret) { @@ -240,9 +246,10 @@ static void assert_src_valid(struct kunit *test, * A usercopy MUST NOT modify the source buffer. */ for (int i = 0; i < PAGE_SIZE; i++) { + char expected = buf_expected(i); char val = src[i]; - if (val == buf_pattern(i)) + if (val == expected) continue; KUNIT_ASSERT_FAILURE(test, @@ -253,6 +260,7 @@ static void assert_src_valid(struct kunit *test, static void assert_dst_valid(struct kunit *test, const struct usercopy_params *params, + char (*buf_expected)(int), const char *dst, long dst_offset, unsigned long ret) { @@ -263,9 +271,10 @@ static void assert_dst_valid(struct kunit *test, * A usercopy MUST NOT modify any bytes before the destination buffer. */ for (int i = 0; i < dst_offset; i++) { + char expected = buf_expected(i); char val = dst[i]; - if (val == 0) + if (val == expected) continue; KUNIT_ASSERT_FAILURE(test, @@ -278,9 +287,10 @@ static void assert_dst_valid(struct kunit *test, * buffer. */ for (int i = dst_offset + size - ret; i < PAGE_SIZE; i++) { + char expected = buf_expected(i); char val = dst[i]; - if (val == 0) + if (val == expected) continue; KUNIT_ASSERT_FAILURE(test, @@ -316,6 +326,29 @@ static void assert_copy_valid(struct kunit *test, } } +static void assert_clear_valid(struct kunit *test, + const struct usercopy_params *params, + const char *dst, long dst_offset, + unsigned long ret) +{ + const unsigned long size = params->size; + const unsigned long offset = params->offset; + + /* + * Have we actually zeroed the bytes we expected to? + */ + for (int i = 0; i < params->size - ret; i++) { + char dst_val = dst[dst_offset + i]; + + if (dst_val == 0) + continue; + + KUNIT_ASSERT_FAILURE(test, + "zeroed bytes incorrect (dst_page[%ld+%d]=0x%x, offset=%ld, size=%lu, ret=%lu", + dst_offset, i, dst_val, + offset, size, ret); + } +} static unsigned long do_copy_to_user(const struct usercopy_env *env, const struct usercopy_params *params) { @@ -344,6 +377,19 @@ static unsigned long do_copy_from_user(const struct usercopy_env *env, return ret; } +static unsigned long do_clear_user(const struct usercopy_env *env, + const struct usercopy_params *params) +{ + void __user *uptr = (void __user *)UBUF_ADDR_BASE + params->offset; + unsigned long ret; + + kthread_use_mm(env->mm); + ret = clear_user(uptr, params->size); + kthread_unuse_mm(env->mm); + + return ret; +} + /* * Generate the size and offset combinations to test. * @@ -378,8 +424,10 @@ static void test_copy_to_user(struct kunit *test) ret = do_copy_to_user(env, ¶ms); assert_size_valid(test, ¶ms, ret); - assert_src_valid(test, ¶ms, env->kbuf, 0, ret); - assert_dst_valid(test, ¶ms, env->ubuf, params.offset, ret); + assert_src_valid(test, ¶ms, buf_pattern, + env->kbuf, 0, ret); + assert_dst_valid(test, ¶ms, buf_zero, + env->ubuf, params.offset, ret); assert_copy_valid(test, ¶ms, env->ubuf, params.offset, env->kbuf, 0, @@ -404,8 +452,10 @@ static void test_copy_from_user(struct kunit *test) ret = do_copy_from_user(env, ¶ms); assert_size_valid(test, ¶ms, ret); - assert_src_valid(test, ¶ms, env->ubuf, params.offset, ret); - assert_dst_valid(test, ¶ms, env->kbuf, 0, ret); + assert_src_valid(test, ¶ms, buf_pattern, + env->ubuf, params.offset, ret); + assert_dst_valid(test, ¶ms, buf_zero, + env->kbuf, 0, ret); assert_copy_valid(test, ¶ms, env->kbuf, 0, env->ubuf, params.offset, @@ -413,9 +463,34 @@ static void test_copy_from_user(struct kunit *test) } } +static void test_clear_user(struct kunit *test) +{ + const struct usercopy_env *env = test->priv; + + for_each_size_offset(size, offset) { + const struct usercopy_params params = { + .size = size, + .offset = offset, + }; + unsigned long ret; + + buf_init_pattern(env->ubuf); + + ret = do_clear_user(env, ¶ms); + + assert_size_valid(test, ¶ms, ret); + assert_dst_valid(test, ¶ms, buf_pattern, + env->ubuf, params.offset, ret); + assert_clear_valid(test, ¶ms, + env->ubuf, params.offset, + ret); + } +} + static struct kunit_case usercopy_cases[] = { KUNIT_CASE(test_copy_to_user), KUNIT_CASE(test_copy_from_user), + KUNIT_CASE(test_clear_user), { /* sentinel */ } }; From patchwork Tue Mar 21 12:25:13 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mark Rutland X-Patchwork-Id: 72849 Return-Path: Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:604a:0:0:0:0:0 with SMTP id j10csp1751128wrt; Tue, 21 Mar 2023 05:40:18 -0700 (PDT) X-Google-Smtp-Source: AK7set+mXoQd3KDGgzgSVp+oEIdDUEllEiaR4tZh2oOEBJpFlqmkL0SfM+BEpttlo513Hl4S6SvW X-Received: by 2002:a17:90b:4f46:b0:237:d2d8:3264 with SMTP id pj6-20020a17090b4f4600b00237d2d83264mr2429353pjb.40.1679402418351; Tue, 21 Mar 2023 05:40:18 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1679402418; cv=none; d=google.com; s=arc-20160816; b=rtGjFHbcTWEuizYdYm5NKBOf+RsedtQZF7+X5TkmwAggzYF55z3SgGIlm1ROqYkxzZ xLIPad2w/XA4mh6Sp36K3kckOQyvjn735hgkCda+IkRb4McW60b68nkBFnJpA04L/Scw 8YXRWeDvyMBGuHoExHNzvJpDL4dr26HwRyZAElcjXAdHxBqro27xLdWXeXDzQqVpnQ/d L9BXYqxyzlNeHfm2pvBBBij6SND4UNtKJ66LlIfxSYLS1MBkyaSQyTHY8YH2504K1HB6 cd02eHpUR68+ajjYgYCTWkYbsaBKEdldtY2hqxYQeNRD77pW4sW7Od0L4hkJGaryhGPc mmNw== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from; bh=HOL3ygBct3J8jE5GfpBBcSmoXtqdXOwoRXXIYfEdcTA=; b=c4KSk231c3HW/K8EWv+enjUWf9cHYYAG98uOGb8V7XQ0ncQ6MIBppZPpBeK4iZJlmX ehzMaMVGhDfImkpq4PR+StP+OSa9wsCX99rJRqXAWAp8XGwH0GF+8SwF4bQz5iFbEaMM Af5qXtreNDbJOqQIOPlBsw612/Rxfn+5mAJ3wNBIn3/yOGOcNl1eyqDqwcKsdf1kmVGF +9X0Pr0Zhg55iklUpQJ2v2OOcBCqhx49ISaV5YmE0zjGTgfEJt5ErYPXdWekf0vVQKXS ZGqyewYOhJCyQnLVxmniPrSPKFqZUTQDQzfTlKlnUUww4DqsiofLZqXpqki0DYRfu95W c5Vg== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=arm.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id bh4-20020a056a02020400b0050be35ede38si14062253pgb.486.2023.03.21.05.40.05; Tue, 21 Mar 2023 05:40:18 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=arm.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231193AbjCUMZt (ORCPT + 99 others); Tue, 21 Mar 2023 08:25:49 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:46090 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230178AbjCUMZh (ORCPT ); Tue, 21 Mar 2023 08:25:37 -0400 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 7F59B30CA; Tue, 21 Mar 2023 05:25:34 -0700 (PDT) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 39564AD7; Tue, 21 Mar 2023 05:26:18 -0700 (PDT) Received: from lakrids.cambridge.arm.com (usa-sjc-imap-foss1.foss.arm.com [10.121.207.14]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPA id D4EAB3F71E; Tue, 21 Mar 2023 05:25:31 -0700 (PDT) From: Mark Rutland To: linux-kernel@vger.kernel.org Cc: agordeev@linux.ibm.com, aou@eecs.berkeley.edu, bp@alien8.de, catalin.marinas@arm.com, dave.hansen@linux.intel.com, davem@davemloft.net, gor@linux.ibm.com, hca@linux.ibm.com, linux-arch@vger.kernel.org, linux@armlinux.org.uk, mark.rutland@arm.com, mingo@redhat.com, palmer@dabbelt.com, paul.walmsley@sifive.com, robin.murphy@arm.com, tglx@linutronix.de, torvalds@linux-foundation.org, viro@zeniv.linux.org.uk, will@kernel.org Subject: [PATCH v2 3/4] arm64: fix __raw_copy_to_user semantics Date: Tue, 21 Mar 2023 12:25:13 +0000 Message-Id: <20230321122514.1743889-4-mark.rutland@arm.com> X-Mailer: git-send-email 2.30.2 In-Reply-To: <20230321122514.1743889-1-mark.rutland@arm.com> References: <20230321122514.1743889-1-mark.rutland@arm.com> MIME-Version: 1.0 X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_MED, SPF_HELO_NONE,SPF_NONE,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1760981070221354439?= X-GMAIL-MSGID: =?utf-8?q?1760981070221354439?= For some combinations of sizes and alignments __{arch,raw}_copy_to_user will copy some bytes between (to + size - N) and (to + size), but will never modify bytes past (to + size). This violates the documentation in , which states: > If raw_copy_{to,from}_user(to, from, size) returns N, size - N bytes > starting at to must become equal to the bytes fetched from the > corresponding area starting at from. All data past to + size - N must > be left unmodified. This can be demonstrated through testing, e.g. | # test_copy_to_user: EXPECTATION FAILED at lib/usercopy_kunit.c:287 | post-destination bytes modified (dst_page[4082]=0x1, offset=4081, size=16, ret=15) | [FAILED] 16 byte copy This happens because the __arch_copy_to_user() can make unaligned stores to the userspace buffer, and the ARM architecture permits (but does not require) that such unaligned stores write some bytes before raising a fault (per ARM DDI 0487I.a Section B2.2.1 and Section B2.7.1). The extable fixup handlers in __arch_copy_to_user() assume that any faulting store has failed entirely, and so under-report the number of bytes copied when an unaligned store writes some bytes before faulting. The only architecturally guaranteed way to avoid this is to only use aligned stores to write to user memory. This patch rewrites __arch_copy_to_user() to only access the user buffer with aligned stores, such that the bytes written can always be determined reliably. For correctness, I've tested this exhaustively for sizes 0 to 128 against every possible alignment relative to a leading and trailing page boundary. I've also boot tested and run a few kernel builds with the new implementations. For performance, I've benchmarked this on a variety of CPU implementations, and across the board this appears at least as good as (or marginally better than) the current implementation of copy_to_user(). Timing a kernel build indicates the same, though the difference is very close to noise. We do not have a similar bug in __{arch,raw}_copy_from_user(), as faults taken on loads from user memory do not have side-effects. We do have similar issues in __arch_clear_user(), which will be addresssed in a subsequent patch. Signed-off-by: Mark Rutland Cc: Catalin Marinas Cc: Robin Murphy Cc: Will Deacon --- arch/arm64/lib/copy_to_user.S | 202 +++++++++++++++++++++++++++------- 1 file changed, 161 insertions(+), 41 deletions(-) diff --git a/arch/arm64/lib/copy_to_user.S b/arch/arm64/lib/copy_to_user.S index 8022317726085..fa603487e8571 100644 --- a/arch/arm64/lib/copy_to_user.S +++ b/arch/arm64/lib/copy_to_user.S @@ -9,6 +9,14 @@ #include #include +#define USER_OFF(off, x...) USER(fixup_offset_##off, x) + +#define FIXUP_OFFSET(n) \ +fixup_offset_##n: \ + sub x0, x3, x0; \ + sub x0, x0, n; \ + ret + /* * Copy to user space from a kernel buffer (alignment handled by the hardware) * @@ -18,56 +26,168 @@ * x2 - n * Returns: * x0 - bytes not copied + * + * Unlike a memcpy(), we need to handle faults on user addresses, and we need + * to precisely report the number of bytes (not) copied. We must only use + * aligned single-copy-atomic stores to write to user memory, as stores which + * are not single-copy-atomic (e.g. unaligned stores, STP, ASMID stores) can be + * split into separate byte accesses (per ARM DDI 0487I.a, Section B2.2.1) and + * some arbitatrary set of those byte accesses might occur prior to a fault + * being raised (per per ARM DDI 0487I.a, Section B2.7.1). + * + * We use STTR to write to user memory, which has 1/2/4/8 byte forms, and the + * user address ('to') might have arbitrary alignment, so we must handle + * misalignment up to 8 bytes. */ - .macro ldrb1 reg, ptr, val - ldrb \reg, [\ptr], \val - .endm +SYM_FUNC_START(__arch_copy_to_user) + /* + * The end address. Fixup handlers will use this to calculate + * the number of bytes copied. + */ + add x3, x0, x2 - .macro strb1 reg, ptr, val - user_ldst 9998f, sttrb, \reg, \ptr, \val - .endm + /* + * Tracing of a kernel build indicates that for the vast + * majority of calls to copy_to_user(), 'to' is aligned to 8 + * bytes. When this is the case, we want to skip to the bulk + * copy as soon as possible. + */ + ands x4, x0, 0x7 + b.eq body - .macro ldrh1 reg, ptr, val - ldrh \reg, [\ptr], \val - .endm + /* + * For small unaligned copies, it's not worth trying to be + * optimal. + */ + cmp x2, #8 + b.lo bytewise_loop - .macro strh1 reg, ptr, val - user_ldst 9997f, sttrh, \reg, \ptr, \val - .endm + /* + * Calculate the distance to the next 8-byte boundary. + */ + mov x5, #8 + sub x4, x5, x4 - .macro ldr1 reg, ptr, val - ldr \reg, [\ptr], \val - .endm +SYM_INNER_LABEL(head_realign_1b, SYM_L_LOCAL) + tbz x4, #0, head_realign_2b - .macro str1 reg, ptr, val - user_ldst 9997f, sttr, \reg, \ptr, \val - .endm + ldrb w8, [x1], #1 +USER_OFF(0, sttrb w8, [x0]) + add x0, x0, #1 - .macro ldp1 reg1, reg2, ptr, val - ldp \reg1, \reg2, [\ptr], \val - .endm +SYM_INNER_LABEL(head_realign_2b, SYM_L_LOCAL) + tbz x4, #1, head_realign_4b - .macro stp1 reg1, reg2, ptr, val - user_stp 9997f, \reg1, \reg2, \ptr, \val - .endm + ldrh w8, [x1], #2 +USER_OFF(0, sttrh w8, [x0]) + add x0, x0, #2 -end .req x5 -srcin .req x15 -SYM_FUNC_START(__arch_copy_to_user) - add end, x0, x2 - mov srcin, x1 -#include "copy_template.S" - mov x0, #0 - ret +SYM_INNER_LABEL(head_realign_4b, SYM_L_LOCAL) + tbz x4, #2, head_realigned - // Exception fixups -9997: cmp dst, dstin - b.ne 9998f - // Before being absolutely sure we couldn't copy anything, try harder - ldrb tmp1w, [srcin] -USER(9998f, sttrb tmp1w, [dst]) - add dst, dst, #1 -9998: sub x0, end, dst // bytes not copied - ret + ldr w8, [x1], #4 +USER_OFF(0, sttr w8, [x0]) + add x0, x0, #4 + +SYM_INNER_LABEL(head_realigned, SYM_L_LOCAL) + /* + * Any 1-7 byte misalignment has now been fixed; subtract this + * misalignment from the remaining size. + */ + sub x2, x2, x4 + +SYM_INNER_LABEL(body, SYM_L_LOCAL) + cmp x2, #64 + b.lo tail_32b + +SYM_INNER_LABEL(body_64b_loop, SYM_L_LOCAL) + ldp x8, x9, [x1, #0] + ldp x10, x11, [x1, #16] + ldp x12, x13, [x1, #32] + ldp x14, x15, [x1, #48] +USER_OFF(0, sttr x8, [x0, #0]) +USER_OFF(8, sttr x9, [x0, #8]) +USER_OFF(16, sttr x10, [x0, #16]) +USER_OFF(24, sttr x11, [x0, #24]) +USER_OFF(32, sttr x12, [x0, #32]) +USER_OFF(40, sttr x13, [x0, #40]) +USER_OFF(48, sttr x14, [x0, #48]) +USER_OFF(56, sttr x15, [x0, #56]) + add x0, x0, #64 + add x1, x1, #64 + sub x2, x2, #64 + + cmp x2, #64 + b.hs body_64b_loop + +SYM_INNER_LABEL(tail_32b, SYM_L_LOCAL) + tbz x2, #5, tail_16b + + ldp x8, x9, [x1, #0] + ldp x10, x11, [x1, #16] +USER_OFF(0, sttr x8, [x0, #0]) +USER_OFF(8, sttr x9, [x0, #8]) +USER_OFF(16, sttr x10, [x0, #16]) +USER_OFF(24, sttr x11, [x0, #24]) + add x0, x0, #32 + add x1, x1, #32 + +SYM_INNER_LABEL(tail_16b, SYM_L_LOCAL) + tbz x2, #4, tail_8b + + ldp x8, x9, [x1], #16 +USER_OFF(0, sttr x8, [x0, #0]) +USER_OFF(8, sttr x9, [x0, #8]) + add x0, x0, #16 + +SYM_INNER_LABEL(tail_8b, SYM_L_LOCAL) + tbz x2, #3, tail_4b + + ldr x8, [x1], #8 +USER_OFF(0, sttr x8, [x0]) + add x0, x0, #8 + +SYM_INNER_LABEL(tail_4b, SYM_L_LOCAL) + tbz x2, #2, tail_2b + + ldr w8, [x1], #4 +USER_OFF(0, sttr w8, [x0]) + add x0, x0, #4 + +SYM_INNER_LABEL(tail_2b, SYM_L_LOCAL) + tbz x2, #1, tail_1b + + ldrh w8, [x1], #2 +USER_OFF(0, sttrh w8, [x0]) + add x0, x0, #2 + +SYM_INNER_LABEL(tail_1b, SYM_L_LOCAL) + tbz x2, #0, done + + ldrb w8, [x1] +USER_OFF(0, sttrb w8, [x0]) + +SYM_INNER_LABEL(done, SYM_L_LOCAL) + mov x0, xzr + ret + +SYM_INNER_LABEL(bytewise_loop, SYM_L_LOCAL) + cbz x2, done + + ldrb w8, [x1], #1 +USER_OFF(0, sttrb w8, [x0]) + add x0, x0, #1 + sub x2, x2, #1 + + b bytewise_loop + +FIXUP_OFFSET(0) +FIXUP_OFFSET(8) +FIXUP_OFFSET(16) +FIXUP_OFFSET(24) +FIXUP_OFFSET(32) +FIXUP_OFFSET(40) +FIXUP_OFFSET(48) +FIXUP_OFFSET(56) SYM_FUNC_END(__arch_copy_to_user) EXPORT_SYMBOL(__arch_copy_to_user) From patchwork Tue Mar 21 12:25:14 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mark Rutland X-Patchwork-Id: 72845 Return-Path: Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:604a:0:0:0:0:0 with SMTP id j10csp1746519wrt; Tue, 21 Mar 2023 05:30:17 -0700 (PDT) X-Google-Smtp-Source: AK7set+MXBBkTqDPqIClZFEG2KZsStWleeFOswzYBLbAVX3ukCEjXwny64aCCP96mCYt4gdMngwF X-Received: by 2002:a17:902:d2ca:b0:19a:727e:d4f3 with SMTP id n10-20020a170902d2ca00b0019a727ed4f3mr19964726plc.5.1679401817055; Tue, 21 Mar 2023 05:30:17 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1679401817; cv=none; d=google.com; s=arc-20160816; b=0IY53P9EBX8tdTnXw85rmEq/185XBUH3JgpV/2++phpWyzgE4A1H6pB9o6zkMuDhjc vBKKf+zHuqTcbAaniPJcbFCroj8FPvdw/hmKrgy/Xu+gxlfYe6B2eIBQBTX7/mqHQ3Qp b76BPm8fj3x6riEZasAfG+zLz2AkuecLNLDiImXZbqW2KkFMNFimj7b7WswZa0oL0Fak +vZ47h6c0k6zJGgv9cTrro5XE4XAKswgmairtRREk8dTMjjrbWoj0x++ajJkgdcFedXk 8q5VgBTAOuXtwCdUDtQpXpaFbL+pLXTYWnshcg3+OZ8mqNY8lROu/zhiXGBl2Ff6xk54 Hq0A== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :references:in-reply-to:message-id:date:subject:cc:to:from; bh=fKa9OEzbO87aw7DThlyfuTYmn76sAAKfv8We/P1td/s=; b=mWZ5HAQGPczw/UW8/EbJYrP7QJGh064IR6HMTQVAlZfqKN2bylOIetxPY/BBpNnmT7 6lRxI6Ej9Uqf3sdbbeZGudwRtxkQNFTqlGBBC1rUyTkzsfunbrBUV6DytrsZirkBUXVP +rr9N6+zQaCw6TpkmhZ33sh4oKJY9AhfS2ibrfMMOAlF1I8rYGPZxHZaKbb11vbHIJsG ms2Oqt+DZRt/7zSe5SnE3jY4eIOzWkOPK8bRj4eMlmZ8nTB0Jk3gloPUFbt565bnwBgn 4hbtPM4QeHFrcoDXxqomWbljNsdW02/HsXfVYX18h7EkdJD4oGUmiv33qR7LAKjG1urW WYfA== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=arm.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id t20-20020a170902dcd400b00188fead2329si8758578pll.135.2023.03.21.05.29.59; Tue, 21 Mar 2023 05:30:17 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=fail (p=NONE sp=NONE dis=NONE) header.from=arm.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231205AbjCUMZx (ORCPT + 99 others); Tue, 21 Mar 2023 08:25:53 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:45996 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230221AbjCUMZk (ORCPT ); Tue, 21 Mar 2023 08:25:40 -0400 Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id B2F4D497E0; Tue, 21 Mar 2023 05:25:37 -0700 (PDT) Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 5E46BC14; Tue, 21 Mar 2023 05:26:21 -0700 (PDT) Received: from lakrids.cambridge.arm.com (usa-sjc-imap-foss1.foss.arm.com [10.121.207.14]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPA id 060103F71E; Tue, 21 Mar 2023 05:25:34 -0700 (PDT) From: Mark Rutland To: linux-kernel@vger.kernel.org Cc: agordeev@linux.ibm.com, aou@eecs.berkeley.edu, bp@alien8.de, catalin.marinas@arm.com, dave.hansen@linux.intel.com, davem@davemloft.net, gor@linux.ibm.com, hca@linux.ibm.com, linux-arch@vger.kernel.org, linux@armlinux.org.uk, mark.rutland@arm.com, mingo@redhat.com, palmer@dabbelt.com, paul.walmsley@sifive.com, robin.murphy@arm.com, tglx@linutronix.de, torvalds@linux-foundation.org, viro@zeniv.linux.org.uk, will@kernel.org Subject: [PATCH v2 4/4] arm64: fix clear_user() semantics Date: Tue, 21 Mar 2023 12:25:14 +0000 Message-Id: <20230321122514.1743889-5-mark.rutland@arm.com> X-Mailer: git-send-email 2.30.2 In-Reply-To: <20230321122514.1743889-1-mark.rutland@arm.com> References: <20230321122514.1743889-1-mark.rutland@arm.com> MIME-Version: 1.0 X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00,RCVD_IN_DNSWL_MED, SPF_HELO_NONE,SPF_NONE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1760980439731678900?= X-GMAIL-MSGID: =?utf-8?q?1760980439731678900?= Generally, clear_user() is supposed to behave the same way as raw_copy_{to,from}_user(), which per requires: > If raw_copy_{to,from}_user(to, from, size) returns N, size - N bytes > starting at to must become equal to the bytes fetched from the > corresponding area starting at from. All data past to + size - N must > be left unmodified. > > If copying succeeds, the return value must be 0. If some data cannot > be fetched, it is permitted to copy less than had been fetched; the > only hard requirement is that not storing anything at all (i.e. > returning size) should happen only when nothing could be copied. In > other words, you don't have to squeeze as much as possible - it is > allowed, but not necessary. Unfortunately arm64's implementation of clear_user() may fail in cases where some bytes could be written, which can be demonstrated through testing, e.g. | # test_clear_user: ASSERTION FAILED at lib/usercopy_kunit.c:221 | too few bytes consumed (offset=4095, size=2, ret=2) | [FAILED] 2 byte(s) As with __{arch,raw}_copy_to_user() there are also a number of potential other problems where the use of misaligned accesses could result in under-reporting the number of bytes zeroed. This patch fixes these issues by replacing the current implementation of __arch_clear_user() with one derived from the new __arch_copy_to_user(). This only uses aligned stores to write to user memory, and reliably reports the number of bytes zeroed. For correctness, I've tested this exhaustively for sizes 0 to 128 against every possible alignment relative to a leading and trailing page boundary. I've also boot tested and run a few kernel builds with the new implementation. For performenace, I've benchmarked reads form /dev/zero and timed kernel builds before and after this patch, and this seems to be at least as good (or marginally better than) the current implementation, though the difference is very close to noise. Signed-off-by: Mark Rutland Cc: Catalin Marinas Cc: Robin Murphy Cc: Will Deacon --- arch/arm64/lib/clear_user.S | 195 +++++++++++++++++++++++++++------- arch/arm64/lib/copy_to_user.S | 1 - 2 files changed, 157 insertions(+), 39 deletions(-) diff --git a/arch/arm64/lib/clear_user.S b/arch/arm64/lib/clear_user.S index a5a5f5b97b175..f422c05d84351 100644 --- a/arch/arm64/lib/clear_user.S +++ b/arch/arm64/lib/clear_user.S @@ -4,52 +4,171 @@ */ #include + #include +#include + +#define USER_OFF(off, x...) USER(fixup_offset_##off, x) - .text +#define FIXUP_OFFSET(n) \ +fixup_offset_##n: \ + sub x0, x3, x0; \ + sub x0, x0, n; \ + ret -/* Prototype: int __arch_clear_user(void *addr, size_t sz) - * Purpose : clear some user memory - * Params : addr - user memory address to clear - * : sz - number of bytes to clear - * Returns : number of bytes NOT cleared +/* + * Write zeroes to a user space buffer + * + * Parameters: + * x0 - to + * x1 - n + * Returns: + * x0 - bytes not copied * - * Alignment fixed up by hardware. + * Unlike a memset(to, 0, n), we need to handle faults on user addresses, and + * we need to precisely report the number of bytes (not) written. We must only + * use aligned single-copy-atomic stores to write to user memory, as stores + * which are not single-copy-atomic (e.g. unaligned stores, STP, ASMID stores) + * can be split into separate byte accesses (per ARM DDI 0487I.a, Section + * B2.2.1) and some arbitatrary set of those byte accesses might occur prior to + * a fault being raised (per per ARM DDI 0487I.a, Section B2.7.1). + * + * We use STTR to write to user memory, which has 1/2/4/8 byte forms, and the + * user address ('to') might have arbitrary alignment, so we must handle + * misalignment up to 8 bytes. */ - - .p2align 4 - // Alignment is for the loop, but since the prologue (including BTI) - // is also 16 bytes we can keep any padding outside the function SYM_FUNC_START(__arch_clear_user) - add x2, x0, x1 - subs x1, x1, #8 - b.mi 2f -1: -USER(9f, sttr xzr, [x0]) - add x0, x0, #8 - subs x1, x1, #8 - b.hi 1b -USER(9f, sttr xzr, [x2, #-8]) - mov x0, #0 - ret + /* + * The end address. Fixup handlers will use this to calculate + * the number of bytes cleared. + */ + add x3, x0, x1 -2: tbz x1, #2, 3f -USER(9f, sttr wzr, [x0]) -USER(8f, sttr wzr, [x2, #-4]) - mov x0, #0 - ret + /* + * Tracing of a kernel build indicates that for the vast + * majority of calls to copy_to_user(), 'to' is aligned to 8 + * bytes. When this is the case, we want to skip to the bulk + * copy as soon as possible. + */ + ands x4, x0, 0x7 + b.eq body -3: tbz x1, #1, 4f -USER(9f, sttrh wzr, [x0]) -4: tbz x1, #0, 5f -USER(7f, sttrb wzr, [x2, #-1]) -5: mov x0, #0 - ret + /* + * For small unaligned copies, it's not worth trying to be + * optimal. + */ + cmp x1, #8 + b.lo bytewise_loop - // Exception fixups -7: sub x0, x2, #5 // Adjust for faulting on the final byte... -8: add x0, x0, #4 // ...or the second word of the 4-7 byte case -9: sub x0, x2, x0 - ret + /* + * Calculate the distance to the next 8-byte boundary. + */ + mov x5, #8 + sub x4, x5, x4 + +SYM_INNER_LABEL(head_realign_1b, SYM_L_LOCAL) + tbz x4, #0, head_realign_2b + +USER_OFF(0, sttrb wzr, [x0]) + add x0, x0, #1 + +SYM_INNER_LABEL(head_realign_2b, SYM_L_LOCAL) + tbz x4, #1, head_realign_4b + +USER_OFF(0, sttrh wzr, [x0]) + add x0, x0, #2 + +SYM_INNER_LABEL(head_realign_4b, SYM_L_LOCAL) + tbz x4, #2, head_realigned + +USER_OFF(0, sttr wzr, [x0]) + add x0, x0, #4 + +SYM_INNER_LABEL(head_realigned, SYM_L_LOCAL) + /* + * Any 1-7 byte misalignment has now been fixed; subtract this + * misalignment from the remaining size. + */ + sub x1, x1, x4 + +SYM_INNER_LABEL(body, SYM_L_LOCAL) + cmp x1, #64 + b.lo tail_32b + +SYM_INNER_LABEL(body_64b_loop, SYM_L_LOCAL) +USER_OFF(0, sttr xzr, [x0, #0]) +USER_OFF(8, sttr xzr, [x0, #8]) +USER_OFF(16, sttr xzr, [x0, #16]) +USER_OFF(24, sttr xzr, [x0, #24]) +USER_OFF(32, sttr xzr, [x0, #32]) +USER_OFF(40, sttr xzr, [x0, #40]) +USER_OFF(48, sttr xzr, [x0, #48]) +USER_OFF(56, sttr xzr, [x0, #56]) + add x0, x0, #64 + sub x1, x1, #64 + + cmp x1, #64 + b.hs body_64b_loop + +SYM_INNER_LABEL(tail_32b, SYM_L_LOCAL) + tbz x1, #5, tail_16b + +USER_OFF(0, sttr xzr, [x0, #0]) +USER_OFF(8, sttr xzr, [x0, #8]) +USER_OFF(16, sttr xzr, [x0, #16]) +USER_OFF(24, sttr xzr, [x0, #24]) + add x0, x0, #32 + +SYM_INNER_LABEL(tail_16b, SYM_L_LOCAL) + tbz x1, #4, tail_8b + +USER_OFF(0, sttr xzr, [x0, #0]) +USER_OFF(8, sttr xzr, [x0, #8]) + add x0, x0, #16 + +SYM_INNER_LABEL(tail_8b, SYM_L_LOCAL) + tbz x1, #3, tail_4b + +USER_OFF(0, sttr xzr, [x0]) + add x0, x0, #8 + +SYM_INNER_LABEL(tail_4b, SYM_L_LOCAL) + tbz x1, #2, tail_2b + +USER_OFF(0, sttr wzr, [x0]) + add x0, x0, #4 + +SYM_INNER_LABEL(tail_2b, SYM_L_LOCAL) + tbz x1, #1, tail_1b + +USER_OFF(0, sttrh wzr, [x0]) + add x0, x0, #2 + +SYM_INNER_LABEL(tail_1b, SYM_L_LOCAL) + tbz x1, #0, done + +USER_OFF(0, sttrb wzr, [x0]) + +SYM_INNER_LABEL(done, SYM_L_LOCAL) + mov x0, xzr + ret + +SYM_INNER_LABEL(bytewise_loop, SYM_L_LOCAL) + cbz x1, done + +USER_OFF(0, sttrb wzr, [x0]) + add x0, x0, #1 + sub x1, x1, #1 + + b bytewise_loop + +FIXUP_OFFSET(0) +FIXUP_OFFSET(8) +FIXUP_OFFSET(16) +FIXUP_OFFSET(24) +FIXUP_OFFSET(32) +FIXUP_OFFSET(40) +FIXUP_OFFSET(48) +FIXUP_OFFSET(56) SYM_FUNC_END(__arch_clear_user) EXPORT_SYMBOL(__arch_clear_user) diff --git a/arch/arm64/lib/copy_to_user.S b/arch/arm64/lib/copy_to_user.S index fa603487e8571..9eab9ec6c71e9 100644 --- a/arch/arm64/lib/copy_to_user.S +++ b/arch/arm64/lib/copy_to_user.S @@ -7,7 +7,6 @@ #include #include -#include #define USER_OFF(off, x...) USER(fixup_offset_##off, x)