Linux-Media Archive on lore.kernel.org
 help / color / Atom feed
From: Kees Cook <keescook@chromium.org>
To: Catalin Marinas <catalin.marinas@arm.com>
Cc: enh <enh@google.com>, Evgenii Stepanov <eugenis@google.com>,
	Andrey Konovalov <andreyknvl@google.com>,
	Khalid Aziz <khalid.aziz@oracle.com>,
	Linux ARM <linux-arm-kernel@lists.infradead.org>,
	Linux Memory Management List <linux-mm@kvack.org>,
	LKML <linux-kernel@vger.kernel.org>,
	amd-gfx@lists.freedesktop.org, dri-devel@lists.freedesktop.org,
	linux-rdma@vger.kernel.org, linux-media@vger.kernel.org,
	kvm@vger.kernel.org,
	"open list:KERNEL SELFTEST FRAMEWORK" 
	<linux-kselftest@vger.kernel.org>,
	Vincenzo Frascino <vincenzo.frascino@arm.com>,
	Will Deacon <will.deacon@arm.com>,
	Mark Rutland <mark.rutland@arm.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Yishai Hadas <yishaih@mellanox.com>,
	Felix Kuehling <Felix.Kuehling@amd.com>,
	Alexander Deucher <Alexander.Deucher@amd.com>,
	Christian Koenig <Christian.Koenig@amd.com>,
	Mauro Carvalho Chehab <mchehab@kernel.org>,
	Jens Wiklander <jens.wiklander@linaro.org>,
	Alex Williamson <alex.williamson@redhat.com>,
	Leon Romanovsky <leon@kernel.org>,
	Dmitry Vyukov <dvyukov@google.com>,
	Kostya Serebryany <kcc@google.com>, Lee Smith <Lee.Smith@arm.com>,
	Ramana Radhakrishnan <Ramana.Radhakrishnan@arm.com>,
	Jacob Bramley <Jacob.Bramley@arm.com>,
	Ruben Ayrapetyan <Ruben.Ayrapetyan@arm.com>,
	Robin Murphy <robin.murphy@arm.com>,
	Luc Van Oostenryck <luc.vanoostenryck@gmail.com>,
	Dave Martin <Dave.Martin@arm.com>,
	Kevin Brodsky <kevin.brodsky@arm.com>,
	Szabolcs Nagy <Szabolcs.Nagy@arm.com>
Subject: Re: [PATCH v15 00/17] arm64: untag user pointers passed to the kernel
Date: Thu, 23 May 2019 09:38:19 -0700
Message-ID: <201905230917.DEE7A75EF0@keescook> (raw)
In-Reply-To: <20190523144449.waam2mkyzhjpqpur@mbp>

On Thu, May 23, 2019 at 03:44:49PM +0100, Catalin Marinas wrote:
> There is also the obvious requirement which I didn't mention: new user
> space continues to run on new/subsequent kernel versions. That's one of
> the points of contention for this series (ignoring MTE) with the
> maintainers having to guarantee this without much effort. IOW, do the
> 500K+ new lines in a subsequent kernel version break any user space out
> there? I'm only talking about the relaxed TBI ABI. Are the usual LTP,
> syskaller sufficient? Better static analysis would definitely help.

We can't have perfect coverage of people actively (or accidentally)
working to trick static analyzers (and the compiler) into "forgetting"
about a __user annotation. We can certainly improve analysis (I see
the other sub-thread on that), but I'd like that work not to block
this series.

What on this front would you be comfortable with? Given it's a new
feature isn't it sufficient to have a CONFIG (and/or boot option)?

> Or, if we ever want MTE to be turned on by default (i.e. tag checking),
> even if everything is tagged with 0, we have to disallow TBI for user
> and this includes hwasan. There were a small number of programs using
> the TBI (I think some JavaScript compilers tried this). But now we are
> bringing in the hwasan support and this can be a large user base. Shall
> we add an ELF note for such binaries that use TBI/hwasan?

Just to be clear, you say "disallow TBI for user" -- you mean a
particular process, yes? i.e. there is no architectural limitation that
says once we're using MTE nothing can switch to TBI. i.e. a process is
either doing MTE or TBI (or nothing, but that's the same as TBI).

> This needs solving as well. Most driver developers won't know why
> untagged_addr() is needed unless we have more rigorous types or type
> annotations and a tool to check them (we should probably revive the old
> sparse thread).

This seems like a parallel concern: we can do that separately from this
series. Without landing it, is it much harder for people to test it,
look for bugs, help with types/annotations, etc.

> > So there needs to be some way to let the kernel know which of three
> > things it should be doing:
> > 1- leaving userspace addresses as-is (present)
> > 2- wiping the top bits before using (this series)
> 
> (I'd say tolerating rather than wiping since get_user still uses the tag
> in the current series)
> 
> The current series does not allow any choice between 1 and 2, the
> default ABI basically becomes option 2.

What about testing tools that intentionally insert high bits for syscalls
and are _expecting_ them to fail? It seems the TBI series will break them?
In that case, do we need to opt into TBI as well?

> > 3- wiping the top bits for most things, but retaining them for MTE as
> >    needed (the future)
> 
> 2 and 3 are not entirely compatible as a tagged pointer may be checked
> against the memory colour by the hardware. So you can't have hwasan
> binary with MTE enabled.

Right: a process must be either MTE or TBI, not both.

> > I expect MTE to be the "default" in the future. Once a system's libc has
> > grown support for it, everything will be trying to use MTE. TBI will be
> > the special case (but TBI is effectively a prerequisite).
> 
> The kernel handling of tagged pointers is indeed a prerequisite. The ABI
> distinction between the above 2 and 3 needs to be solved.

Does that need solving now or when the MTE series appears? As there is
no reason to distinguish between "normal" and "TBI", that doesn't seem
to need solving at this point?

> > AFAICT, the only difference I see between 2 and 3 will be the tag handling
> > in usercopy (all other places will continue to ignore the top bits). Is
> > that accurate?
> 
> Yes, mostly (for the kernel). If MTE is enabled by default for a hwasan
> binary, it will SEGFAULT (either in user space or in kernel uaccess).
> How does the kernel choose between 2 and 3?

Right -- that was my question as well.

> > Is "1" a per-process state we want to keep? (I assume not, but rather it
> > is available via no TBI/MTE CONFIG or a boot-time option, if at all?)
> 
> Possibly, though not necessarily per process. For testing or if
> something goes wrong during boot, a command line option with a static
> label would do. The AT_FLAGS bit needs to be checked by user space. My
> preference would be per-process.

I would agree.

> > To choose between "2" and "3", it seems we need a per-process flag to
> > opt into TBI (and out of MTE).
> 
> Or leave option 2 the default and get it to opt in to MTE.

Given that MTE has to "start" at some point in the binary lifetime, I'm
fine with opting into MTE. I do expect, though, this will feel redundant
in a couple years as everything will immediately opt-in. But, okay, this
is therefore an issue for the MTE series.

> The current plan is that a future binary issues a prctl(), after
> checking the HWCAP_MTE bit (as I replied to Elliot, the MTE instructions
> are not in the current NOP space). I'd expect this to be done by the
> libc or dynamic loader under the assumption that the binaries it loads
> do _not_ use the top pointer byte for anything else. With hwasan
> compiled objects this gets more confusing (any ELF note to identify
> them?).

Okay, sounds fine.

> (there is also the risk of existing applications using TBI already but
> I'm not aware of any still using this feature other than hwasan)

Correct.


Alright, the tl;dr appears to be:
- you want more assurances that we can find __user stripping in the
  kernel more easily. (But this seems like a parallel problem.)
- we might need to opt in to TBI with a prctl()
- all other concerns are for the future MTE series (though it sounds
  like HWCAP_MTE and a prctl() solve those issues too).

Is this accurate? What do you see as the blockers for this series at
this point?

-- 
Kees Cook

  parent reply index

Thread overview: 99+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-06 16:30 Andrey Konovalov
2019-05-06 16:30 ` [PATCH v15 01/17] uaccess: add untagged_addr definition for other arches Andrey Konovalov
2019-05-29 14:49   ` Khalid Aziz
2019-05-06 16:30 ` [PATCH v15 02/17] arm64: untag user pointers in access_ok and __uaccess_mask_ptr Andrey Konovalov
2019-05-06 16:30 ` [PATCH v15 03/17] lib, arm64: untag user pointers in strn*_user Andrey Konovalov
2019-05-22 10:41   ` Catalin Marinas
2019-05-06 16:30 ` [PATCH v15 04/17] mm: add ksys_ wrappers to memory syscalls Andrey Konovalov
2019-05-22 10:56   ` Catalin Marinas
2019-05-06 16:30 ` [PATCH v15 05/17] arms64: untag user pointers passed " Andrey Konovalov
2019-05-22 11:49   ` Catalin Marinas
2019-05-22 21:16     ` Evgenii Stepanov
2019-05-23  9:04       ` Catalin Marinas
2019-05-24  4:23         ` Evgenii Stepanov
2019-05-24 15:41   ` Andrew Murray
2019-05-25  9:57   ` Catalin Marinas
2019-05-27  9:42   ` Catalin Marinas
2019-05-27 14:37   ` Catalin Marinas
2019-05-28 14:54     ` Andrew Murray
2019-05-28 15:40       ` Catalin Marinas
2019-05-28 15:56         ` Dave Martin
2019-05-28 16:34           ` Catalin Marinas
2019-05-29 12:42             ` Dave Martin
2019-05-29 13:23               ` Catalin Marinas
2019-05-29 15:18                 ` Dave Martin
2019-05-28 23:33         ` Khalid Aziz
2019-05-29 14:20           ` Catalin Marinas
2019-05-29 19:16             ` Khalid Aziz
2019-05-30 15:11               ` Catalin Marinas
2019-05-30 16:05                 ` Khalid Aziz
2019-05-30 16:57                   ` Catalin Marinas
2019-05-28 13:05   ` Catalin Marinas
2019-05-06 16:30 ` [PATCH v15 06/17] mm: untag user pointers in do_pages_move Andrey Konovalov
2019-05-22 11:51   ` Catalin Marinas
2019-05-06 16:30 ` [PATCH v15 07/17] mm, arm64: untag user pointers in mm/gup.c Andrey Konovalov
2019-05-22 11:56   ` Catalin Marinas
2019-05-06 16:30 ` [PATCH v15 08/17] mm, arm64: untag user pointers in get_vaddr_frames Andrey Konovalov
2019-05-06 16:30 ` [PATCH v15 09/17] fs, arm64: untag user pointers in copy_mount_options Andrey Konovalov
2019-05-22 12:09   ` Catalin Marinas
2019-05-06 16:30 ` [PATCH v15 10/17] fs, arm64: untag user pointers in fs/userfaultfd.c Andrey Konovalov
2019-05-06 16:30 ` [PATCH v15 11/17] drm/amdgpu, arm64: untag user pointers Andrey Konovalov
2019-05-07 16:43   ` Kuehling, Felix
2019-05-06 16:30 ` [PATCH v15 12/17] drm/radeon, arm64: untag user pointers in radeon_gem_userptr_ioctl Andrey Konovalov
2019-05-07 16:44   ` Kuehling, Felix
2019-05-06 16:30 ` [PATCH v15 13/17] IB, arm64: untag user pointers in ib_uverbs_(re)reg_mr() Andrey Konovalov
2019-05-06 19:50   ` Jason Gunthorpe
2019-05-07  6:33     ` Leon Romanovsky
2019-05-06 16:31 ` [PATCH v15 14/17] media/v4l2-core, arm64: untag user pointers in videobuf_dma_contig_user_get Andrey Konovalov
2019-05-24 13:13   ` Mauro Carvalho Chehab
2019-05-06 16:31 ` [PATCH v15 15/17] tee, arm64: untag user pointers in tee_shm_register Andrey Konovalov
2019-05-06 16:31 ` [PATCH v15 16/17] vfio/type1, arm64: untag user pointers in vaddr_get_pfn Andrey Konovalov
2019-05-06 16:31 ` [PATCH v15 17/17] selftests, arm64: add a selftest for passing tagged pointers to kernel Andrey Konovalov
2019-05-22 14:16   ` Catalin Marinas
2019-05-31 14:21     ` Andrey Konovalov
2019-05-31 16:22       ` Catalin Marinas
2019-05-17 14:49 ` [PATCH v15 00/17] arm64: untag user pointers passed to the kernel Catalin Marinas
2019-05-20 23:53   ` Evgenii Stepanov
2019-05-21 18:29     ` Catalin Marinas
2019-05-22  0:04       ` Kees Cook
2019-05-22 10:11         ` Catalin Marinas
2019-05-22 15:30           ` enh
2019-05-22 16:35             ` Catalin Marinas
2019-05-22 16:58               ` enh
2019-05-23 15:21                 ` Catalin Marinas
2019-05-22 20:47               ` Kees Cook
2019-05-22 23:03                 ` Evgenii Stepanov
2019-05-22 23:09                   ` enh
2019-05-23  7:34                     ` Catalin Marinas
2019-05-23 14:44                 ` Catalin Marinas
2019-05-23 15:44                   ` enh
2019-05-23 17:00                     ` Catalin Marinas
2019-05-23 16:38                   ` Kees Cook [this message]
2019-05-23 17:43                     ` Catalin Marinas
2019-05-23 21:31                       ` Kees Cook
2019-05-24 11:20                         ` Catalin Marinas
2019-05-28 17:02                         ` Catalin Marinas
2019-06-02  5:06                           ` Kees Cook
2019-05-22 19:21             ` Kees Cook
2019-05-22 20:15               ` enh
2019-05-23 15:08               ` Catalin Marinas
2019-05-23 17:51         ` Khalid Aziz
2019-05-23 20:11           ` Catalin Marinas
2019-05-23 21:42             ` Khalid Aziz
2019-05-23 21:49             ` Khalid Aziz
2019-05-24 10:11               ` Catalin Marinas
2019-05-24 14:25                 ` Khalid Aziz
2019-05-28 14:14                   ` Andrey Konovalov
2019-05-29  6:11                     ` Christoph Hellwig
2019-05-29 12:12                       ` Catalin Marinas
2019-05-30 17:15                     ` Catalin Marinas
2019-05-31 14:29                       ` Andrey Konovalov
2019-05-31 16:19                         ` Catalin Marinas
2019-05-31 16:24                           ` Andrey Konovalov
2019-05-31 16:46                             ` Catalin Marinas
2019-05-21 18:48   ` Jason Gunthorpe
2019-05-22 13:49     ` Dave Martin
2019-05-23  0:20       ` Jason Gunthorpe
2019-05-23 10:42         ` Dave Martin
2019-05-23 16:57           ` Catalin Marinas
2019-05-24 14:23             ` Dave Martin

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=201905230917.DEE7A75EF0@keescook \
    --to=keescook@chromium.org \
    --cc=Alexander.Deucher@amd.com \
    --cc=Christian.Koenig@amd.com \
    --cc=Dave.Martin@arm.com \
    --cc=Felix.Kuehling@amd.com \
    --cc=Jacob.Bramley@arm.com \
    --cc=Lee.Smith@arm.com \
    --cc=Ramana.Radhakrishnan@arm.com \
    --cc=Ruben.Ayrapetyan@arm.com \
    --cc=Szabolcs.Nagy@arm.com \
    --cc=akpm@linux-foundation.org \
    --cc=alex.williamson@redhat.com \
    --cc=amd-gfx@lists.freedesktop.org \
    --cc=andreyknvl@google.com \
    --cc=catalin.marinas@arm.com \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=dvyukov@google.com \
    --cc=enh@google.com \
    --cc=eugenis@google.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=jens.wiklander@linaro.org \
    --cc=kcc@google.com \
    --cc=kevin.brodsky@arm.com \
    --cc=khalid.aziz@oracle.com \
    --cc=kvm@vger.kernel.org \
    --cc=leon@kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=linux-media@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=linux-rdma@vger.kernel.org \
    --cc=luc.vanoostenryck@gmail.com \
    --cc=mark.rutland@arm.com \
    --cc=mchehab@kernel.org \
    --cc=robin.murphy@arm.com \
    --cc=vincenzo.frascino@arm.com \
    --cc=will.deacon@arm.com \
    --cc=yishaih@mellanox.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-Media Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-media/0 linux-media/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-media linux-media/ https://lore.kernel.org/linux-media \
		linux-media@vger.kernel.org linux-media@archiver.kernel.org
	public-inbox-index linux-media


Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-media


AGPL code for this site: git clone https://public-inbox.org/ public-inbox