Linux-ARM-Kernel Archive on lore.kernel.org
 help / color / Atom feed
From: Kevin Brodsky <kevin.brodsky@arm.com>
To: Vincenzo Frascino <vincenzo.frascino@arm.com>,
	linux-arm-kernel@lists.infradead.org, linux-doc@vger.kernel.org,
	linux-mm@kvack.org, linux-arch@vger.kernel.org,
	linux-kselftest@vger.kernel.org, linux-kernel@vger.kernel.org
Cc: Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will.deacon@arm.com>,
	Alexander Viro <viro@zeniv.linux.org.uk>,
	Andrey Konovalov <andreyknvl@google.com>
Subject: Re: [PATCH v5 1/2] arm64: Define Documentation/arm64/tagged-address-abi.txt
Date: Tue, 18 Jun 2019 14:13:01 +0100
Message-ID: <1c55a610-9aa5-4675-f7de-79a1661a660d@arm.com> (raw)
In-Reply-To: <20190613155137.47675-2-vincenzo.frascino@arm.com>

I'm happy with the ABI overall, but I think we need a few more tweaks.

On 13/06/2019 16:51, Vincenzo Frascino wrote:
> On arm64 the TCR_EL1.TBI0 bit has been always enabled hence
> the userspace (EL0) is allowed to set a non-zero value in the
> top byte but the resulting pointers are not allowed at the
> user-kernel syscall ABI boundary.
>
> With the relaxed ABI proposed through this document, it is now possible
> to pass tagged pointers to the syscalls, when these pointers are in
> memory ranges obtained by an anonymous (MAP_ANONYMOUS) mmap().
>
> This change in the ABI requires a mechanism to requires the userspace
> to opt-in to such an option.
>
> Specify and document the way in which sysctl and prctl() can be used
> in combination to allow the userspace to opt-in this feature.
>
> Cc: Catalin Marinas <catalin.marinas@arm.com>
> Cc: Will Deacon <will.deacon@arm.com>
> CC: Andrey Konovalov <andreyknvl@google.com>
> Signed-off-by: Vincenzo Frascino <vincenzo.frascino@arm.com>
> ---
>   Documentation/arm64/tagged-address-abi.txt | 134 +++++++++++++++++++++
>   1 file changed, 134 insertions(+)
>   create mode 100644 Documentation/arm64/tagged-address-abi.txt
>
> diff --git a/Documentation/arm64/tagged-address-abi.txt b/Documentation/arm64/tagged-address-abi.txt
> new file mode 100644
> index 000000000000..0ae900d4bb2d
> --- /dev/null
> +++ b/Documentation/arm64/tagged-address-abi.txt
> @@ -0,0 +1,134 @@
> +ARM64 TAGGED ADDRESS ABI
> +========================
> +
> +This document describes the usage and semantics of the Tagged Address
> +ABI on arm64.
> +
> +1. Introduction
> +---------------
> +
> +On arm64 the TCR_EL1.TBI0 bit has been always enabled on the kernel, hence
"been always" -> "always been"

> +the userspace (EL0) is entitled to perform a user memory access through a
> +64-bit pointer with a non-zero top byte but the resulting pointers are not
> +allowed at the user-kernel syscall ABI boundary.
> +
> +This document describes a relaxation of the ABI that makes it possible to
> +to pass tagged pointers to the syscalls, when these pointers are in memory
> +ranges obtained as described in section 2.
> +
> +Since it is not desirable to relax the ABI to allow tagged user addresses
> +into the kernel indiscriminately, arm64 provides a new sysctl interface
> +(/proc/sys/abi/tagged_addr) that is used to prevent the applications from
> +enabling the relaxed ABI and a new prctl() interface that can be used to
> +enable or disable the relaxed ABI.
> +A detailed description of the newly introduced mechanisms will be provided
> +in section 2.
> +
> +2. ARM64 Tagged Address ABI
> +---------------------------
> +
> +From the kernel syscall interface perspective, we define, for the purposes
> +of this document, a "valid tagged pointer" as a pointer that either has a
> +zero value set in the top byte or has a non-zero value, it is in memory
> +ranges privately owned by a userspace process and it is obtained in one of

Remove all the remaining "it": "a pointer that either [...], is in memory ranges 
[...] and is obtained..."

> +the following ways:
> +  - mmap() done by the process itself, where either:
> +    * flags have MAP_PRIVATE and MAP_ANONYMOUS
> +    * flags have MAP_PRIVATE and the file descriptor refers to a regular
> +      file or "/dev/zero"
> +  - brk() system call done by the process itself (i.e. the heap area between
> +    the initial location of the program break at process creation and its
> +    current location).
> +  - any memory mapped by the kernel in the process's address space during
> +    creation and following the restrictions presented above (i.e. data, bss,
> +    stack).

As I commented on v2, the "i.e." is not correct: these 3 sections are not the only 
ones that are covered by this ABI (.text also is, for instance). Replacing "i.e." 
with "e.g." would work.

Also, since the rules above say explicitly "done by the process itself", it might be 
clearer to replace "following the restrictions presented above" with "with the same 
restrictions as for mmap()".

> +
> +The ARM64 Tagged Address ABI is an opt-in feature, and an application can
> +control it using the following:
> + - /proc/sys/abi/tagged_addr: a new sysctl interface that can be used to
> +        prevent the applications from enabling the relaxed ABI.
> +        The sysctl is meant also for testing purposes in order to provide a
> +        simple way for the userspace to verify the return error checking of
> +        the prctl() commands without having to reconfigure the kernel.
> +        The sysctl supports the following configuration options:
> +         - 0: Disable ARM64 Tagged Address ABI for all the applications.
> +         - 1 (Default): Enable ARM64 Tagged Address ABI for all the
> +                        applications.

I find this very confusing, because it suggests that the default value of 
PR_GET_TAGGED_ADDR_CTRL for new processes will be set to the value of this sysctl, 
when in fact this sysctl is about restricting the *availability* of the new ABI. 
Instead of disabling the ABI, I would talk about disabling access to the new ABI here.

> +        If the ARM64 Tagged Address ABI is disabled at a certain point in
> +        time, all the applications that were using tagging before this event
> +        occurs, will continue to use tagging.
> +
> + - prctl()s:
> +  - PR_SET_TAGGED_ADDR_CTRL: can be used to enable or disable the Tagged
> +        Address ABI.
> +        The (unsigned int) arg2 argument is a bit mask describing the
> +        control mode used:
> +          - PR_TAGGED_ADDR_ENABLE: Enable ARM64 Tagged Address ABI.
> +        The arguments arg3, arg4, and arg5 are ignored.

Have we definitely decided that arg{3,4,5} are ignored? Catalin?

> +
> +  - PR_GET_TAGGED_ADDR_CTRL: can be used to check the status of the Tagged
> +        Address ABI.
> +        The arguments arg2, arg3, arg4, and arg5 are ignored.
> +
> +The ABI properties set by the mechanisms described above are inherited by threads
> +of the same application and fork()'ed children but cleared by execve().
> +
> +As a consequence of invoking PR_SET_TAGGED_ADDR_CTRL prctl() by an applications,

I think this is too vague, you can use this prctl() to disable the new ABI, and it 
can also fail. Maybe it's best to simply say that the process has successfully opted 
into the new ABI.

> +the ABI guarantees the following behaviours:
> +
> +  - Every current or newly introduced syscall can accept any valid tagged
> +    pointers.
"pointer". Also, is it really useful to talk about newly introduced syscall? New from 
which point of view?

> +
> +  - If a non valid tagged pointer is passed to a syscall then the behaviour
> +    is undefined.
> +
> +  - Every valid tagged pointer is expected to work as an untagged one.
> +
> +  - The kernel preserves any valid tagged pointers and returns them to the
"pointer", "returns it"

> +    userspace unchanged (i.e. on syscall return) in all the cases except the
> +    ones documented in the "Preserving tags" section of tagged-pointers.txt.
> +
> +A definition of the meaning of tagged pointers on arm64 can be found in:
> +Documentation/arm64/tagged-pointers.txt.
> +
> +3. ARM64 Tagged Address ABI Exceptions
> +--------------------------------------
> +
> +The behaviours described in section 2, with particular reference to the
> +acceptance by the syscalls of any valid tagged pointer are not applicable
> +to the following cases:
> +  - mmap() addr parameter.
> +  - mremap() new_address parameter.
> +  - prctl_set_mm() struct prctl_map fields.
> +  - prctl_set_mm_map() struct prctl_map fields.

prctl_set_mm() and prctl_set_mm_map() are internal kernel functions, not syscall 
names. IIUC, we don't want to allow any address field settable via the PR_SET_MM 
prctl() to be tagged. Catalin, is that correct? I think this needs rephrasing.

Kevin

> +
> +Any attempt to use non-zero tagged pointers will lead to undefined behaviour.
> +
> +4. Example of correct usage
> +---------------------------
> +
> +void main(void)
> +{
> +	static int tbi_enabled = 0;
> +	unsigned long tag = 0;
> +
> +	char *ptr = mmap(NULL, PAGE_SIZE, PROT_READ | PROT_WRITE,
> +			 MAP_ANONYMOUS, -1, 0);
> +
> +	if (prctl(PR_SET_TAGGED_ADDR_CTRL, PR_TAGGED_ADDR_ENABLE,
> +		  0, 0, 0) == 0)
> +		tbi_enabled = 1;
> +
> +	if (ptr == (void *)-1) /* MAP_FAILED */
> +		return -1;
> +
> +	if (tbi_enabled)
> +		tag = rand() & 0xff;
> +
> +	ptr = (char *)((unsigned long)ptr | (tag << TAG_SHIFT));
> +
> +	*ptr = 'a';
> +
> +	...
> +}
> +


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

  parent reply index

Thread overview: 78+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-06-12 11:43 [PATCH v17 00/15] arm64: untag user pointers passed to the kernel Andrey Konovalov
2019-06-12 11:43 ` [PATCH v17 01/15] arm64: untag user pointers in access_ok and __uaccess_mask_ptr Andrey Konovalov
2019-06-12 14:26   ` Vincenzo Frascino
2019-06-12 11:43 ` [PATCH v17 02/15] lib, arm64: untag user pointers in strn*_user Andrey Konovalov
2019-06-12 14:28   ` Vincenzo Frascino
2019-06-12 11:43 ` [PATCH v17 03/15] arm64: Introduce prctl() options to control the tagged user addresses ABI Andrey Konovalov
2019-06-12 14:30   ` Vincenzo Frascino
2019-06-12 14:55   ` Catalin Marinas
2019-06-13 11:02   ` Dave Martin
2019-06-13 15:26     ` Catalin Marinas
2019-06-14  5:13       ` Kees Cook
2019-06-18  9:18         ` Dave Martin
2019-06-13 11:16   ` Dave Martin
2019-06-13 15:35     ` Catalin Marinas
2019-06-13 15:45       ` Vincenzo Frascino
2019-06-13 15:57         ` Catalin Marinas
2019-06-13 16:15           ` Vincenzo Frascino
2019-06-17 13:56   ` Catalin Marinas
2019-06-17 16:56     ` Szabolcs Nagy
2019-06-17 16:57     ` Evgenii Stepanov
2019-06-17 17:18       ` Catalin Marinas
2019-06-17 21:59         ` Evgenii Stepanov
2019-06-19 14:45   ` Andrey Konovalov
2019-06-19 15:29     ` Catalin Marinas
2019-06-12 11:43 ` [PATCH v17 04/15] mm, arm64: untag user pointers passed to memory syscalls Andrey Konovalov
2019-06-12 14:31   ` Vincenzo Frascino
2019-06-19 15:55   ` Khalid Aziz
2019-06-19 16:46     ` Khalid Aziz
2019-06-24 14:22       ` Andrey Konovalov
2019-06-12 11:43 ` [PATCH v17 05/15] mm, arm64: untag user pointers in mm/gup.c Andrey Konovalov
2019-06-12 14:33   ` Vincenzo Frascino
2019-06-19 16:41   ` Khalid Aziz
2019-06-12 11:43 ` [PATCH v17 06/15] mm, arm64: untag user pointers in get_vaddr_frames Andrey Konovalov
2019-06-12 14:34   ` Vincenzo Frascino
2019-06-19 16:48   ` Khalid Aziz
2019-06-12 11:43 ` [PATCH v17 07/15] fs, arm64: untag user pointers in copy_mount_options Andrey Konovalov
2019-06-12 14:35   ` Vincenzo Frascino
2019-06-19 20:01   ` Khalid Aziz
2019-06-12 11:43 ` [PATCH v17 08/15] userfaultfd, arm64: untag user pointers Andrey Konovalov
2019-06-12 14:40   ` Vincenzo Frascino
2019-06-12 11:43 ` [PATCH v17 09/15] drm/amdgpu, " Andrey Konovalov
2019-06-12 11:43 ` [PATCH v17 10/15] drm/radeon, arm64: untag user pointers in radeon_gem_userptr_ioctl Andrey Konovalov
2019-06-12 11:43 ` [PATCH v17 11/15] IB/mlx4, arm64: untag user pointers in mlx4_get_umem_mr Andrey Konovalov
2019-06-12 11:43 ` [PATCH v17 12/15] media/v4l2-core, arm64: untag user pointers in videobuf_dma_contig_user_get Andrey Konovalov
2019-06-19 20:05   ` Khalid Aziz
2019-06-12 11:43 ` [PATCH v17 13/15] tee/shm, arm64: untag user pointers in tee_shm_register Andrey Konovalov
2019-06-12 11:43 ` [PATCH v17 14/15] vfio/type1, arm64: untag user pointers in vaddr_get_pfn Andrey Konovalov
2019-06-12 14:41   ` Vincenzo Frascino
2019-06-12 15:58   ` Auger Eric
2019-06-12 11:43 ` [PATCH v17 15/15] selftests, arm64: add a selftest for passing tagged pointers to kernel Andrey Konovalov
2019-06-12 12:30   ` Szabolcs Nagy
2019-06-12 15:00     ` Catalin Marinas
2019-06-19 14:42       ` Andrey Konovalov
2019-06-12 14:21 ` [PATCH v4 0/2] arm64 relaxed ABI Vincenzo Frascino
2019-06-12 14:21   ` [PATCH v4 1/2] arm64: Define Documentation/arm64/tagged-address-abi.txt Vincenzo Frascino
2019-06-12 15:35     ` Catalin Marinas
2019-06-13 10:15       ` Vincenzo Frascino
2019-06-13 11:37         ` Dave Martin
2019-06-13 12:28           ` Catalin Marinas
2019-06-13 13:23             ` Dave Martin
2019-06-13 15:39               ` Catalin Marinas
2019-06-12 16:30     ` Szabolcs Nagy
2019-06-13  9:20       ` Catalin Marinas
2019-06-13 10:14         ` Szabolcs Nagy
2019-06-13 11:16           ` Vincenzo Frascino
2019-06-13 12:28             ` Szabolcs Nagy
2019-06-13 14:03               ` Vincenzo Frascino
2019-06-13 15:32                 ` Szabolcs Nagy
2019-06-13 15:35                   ` Vincenzo Frascino
2019-06-12 14:21   ` [PATCH v4 2/2] arm64: Relax Documentation/arm64/tagged-pointers.txt Vincenzo Frascino
2019-06-12 15:56     ` Catalin Marinas
2019-06-12 16:37     ` Szabolcs Nagy
2019-06-13 15:51 ` [PATCH v5 0/2] arm64 relaxed ABI Vincenzo Frascino
2019-06-13 15:51   ` [PATCH v5 1/2] arm64: Define Documentation/arm64/tagged-address-abi.txt Vincenzo Frascino
2019-06-18 11:02     ` Szabolcs Nagy
2019-06-18 13:13     ` Kevin Brodsky [this message]
2019-06-21 15:16       ` Catalin Marinas
2019-06-13 15:51   ` [PATCH v5 2/2] arm64: Relax Documentation/arm64/tagged-pointers.txt Vincenzo Frascino

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=1c55a610-9aa5-4675-f7de-79a1661a660d@arm.com \
    --to=kevin.brodsky@arm.com \
    --cc=andreyknvl@google.com \
    --cc=catalin.marinas@arm.com \
    --cc=linux-arch@vger.kernel.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-doc@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-kselftest@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=vincenzo.frascino@arm.com \
    --cc=viro@zeniv.linux.org.uk \
    --cc=will.deacon@arm.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-ARM-Kernel Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/0 linux-arm-kernel/git/0.git
	git clone --mirror https://lore.kernel.org/linux-arm-kernel/1 linux-arm-kernel/git/1.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-arm-kernel linux-arm-kernel/ https://lore.kernel.org/linux-arm-kernel \
		linux-arm-kernel@lists.infradead.org
	public-inbox-index linux-arm-kernel

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.infradead.lists.linux-arm-kernel


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