Re: [PATCH v6 1/2] arm64: Define Documentation/arm64/tagged-address-abi.rst

[Kevin Brodsky <kevin.brodsky@arm.com>]

On 30/07/2019 15:24, Vincenzo Frascino wrote:
> Hi Kevin,
>
> On 7/30/19 2:57 PM, Kevin Brodsky wrote:
>> On 30/07/2019 14:25, Vincenzo Frascino wrote:
>>> Hi Kevin,
>>>
>>> On 7/30/19 11:32 AM, Kevin Brodsky wrote:
>>>> Some more comments. Mostly minor wording issues, except the prctl() exclusion at
>>>> the end.
>>>>
>>>> On 25/07/2019 14:50, 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>
>>>>> Acked-by: Szabolcs Nagy <szabolcs.nagy@arm.com>
>>>>> ---
>>>>>     Documentation/arm64/tagged-address-abi.rst | 148 +++++++++++++++++++++
>>>>>     1 file changed, 148 insertions(+)
>>>>>     create mode 100644 Documentation/arm64/tagged-address-abi.rst
>>>>>
>>>>> diff --git a/Documentation/arm64/tagged-address-abi.rst
>>>>> b/Documentation/arm64/tagged-address-abi.rst
>>>>> new file mode 100644
>>>>> index 000000000000..a8ecb991de82
>>>>> --- /dev/null
>>>>> +++ b/Documentation/arm64/tagged-address-abi.rst
>>>>> @@ -0,0 +1,148 @@
>>>>> +========================
>>>>> +ARM64 TAGGED ADDRESS ABI
>>>>> +========================
>>>>> +
>>>>> +Author: Vincenzo Frascino <vincenzo.frascino@arm.com>
>>>>> +
>>>>> +Date: 25 July 2019
>>>>> +
>>>>> +This document describes the usage and semantics of the Tagged Address
>>>>> +ABI on arm64.
>>>>> +
>>>>> +1. Introduction
>>>>> +---------------
>>>>> +
>>>>> +On arm64 the TCR_EL1.TBI0 bit has always been enabled on the kernel, hence
>>>>> +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
>>>> One too many "to" (at the end the previous line).
>>>>
>>> Yep will fix in v7.
>>>
>>>>> +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, is in memory ranges
>>>>> +privately owned by a userspace process and is obtained in one of 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 with the same restrictions as for mmap() (e.g. data, bss,
>>>>> +  stack).
>>>>> +
>>>>> +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 access to the relaxed ABI.
>>>>> +  The sysctl supports the following configuration options:
>>>>> +
>>>>> +  - **0**: Disable the access to the ARM64 Tagged Address ABI for all
>>>>> +    the applications.
>>>>> +  - **1** (Default): Enable the access to the ARM64 Tagged Address ABI for
>>>>> +    all the applications.
>>>>> +
>>>>> +   If the access to 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.
>>>> "tagging" may be misinterpreted here. I would be more explicit by saying that
>>>> the tagged address ABI remains enabled in processes that opted in before the
>>>> access got disabled.
>>>>
>>> Assuming that ARM64 Tagged Address ABI gives access to "tagging" and since it is
>>> what this document is talking about, I do not see how it can be misinterpreted ;)
>> "tagging" is a confusing term ("using tagging" even more so), it could be
>> interpreted as memory tagging (especially in the presence of MTE). This document
>> does not use "tagging" anywhere else, which is good. Let's stick to the same
>> name for the ABI throughout the document, repetition is less problematic than
>> vague wording.
>>
> This document does not cover MTE, it covers the "ARM64 Tagged Address ABI" hence
> "tagging" has a precise semantical meaning in this context. Still I do not see
> how it can be confused.
>
>>>>> +- **prctl()s**:
>>>>> +
>>>>> +  - **PR_SET_TAGGED_ADDR_CTRL**: Invoked by a process, can be used to
>>>>> enable or
>>>>> +    disable its access to the ARM64 Tagged Address ABI.
>>>> I still find the wording confusing, because "access to the ABI" is not used
>>>> consistently. The "tagged_addr" sysctl enables *access to the ABI*, that's fine.
>>>> However, PR_SET_TAGGED_ADDR_CTRL enables *the ABI itself* (which is only
>>>> possible if access to the ABI is enabled).
>>>>
>>> As it stands, it enables or disables the ABI itself when used with
>>> PR_TAGGED_ADDR_ENABLE, or can enable other things in future. IMHO the only thing
>>> that these features have in common is the access to the ABI which is granted by
>>> this prctl().
>> I see your point, you could have other bits controlling other aspects. However,
>> I would really avoid saying that this prctl is used to enable or disable access
>> to the new ABI, because it isn't (either you have access to the new ABI and this
>> prctl can be used, or you don't and this prctl will fail).
>>
> What is the system wide evidence that the access to the ABI is denied? Or what
> is the system wide evidence that it is granted?
>
> In other words, is it enough for a process to have the sysctl set (system wide)
> to know that the the ABI is enabled and have granted access to it? or does it
> need to do something else?

I think we really have a wording problem here, which is why this part of the document 
and this discussion is confusing.

tagged_addr=1 (system-wide) allows processes to enable the tagged address ABI by 
calling prctl(PR_SET_TAGGED_ADDR_CTRL). It does not alter the state of any running 
process, and does not enable the ABI by default for new processes either. Conversely, 
when tagged_addr=0, that prctl() is always denied.

The current description of the sysctl and prctl does not make that clear. I think 
that it would be much more obvious by reorganising that section as such:
- prctl() first, the current wording is fine.
- sysctl() second, described *only* in terms of the prctl() (denying 
PR_SET_TAGGED_ADDR_CTRL or not), and nothing else, to avoid wording issues.

It's certainly not the only way to do it, but that would be much clearer to me :)

Kevin

>>>>> +
>>>>> +    The (unsigned int) arg2 argument is a bit mask describing the control mode
>>>>> +    used:
>>>>> +
>>>>> +    - **PR_TAGGED_ADDR_ENABLE**: Enable ARM64 Tagged Address ABI.
>>>>> +
>>>>> +    The prctl(PR_SET_TAGGED_ADDR_CTRL, ...) will return -EINVAL if the ARM64
>>>>> +    Tagged Address ABI is not available.
>>>> For clarity, it would be good to mention that one possible reason for the ABI
>>>> not to be available is tagged_addr == 0.
>>>>
>>> The logical implication is already quite clear tagged_addr == 0 (Disabled) =>
>>> Tagged Address ABI not available => return -EINVAL. I do not see the need to
>>> repeat the concept twice.
>>>
>>>>> +
>>>>> +    The arguments arg3, arg4, and arg5 are ignored.
>>>>> +  - **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().
>>>>> +
>>>>> +When a process has successfully opted into the new ABI by invoking
>>>>> +PR_SET_TAGGED_ADDR_CTRL prctl(), this guarantees the following behaviours:
>>>>> +
>>>>> + - Every currently available syscall, except the cases mentioned in section
>>>>> 3, can
>>>>> +   accept any valid tagged pointer. The same rule is applicable to any syscall
>>>>> +   introduced in the future.
>>>> I thought Catalin wanted to drop this guarantee?
>>>>
>>> The guarantee is changed and explicitly includes the syscalls that can be added
>>> in the future. IMHO since we are defining an ABI, we cannot leave that topic in
>>> an uncharted territory, we need to address it.
>> It makes sense to me, just wanted to be sure that Catalin is on the same page.
>>
>>>>> + - 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 pointer and returns it to the
>>>>> +   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(PR_SET_MM, PR_SET_MM_MAP, ...) struct prctl_mm_map fields.
>>>>> + - prctl(PR_SET_MM, PR_SET_MM_MAP_SIZE, ...) struct prctl_mm_map fields.
>>>> All the PR_SET_MM options that specify pointers (PR_SET_MM_START_CODE,
>>>> PR_SET_MM_END_CODE, ...) should be excluded as well. AFAICT (but don't take my
>>>> word for it), that's all of them except PR_SET_MM_EXE_FILE. Conversely,
>>>> PR_SET_MM_MAP_SIZE should not be excluded (it does not pass a prctl_mm_map
>>>> struct, and the pointer to unsigned int can be tagged).
>>>>
>>> Agreed, I clearly misread the prctl() man page here. Fill fix in v7.
>>> PR_SET_MM_MAP_SIZE _returns_  struct prctl_mm_map, does not take it as a
>>> parameter.
>> OK. About PR_SET_MM_MAP_SIZE, it neither takes nor returns struct prctl_mm_map.
>> It writes the size of prctl_map to the int pointed to by arg3, and does nothing
>> else. Therefore, there's no need to exclude it.
>>
> Agreed, I missed the word size in my reply: s/_returns_  struct
> prctl_mm_map/_returns_  the size of struct prctl_mm_map/
>
>> BTW I've just realised that the man page is wrong about PR_SET_MM_MAP_SIZE, the
>> pointer to int is passed in arg3, not arg4. Anyone knows where to report that?
>>
>> Thanks,
>> Kevin
>>
>>> Vincenzo
>>>
>>>> Kevin
>>>>
>>>>> +
>>>>> +Any attempt to use non-zero tagged pointers will lead to undefined behaviour.
>>>>> +
>>>>> +4. Example of correct usage
>>>>> +---------------------------
>>>>> +.. code-block:: c
>>>>> +
>>>>> +   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