as_user_ptr() is intended to be used where an arbitrary integer e.g. an error code is stored in a __user ptr.
The current implementation can be somewhat confusing in that it looks like it can be used as direct replacement for u64_to_user_ptr e.g. in PCuABI, where u64 addresses in the kernel-user interface are being replaced with capability containing types such as __kernel_uintptr_t.
Currently, passing a valid capability represented as an integer e.g. any __kernel_uintptr_t, __uintcap_t or user_uintptr_t to as_user_ptr() will result in a cast to (void __user *) and a valid capability/pointer that can be dereferenced. This is contrary to the code comment and intended usage for as_user_ptr().
Add a cast to (u64) before the cast to (void __user *)(user_uintptr_t) to make clearer the intended usage. This also always results in a null capability that cannot be dereferenced.
Signed-off-by: Zachary Leaf zachary.leaf@arm.com --- Documentation/core-api/user_ptr.rst | 6 +++--- include/linux/user_ptr.h | 5 +++-- 2 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/Documentation/core-api/user_ptr.rst b/Documentation/core-api/user_ptr.rst index 21e02d4bd11b..e4cc0676a92b 100644 --- a/Documentation/core-api/user_ptr.rst +++ b/Documentation/core-api/user_ptr.rst @@ -103,7 +103,7 @@ Each function covers a particular category of input integer:
* **Arbitrary integer**:
- - Integer of any type: ``as_user_ptr()`` + - 64-bit or less integer, of any type: ``as_user_ptr()`` - ``u64`` (deprecated): ``u64_to_user_ptr()``
These functions are available in ``<linux/user_ptr.h>``, except @@ -141,8 +141,8 @@ derived from in the PCuABI case. | | | user-provided | | pointer to a native user pointer. | | | | ``compat_*`` struct | | | +------------------------------+--------------------+------------------------+-----------------------------------+------------------------------------------------------+ -| ``as_user_ptr()`` | Arbitrary integer | Error code | Null capability | This is a pure representation change, as suggested | -| | | | | by the ``as_`` prefix. The resulting pointer cannot | +| ``as_user_ptr()`` | Arbitrary 64-bit | Error code | Null capability | This is a pure representation change, as suggested | +| | or less integer | | | by the ``as_`` prefix. The resulting pointer cannot | | | | | | be dereferenced. | +------------------------------+--------------------+------------------------+-----------------------------------+------------------------------------------------------+ | ``u64_to_user_ptr()`` | ``u64`` integer | [Deprecated] | Null capability | Legacy function, new callers should not be added. | diff --git a/include/linux/user_ptr.h b/include/linux/user_ptr.h index 0942b58cfb6a..183e40ccc51f 100644 --- a/include/linux/user_ptr.h +++ b/include/linux/user_ptr.h @@ -9,13 +9,14 @@ #endif
/** - * as_user_ptr - convert an arbitrary integer value to a user pointer + * as_user_ptr - convert an arbitrary 64-bit or less integer value to a user + * pointer * @x: the integer value to convert * * Returns @x represented as a user pointer. The result is not a valid pointer * and shall not be dereferenced. */ -#define as_user_ptr(x) ((void __user *)(user_uintptr_t)(x)) +#define as_user_ptr(x) ((void __user *)(user_uintptr_t)(u64)(x))
/* Same semantics as as_user_ptr(), but also requires x to be of a given type */ #define as_user_ptr_strict(type, x) ( \
On 20/12/2022 13:20, Zachary Leaf wrote:
as_user_ptr() is intended to be used where an arbitrary integer e.g. an error code is stored in a __user ptr.
The current implementation can be somewhat confusing in that it looks like it can be used as direct replacement for u64_to_user_ptr e.g. in PCuABI, where u64 addresses in the kernel-user interface are being replaced with capability containing types such as __kernel_uintptr_t.
Currently, passing a valid capability represented as an integer e.g. any
In fact this is true for any capability, regardless of how it's represented (e.g. void * __capability as well).
__kernel_uintptr_t, __uintcap_t or user_uintptr_t to as_user_ptr() will result in a cast to (void __user *) and a valid capability/pointer that can be dereferenced. This is contrary to the code comment and intended usage for as_user_ptr().
Add a cast to (u64) before the cast to (void __user *)(user_uintptr_t) to make clearer the intended usage. This also always results in a null capability that cannot be dereferenced.
Signed-off-by: Zachary Leaf zachary.leaf@arm.com
Documentation/core-api/user_ptr.rst | 6 +++--- include/linux/user_ptr.h | 5 +++-- 2 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/Documentation/core-api/user_ptr.rst b/Documentation/core-api/user_ptr.rst index 21e02d4bd11b..e4cc0676a92b 100644 --- a/Documentation/core-api/user_ptr.rst +++ b/Documentation/core-api/user_ptr.rst @@ -103,7 +103,7 @@ Each function covers a particular category of input integer:
- **Arbitrary integer**:
- Integer of any type: ``as_user_ptr()``
- 64-bit or less integer, of any type: ``as_user_ptr()``
I'm not sure about this change and the related ones below. With the new implementation of as_user_ptr(), if you pass a user_uintptr_t (for instance), the right thing will still happen (i.e. a null-derived capability with the 64-bit value of the input). This may actually be used in practice, for instance the third argument of ioctl handlers is represented as user_uintptr_t, but most of the time it really is a 64-bit integer - user_uintptr_t having become the new catch-all user type - and you may want to get an (invalid) void __user * out of it.
What we could potentially say is that "user-pointer-carrying" integers should simply be cast to void __user *, without using any helper. This is what the "Representing user pointers" section already suggests, but we could reiterate here.
- ``u64`` (deprecated): ``u64_to_user_ptr()``
These functions are available in ``<linux/user_ptr.h>``, except @@ -141,8 +141,8 @@ derived from in the PCuABI case. | | | user-provided | | pointer to a native user pointer. | | | | ``compat_*`` struct | | | +------------------------------+--------------------+------------------------+-----------------------------------+------------------------------------------------------+ -| ``as_user_ptr()`` | Arbitrary integer | Error code | Null capability | This is a pure representation change, as suggested | -| | | | | by the ``as_`` prefix. The resulting pointer cannot | +| ``as_user_ptr()`` | Arbitrary 64-bit | Error code | Null capability | This is a pure representation change, as suggested | +| | or less integer | | | by the ``as_`` prefix. The resulting pointer cannot | | | | | | be dereferenced. | +------------------------------+--------------------+------------------------+-----------------------------------+------------------------------------------------------+ | ``u64_to_user_ptr()`` | ``u64`` integer | [Deprecated] | Null capability | Legacy function, new callers should not be added. | diff --git a/include/linux/user_ptr.h b/include/linux/user_ptr.h index 0942b58cfb6a..183e40ccc51f 100644 --- a/include/linux/user_ptr.h +++ b/include/linux/user_ptr.h @@ -9,13 +9,14 @@ #endif /**
- as_user_ptr - convert an arbitrary integer value to a user pointer
- as_user_ptr - convert an arbitrary 64-bit or less integer value to a user
- pointer
- @x: the integer value to convert
- Returns @x represented as a user pointer. The result is not a valid pointer
I think we could clarify by saying "the integer value of @x" (covering the special case of a __uintcap_t input).
Kevin
- and shall not be dereferenced.
*/ -#define as_user_ptr(x) ((void __user *)(user_uintptr_t)(x)) +#define as_user_ptr(x) ((void __user *)(user_uintptr_t)(u64)(x)) /* Same semantics as as_user_ptr(), but also requires x to be of a given type */ #define as_user_ptr_strict(type, x) ( \
On 21/12/2022 12:44, Kevin Brodsky wrote:
On 20/12/2022 13:20, Zachary Leaf wrote:
as_user_ptr() is intended to be used where an arbitrary integer e.g. an error code is stored in a __user ptr.
The current implementation can be somewhat confusing in that it looks like it can be used as direct replacement for u64_to_user_ptr e.g. in PCuABI, where u64 addresses in the kernel-user interface are being replaced with capability containing types such as __kernel_uintptr_t.
Currently, passing a valid capability represented as an integer e.g. any
In fact this is true for any capability, regardless of how it's represented (e.g. void * __capability as well).
Technically true :)
Changed in v2.
__kernel_uintptr_t, __uintcap_t or user_uintptr_t to as_user_ptr() will result in a cast to (void __user *) and a valid capability/pointer that can be dereferenced. This is contrary to the code comment and intended usage for as_user_ptr().
Add a cast to (u64) before the cast to (void __user *)(user_uintptr_t) to make clearer the intended usage. This also always results in a null capability that cannot be dereferenced.
Signed-off-by: Zachary Leaf zachary.leaf@arm.com
Documentation/core-api/user_ptr.rst | 6 +++--- include/linux/user_ptr.h | 5 +++-- 2 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/Documentation/core-api/user_ptr.rst b/Documentation/core-api/user_ptr.rst index 21e02d4bd11b..e4cc0676a92b 100644 --- a/Documentation/core-api/user_ptr.rst +++ b/Documentation/core-api/user_ptr.rst @@ -103,7 +103,7 @@ Each function covers a particular category of input integer:
- **Arbitrary integer**:
- Integer of any type: ``as_user_ptr()``
- 64-bit or less integer, of any type: ``as_user_ptr()``
I'm not sure about this change and the related ones below. With the new implementation of as_user_ptr(), if you pass a user_uintptr_t (for instance), the right thing will still happen (i.e. a null-derived capability with the 64-bit value of the input). This may actually be used in practice, for instance the third argument of ioctl handlers is represented as user_uintptr_t, but most of the time it really is a 64-bit integer - user_uintptr_t having become the new catch-all user type - and you may want to get an (invalid) void __user * out of it.
What we could potentially say is that "user-pointer-carrying" integers should simply be cast to void __user *, without using any helper. This is what the "Representing user pointers" section already suggests, but we could reiterate here.
Okay - yes it's a good point. See v2 - I've hopefully cleared it up and added another example in "Representing user pointers".
- ``u64`` (deprecated): ``u64_to_user_ptr()``
These functions are available in ``<linux/user_ptr.h>``, except @@ -141,8 +141,8 @@ derived from in the PCuABI case. | | | user-provided | | pointer to a native user pointer. | | | | ``compat_*`` struct | | | +------------------------------+--------------------+------------------------+-----------------------------------+------------------------------------------------------+ -| ``as_user_ptr()`` | Arbitrary integer | Error code | Null capability | This is a pure representation change, as suggested | -| | | | | by the ``as_`` prefix. The resulting pointer cannot | +| ``as_user_ptr()`` | Arbitrary 64-bit | Error code | Null capability | This is a pure representation change, as suggested | +| | or less integer | | | by the ``as_`` prefix. The resulting pointer cannot | | | | | | be dereferenced. | +------------------------------+--------------------+------------------------+-----------------------------------+------------------------------------------------------+ | ``u64_to_user_ptr()`` | ``u64`` integer | [Deprecated] | Null capability | Legacy function, new callers should not be added. | diff --git a/include/linux/user_ptr.h b/include/linux/user_ptr.h index 0942b58cfb6a..183e40ccc51f 100644 --- a/include/linux/user_ptr.h +++ b/include/linux/user_ptr.h @@ -9,13 +9,14 @@ #endif /**
- as_user_ptr - convert an arbitrary integer value to a user pointer
- as_user_ptr - convert an arbitrary 64-bit or less integer value to a user
- pointer
- @x: the integer value to convert
- Returns @x represented as a user pointer. The result is not a valid pointer
I think we could clarify by saying "the integer value of @x" (covering the special case of a __uintcap_t input).
As discussed offline, this doesn't exactly work for all cases. For one, if you pass a uint128_t then the integer value would be expected to be whatever the full 128b value is, not just the lowest 64b. Updated in v2 to be more precise about what exactly this does now.
Thanks for the review.
Zach
Kevin
- and shall not be dereferenced.
*/ -#define as_user_ptr(x) ((void __user *)(user_uintptr_t)(x)) +#define as_user_ptr(x) ((void __user *)(user_uintptr_t)(u64)(x)) /* Same semantics as as_user_ptr(), but also requires x to be of a given type */ #define as_user_ptr_strict(type, x) ( \
linux-morello@op-lists.linaro.org
-
Kevin Brodsky -
Zachary Leaf