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 X-Spam-Level: X-Spam-Status: No, score=-12.7 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_CR_TRAILER,INCLUDES_PATCH, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D6616C07E95 for ; Sun, 4 Jul 2021 15:40:08 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 6A86F613E1 for ; Sun, 4 Jul 2021 15:40:08 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 6A86F613E1 Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=gmail.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id C47BA6B005D; Sun, 4 Jul 2021 11:40:07 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id C1EDA6B006C; Sun, 4 Jul 2021 11:40:07 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id AC00F6B0071; Sun, 4 Jul 2021 11:40:07 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0110.hostedemail.com [216.40.44.110]) by kanga.kvack.org (Postfix) with ESMTP id 8B6466B005D for ; Sun, 4 Jul 2021 11:40:07 -0400 (EDT) Received: from smtpin08.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id 0EEE22030A for ; Sun, 4 Jul 2021 15:40:07 +0000 (UTC) X-FDA: 78325316454.08.1B3ADF1 Received: from mail-ed1-f49.google.com (mail-ed1-f49.google.com [209.85.208.49]) by imf10.hostedemail.com (Postfix) with ESMTP id BE5006002098 for ; Sun, 4 Jul 2021 15:40:06 +0000 (UTC) Received: by mail-ed1-f49.google.com with SMTP id s15so20297789edt.13 for ; Sun, 04 Jul 2021 08:40:06 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=aR0m64HeW1gtkHq8ZZxeKO2/pCX7wNNQkpb6dCYWvC8=; b=u606kjj0a6C9fMeiDMWZLB70p9UxGg8aCQlOcri6UE/6YXAoa/BSN5awecnpEI8sY6 CReq3PAKQqzNOctkIBDv9e49CmVIJA4a+L15S8Rak3GEOHm31NuWsvg+e6YTZREWRdDt fyuhgtZihch1663UDE20/NrtCTSaSIqFL3Ie17AHfqhqwiwwzBYGf/KIX6B/j1Ts0Zy5 3LsEZ7SytCXXqyQyrwxn5WnB9KtwojMSwlMYpsmsGk/vJmGxC0rFY6dZQJTJngo2tWCi tjaMSex/KvG2rhIc3mOTMSfujmxbdUeRRW0PtMq4ua7uyv1rmh8BrZRu4gUkrpOJ7UCx T2Eg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=aR0m64HeW1gtkHq8ZZxeKO2/pCX7wNNQkpb6dCYWvC8=; b=KfK8mnXXfirxBi4Ym8xuaRtcxS7tggQ/BXbzGfvPhYMRQCcgBjFkVJjXvKZ82HaH+L 5+9TCvgh9jWfqzJJQP5prTzDdVOsIv9YJkF2y/U7IKDBIESH5Cyr/k4Gc8EVjECZgJD4 8cgBACD3IcbPpiYh5R89zQLSFR1Jg589CdwL6Kvfw80xVaqEXggjuAA9PLP8vWGHa9xA mt1qDZ1YIOGaMKVs+X2RR1AaKA16eEaXkRnCwMDjRBGh+buR/pnUlEQcz/Bk+waMUp+2 zHNKYA9LKvhp9D62bRhCjFnJLtok8YAWMZpMPoqfefA8oYwORdf06xel2vuNeiZbvpr3 owtA== X-Gm-Message-State: AOAM531I5JQLTsBwKh+U2TBG58m1riGyFI4JJl3MnYIzLkG7EPpN5VPN iWEjnV5JoIzKnjD7wA9KZe7TgPeKLv8g1nivUPc= X-Google-Smtp-Source: ABdhPJzTAjYZcBhcQWaT/Woz2xjU8jdKDKOTHfH76vNOgbXwnObuf8XjWcZnIDBwxWtsl56SWa+y/5sSeur1MyQiPbk= X-Received: by 2002:a05:6402:216:: with SMTP id t22mr10942233edv.70.1625413205386; Sun, 04 Jul 2021 08:40:05 -0700 (PDT) MIME-Version: 1.0 References: <20210702225705.2477947-1-pcc@google.com> <20210702225705.2477947-2-pcc@google.com> In-Reply-To: <20210702225705.2477947-2-pcc@google.com> From: Andrey Konovalov Date: Sun, 4 Jul 2021 17:39:54 +0200 Message-ID: Subject: Re: [PATCH v3 1/2] userfaultfd: do not untag user pointers To: Peter Collingbourne Cc: Catalin Marinas , Vincenzo Frascino , Dave Martin , Will Deacon , Andrew Morton , Andrea Arcangeli , Alistair Delva , Lokesh Gidra , William McVicker , Evgenii Stepanov , Mitch Phillips , Linux ARM , Linux Memory Management List , stable@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Authentication-Results: imf10.hostedemail.com; dkim=pass header.d=gmail.com header.s=20161025 header.b=u606kjj0; spf=pass (imf10.hostedemail.com: domain of andreyknvl@gmail.com designates 209.85.208.49 as permitted sender) smtp.mailfrom=andreyknvl@gmail.com; dmarc=pass (policy=none) header.from=gmail.com X-Stat-Signature: zgm8rgapr6tmm9q6cpx5aky9yjnycq71 X-Rspamd-Queue-Id: BE5006002098 X-Rspamd-Server: rspam06 X-HE-Tag: 1625413206-273077 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 Sat, Jul 3, 2021 at 12:57 AM Peter Collingbourne wrote: > > If a user program uses userfaultfd on ranges of heap memory, it may > end up passing a tagged pointer to the kernel in the range.start > field of the UFFDIO_REGISTER ioctl. This can happen when using an > MTE-capable allocator, or on Android if using the Tagged Pointers > feature for MTE readiness [1]. > > When a fault subsequently occurs, the tag is stripped from the fault > address returned to the application in the fault.address field > of struct uffd_msg. However, from the application's perspective, > the tagged address *is* the memory address, so if the application > is unaware of memory tags, it may get confused by receiving an > address that is, from its point of view, outside of the bounds of the > allocation. We observed this behavior in the kselftest for userfaultfd > [2] but other applications could have the same problem. > > Address this by not untagging pointers passed to the userfaultfd > ioctls. Instead, let the system call fail. This will provide an > early indication of problems with tag-unaware userspace code instead > of letting the code get confused later, and is consistent with how > we decided to handle brk/mmap/mremap in commit dcde237319e6 ("mm: > Avoid creating virtual address aliases in brk()/mmap()/mremap()"), > as well as being consistent with the existing tagged address ABI > documentation relating to how ioctl arguments are handled. > > The code change is a revert of commit 7d0325749a6c ("userfaultfd: > untag user pointers"). > > [1] https://source.android.com/devices/tech/debug/tagged-pointers > [2] tools/testing/selftests/vm/userfaultfd.c > > Signed-off-by: Peter Collingbourne > Link: https://linux-review.googlesource.com/id/I761aa9f0344454c482b83fcfcce547db0a25501b > Fixes: 63f0c6037965 ("arm64: Introduce prctl() options to control the tagged user addresses ABI") > Cc: # 5.4 > --- > Documentation/arm64/tagged-address-abi.rst | 25 +++++++++++++++------- > fs/userfaultfd.c | 22 +++++++++---------- > 2 files changed, 27 insertions(+), 20 deletions(-) > > diff --git a/Documentation/arm64/tagged-address-abi.rst b/Documentation/arm64/tagged-address-abi.rst > index 459e6b66ff68..737f9d8565a2 100644 > --- a/Documentation/arm64/tagged-address-abi.rst > +++ b/Documentation/arm64/tagged-address-abi.rst > @@ -45,14 +45,23 @@ how the user addresses are used by the kernel: > > 1. User addresses not accessed by the kernel but used for address space > management (e.g. ``mprotect()``, ``madvise()``). The use of valid > - tagged pointers in this context is allowed with the exception of > - ``brk()``, ``mmap()`` and the ``new_address`` argument to > - ``mremap()`` as these have the potential to alias with existing > - user addresses. > - > - NOTE: This behaviour changed in v5.6 and so some earlier kernels may > - incorrectly accept valid tagged pointers for the ``brk()``, > - ``mmap()`` and ``mremap()`` system calls. > + tagged pointers in this context is allowed with these exceptions: > + > + - ``brk()``, ``mmap()`` and the ``new_address`` argument to > + ``mremap()`` as these have the potential to alias with existing > + user addresses. > + > + NOTE: This behaviour changed in v5.6 and so some earlier kernels may > + incorrectly accept valid tagged pointers for the ``brk()``, > + ``mmap()`` and ``mremap()`` system calls. > + > + - The ``range.start`` argument to the ``UFFDIO_REGISTER`` ``ioctl()`` > + used on a file descriptor obtained from ``userfaultfd()``, as > + fault addresses subsequently obtained by reading the file descriptor > + will be untagged, which may otherwise confuse tag-unaware programs. > + > + NOTE: This behaviour changed in v5.14 and so some earlier kernels may > + incorrectly accept valid tagged pointers for this system call. > > 2. User addresses accessed by the kernel (e.g. ``write()``). This ABI > relaxation is disabled by default and the application thread needs to > diff --git a/fs/userfaultfd.c b/fs/userfaultfd.c > index dd7a6c62b56f..7613efe002c1 100644 > --- a/fs/userfaultfd.c > +++ b/fs/userfaultfd.c > @@ -1236,23 +1236,21 @@ static __always_inline void wake_userfault(struct userfaultfd_ctx *ctx, > } > > static __always_inline int validate_range(struct mm_struct *mm, > - __u64 *start, __u64 len) > + __u64 start, __u64 len) > { > __u64 task_size = mm->task_size; > > - *start = untagged_addr(*start); > - > - if (*start & ~PAGE_MASK) > + if (start & ~PAGE_MASK) > return -EINVAL; > if (len & ~PAGE_MASK) > return -EINVAL; > if (!len) > return -EINVAL; > - if (*start < mmap_min_addr) > + if (start < mmap_min_addr) > return -EINVAL; > - if (*start >= task_size) > + if (start >= task_size) > return -EINVAL; > - if (len > task_size - *start) > + if (len > task_size - start) > return -EINVAL; > return 0; > } > @@ -1313,7 +1311,7 @@ static int userfaultfd_register(struct userfaultfd_ctx *ctx, > vm_flags |= VM_UFFD_MINOR; > } > > - ret = validate_range(mm, &uffdio_register.range.start, > + ret = validate_range(mm, uffdio_register.range.start, > uffdio_register.range.len); > if (ret) > goto out; > @@ -1519,7 +1517,7 @@ static int userfaultfd_unregister(struct userfaultfd_ctx *ctx, > if (copy_from_user(&uffdio_unregister, buf, sizeof(uffdio_unregister))) > goto out; > > - ret = validate_range(mm, &uffdio_unregister.start, > + ret = validate_range(mm, uffdio_unregister.start, > uffdio_unregister.len); > if (ret) > goto out; > @@ -1668,7 +1666,7 @@ static int userfaultfd_wake(struct userfaultfd_ctx *ctx, > if (copy_from_user(&uffdio_wake, buf, sizeof(uffdio_wake))) > goto out; > > - ret = validate_range(ctx->mm, &uffdio_wake.start, uffdio_wake.len); > + ret = validate_range(ctx->mm, uffdio_wake.start, uffdio_wake.len); > if (ret) > goto out; > > @@ -1708,7 +1706,7 @@ static int userfaultfd_copy(struct userfaultfd_ctx *ctx, > sizeof(uffdio_copy)-sizeof(__s64))) > goto out; > > - ret = validate_range(ctx->mm, &uffdio_copy.dst, uffdio_copy.len); > + ret = validate_range(ctx->mm, uffdio_copy.dst, uffdio_copy.len); > if (ret) > goto out; > /* > @@ -1765,7 +1763,7 @@ static int userfaultfd_zeropage(struct userfaultfd_ctx *ctx, > sizeof(uffdio_zeropage)-sizeof(__s64))) > goto out; > > - ret = validate_range(ctx->mm, &uffdio_zeropage.range.start, > + ret = validate_range(ctx->mm, uffdio_zeropage.range.start, > uffdio_zeropage.range.len); > if (ret) > goto out; > -- > 2.32.0.93.g670b81a890-goog > Reviewed-by: Andrey Konovalov