Add a small guide documenting the most common changes required to get drivers (and potentially other subsystems) compatible with the pure-capability user ABI (PCuABI).
There is no obvious subdirectory for this type of document, and it is transient in nature, so add it at the top-level.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com --- Rendered version: https://git.morello-project.org/morello/kernel/linux/-/blob/review/kevin/por...
Documentation/pcuabi-porting.rst | 188 +++++++++++++++++++++++++++++++ 1 file changed, 188 insertions(+) create mode 100644 Documentation/pcuabi-porting.rst
diff --git a/Documentation/pcuabi-porting.rst b/Documentation/pcuabi-porting.rst new file mode 100644 index 000000000000..9c78523e39b7 --- /dev/null +++ b/Documentation/pcuabi-porting.rst @@ -0,0 +1,188 @@ +================================= +Adding PCuABI support to drivers +================================= + +This document provides a non-exhaustive overview of the most common +changes required to support the pure-capability user ABI (PCuABI) in +arbitrary drivers. It may also be helpful for core subsystems, though +note that more extensive changes may be required than for drivers with +straightforward interactions with userspace. + +.. _user pointer documentation: core-api/user_ptr.rst + +User pointer representation and conversions +=========================================== + +The appropriate API to represent and convert user pointers is described +in the `user pointer documentation`_. A few examples of modifications +required for PCuABI compliance: + ++--------------------------------+------------------------------------+ +| Invalid code | Potential replacement | ++================================+====================================+ +| ``(unsigned long)uptr`` | ``user_ptr_addr(uptr)`` | ++--------------------------------+------------------------------------+ +| ``(void __user *)u64`` | | ``uaddr_to_user_ptr(u64)`` | +| | | ``as_user_ptr(u64)`` | ++--------------------------------+------------------------------------+ +| ``get_user(uptr, &uarg->ptr)`` | ``get_user_ptr(uptr, &uarg->ptr)`` | ++--------------------------------+------------------------------------+ +| ``IS_ERR(ubuf)`` | ``USER_PTR_IS_ERR(ubuf)`` | ++--------------------------------+------------------------------------+ + +``ioctl`` handlers' third argument +================================== + +Traditionally, the type of the third argument of ``ioctl`` handlers is +``unsigned long``. Unfortunately this is no longer appropriate in PCuABI +in many cases, as this argument may represent a pointer. A larger type, +able to hold a capability, is therefore necessary: this type is +``user_uintptr_t``. + +The prototype of most ``ioctl`` methods has been modified to take +``user_uintptr_t`` as third argument (``arg``), notably +``struct file_operations::unlocked_ioctl`` and +``struct block_device_operations::ioctl``. As a consequence, any driver +implementing such methods must change the prototype of the callback +accordingly, that is replace ``unsigned long`` with ``user_uintptr_t``, +as the prototypes must match exactly. + +Additionally, if the handler passes down ``arg`` to other functions, +such functions must also represent this argument as ``user_uintptr_t`` +**if they handle any request type where arg is a pointer** - in other +words, if they cast ``arg`` to a user pointer, e.g. ``void __user *``. +Otherwise, it is acceptable to leave these functions unchanged (i.e. +keep taking an ``unsigned long``). + +Compat ``ioctl`` handlers +========================= + +Third argument +-------------- + +Unlike native ``ioctl`` handlers, the type of the third argument of +compat ``ioctl`` handlers remains ``unsigned long``, as this remains an +appropriate type to represent compat pointers in both compat32 and +compat64. As a result, a native handler can no longer be used as a compat +handler, because their prototypes differ. An appropriate compat handler +must therefore always be chosen. + +Compat user pointers should always be converted to native user pointers +using ``compat_ptr()``. Unfortunately this is not always the case in +existing drivers, especially in compat ``ioctl`` handlers, which may +simply pass their arguments through to the native handler without using +the appropriate pointer conversion. This is complicated by the fact that +the actual type of the third argument (``arg``) may depend on the +request type, just like in native handlers. The subsections below cover +the various possible situations. + +``arg`` is always a pointer +~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +If ``arg`` always represents a pointer, it can be unconditionally +converted to a native pointer and passed to the native handler, for +instance:: + + static long my_compat_ioctl(struct file *file, unsigned int cmd, + unsigned long arg) + { + return my_ioctl(file, cmd, (user_uintptr_t)compat_ptr(arg)); + } + +For such a trivial ``file_operations`` compat handler, there is in fact +no need to create a compat handler; the ``compat_ptr_ioctl`` helper can +be used instead:: + + static const struct file_operations my_fops = { + ... + .compat_ioctl = compat_ptr_ioctl, + ... + }; + +``arg`` is never a pointer +~~~~~~~~~~~~~~~~~~~~~~~~~~ + +If ``arg`` never represents a pointer, it can directly be passed to +the native handler, optionally cast to ``user_uintptr_t``. + +Similarly, in this situation, the ``compat_noptr_ioctl`` helper can be +used instead of writing a trivial ``file_operations`` compat handler:: + + static const struct file_operations my_fops = { + ... + .compat_ioctl = compat_noptr_ioctl, + ... + }; + +``arg`` is sometimes a pointer +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +If ``arg`` represents a pointer for certain request types but not +others, then the compat handler should ensure that the appropriate +conversion is made depending on the request type, i.e. ``compat_ptr()`` +is used **if and only if arg is a pointer for the specific request +type.** + +Struct layout / 32-bit assumptions +---------------------------------- + +Aside from the conversion of ``arg``, existing compat ``ioctl`` handlers +should be examined carefully as they typically include two different +types of transformations: + +1. Struct layout transformation, which may include pointer conversions. +2. 32-bit-specific transformations, e.g. for 32-bit time representation. + +The difficulty with supporting compat64 is that the first transformation +may still required, while the second transformation is not relevant and +should not be carried out in compat64 (some ``#ifdef CONFIG_COMPAT32`` +may be required). + +When a given request takes a pointer to a struct, and that struct +contains types that differ in compat, it is normally represented as +``struct compat_<name>`` in the compat handler. It may happen that some +of the types used in this struct are only appropriate for compat32. The +preferred approach in this case is to change these types to appropriate +``compat_*`` types, for instance ``compat_long`` instead of ``s32``. +This holds even if the entire transformation is unnecessary in compat64; +this is so that the compat handler is kept as generic as possible. + +When the struct contains pointers, they must be represented as +``compat_uptr_t`` (preferred) or ``compat_caddr_t`` in its compat +counterpart, and conversions between compat user pointers and native +user pointers must always be made using ``compat_ptr()`` and +``ptr_to_compat()``. + +``__user`` annotation fixups +============================ + +Most complex types involving ``__user`` fail to build in PCuABI, notably +double user pointers (user pointer to a user pointer). This is because +using ``__capability`` as a prefix to ``*`` only has the intended +meaning in a limited number of situations. Otherwise, the compiler will +typically throw the following error:: + + error: use of __capability is ambiguous + +A fixup is then required, as described in section "PCuABI-specific +changes" of the `user pointer documentation`_. For instance:: + + diff --git a/net/socket.c b/net/socket.c + index 8597fbacb089..ab2a610825cc 100644 + --- a/net/socket.c + +++ b/net/socket.c + @@ -3156,7 +3169,11 @@ void socket_seq_show(struct seq_file *seq) + * the next page isn't readable/writable, we get a fault. To prevent + * that, copy back and forth to the full size. + */ + +#ifdef CONFIG_CHERI_PURECAP_UABI + +int get_user_ifreq(struct ifreq *ifr, void * __capability *ifrdata, void __user *arg) + +#else + int get_user_ifreq(struct ifreq *ifr, void __user **ifrdata, void __user *arg) + +#endif + { + if (in_compat_syscall()) { + struct compat_ifreq *ifr32 = (struct compat_ifreq *)ifr; + +Fortunately, ``__user`` is mostly used in simple types, and such fixups +are rarely needed in driver code.
On 01/08/2022 16:53, Kevin Brodsky wrote:
Add a small guide documenting the most common changes required to get drivers (and potentially other subsystems) compatible with the pure-capability user ABI (PCuABI).
There is no obvious subdirectory for this type of document, and it is transient in nature, so add it at the top-level.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com
Rendered version: https://git.morello-project.org/morello/kernel/linux/-/blob/review/kevin/por...
The review branch has moved, new link to the rendered version: https://git.morello-project.org/kbrodsky-arm/linux/-/blob/porting_guide/Docu...
Kevin
Documentation/pcuabi-porting.rst | 188 +++++++++++++++++++++++++++++++ 1 file changed, 188 insertions(+) create mode 100644 Documentation/pcuabi-porting.rst
diff --git a/Documentation/pcuabi-porting.rst b/Documentation/pcuabi-porting.rst new file mode 100644 index 000000000000..9c78523e39b7 --- /dev/null +++ b/Documentation/pcuabi-porting.rst @@ -0,0 +1,188 @@ +================================= +Adding PCuABI support to drivers +=================================
+This document provides a non-exhaustive overview of the most common +changes required to support the pure-capability user ABI (PCuABI) in +arbitrary drivers. It may also be helpful for core subsystems, though +note that more extensive changes may be required than for drivers with +straightforward interactions with userspace.
+.. _user pointer documentation: core-api/user_ptr.rst
+User pointer representation and conversions +===========================================
+The appropriate API to represent and convert user pointers is described +in the `user pointer documentation`_. A few examples of modifications +required for PCuABI compliance:
++--------------------------------+------------------------------------+ +| Invalid code | Potential replacement | ++================================+====================================+ +| ``(unsigned long)uptr`` | ``user_ptr_addr(uptr)`` | ++--------------------------------+------------------------------------+ +| ``(void __user *)u64`` | | ``uaddr_to_user_ptr(u64)`` | +| | | ``as_user_ptr(u64)`` | ++--------------------------------+------------------------------------+ +| ``get_user(uptr, &uarg->ptr)`` | ``get_user_ptr(uptr, &uarg->ptr)`` | ++--------------------------------+------------------------------------+ +| ``IS_ERR(ubuf)`` | ``USER_PTR_IS_ERR(ubuf)`` | ++--------------------------------+------------------------------------+
+``ioctl`` handlers' third argument +==================================
+Traditionally, the type of the third argument of ``ioctl`` handlers is +``unsigned long``. Unfortunately this is no longer appropriate in PCuABI +in many cases, as this argument may represent a pointer. A larger type, +able to hold a capability, is therefore necessary: this type is +``user_uintptr_t``.
+The prototype of most ``ioctl`` methods has been modified to take +``user_uintptr_t`` as third argument (``arg``), notably +``struct file_operations::unlocked_ioctl`` and +``struct block_device_operations::ioctl``. As a consequence, any driver +implementing such methods must change the prototype of the callback +accordingly, that is replace ``unsigned long`` with ``user_uintptr_t``, +as the prototypes must match exactly.
+Additionally, if the handler passes down ``arg`` to other functions, +such functions must also represent this argument as ``user_uintptr_t`` +**if they handle any request type where arg is a pointer** - in other +words, if they cast ``arg`` to a user pointer, e.g. ``void __user *``. +Otherwise, it is acceptable to leave these functions unchanged (i.e. +keep taking an ``unsigned long``).
+Compat ``ioctl`` handlers +=========================
+Third argument +--------------
+Unlike native ``ioctl`` handlers, the type of the third argument of +compat ``ioctl`` handlers remains ``unsigned long``, as this remains an +appropriate type to represent compat pointers in both compat32 and +compat64. As a result, a native handler can no longer be used as a compat +handler, because their prototypes differ. An appropriate compat handler +must therefore always be chosen.
+Compat user pointers should always be converted to native user pointers +using ``compat_ptr()``. Unfortunately this is not always the case in +existing drivers, especially in compat ``ioctl`` handlers, which may +simply pass their arguments through to the native handler without using +the appropriate pointer conversion. This is complicated by the fact that +the actual type of the third argument (``arg``) may depend on the +request type, just like in native handlers. The subsections below cover +the various possible situations.
+``arg`` is always a pointer +~~~~~~~~~~~~~~~~~~~~~~~~~~~
+If ``arg`` always represents a pointer, it can be unconditionally +converted to a native pointer and passed to the native handler, for +instance::
- static long my_compat_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)- {
return my_ioctl(file, cmd, (user_uintptr_t)compat_ptr(arg));- }
+For such a trivial ``file_operations`` compat handler, there is in fact +no need to create a compat handler; the ``compat_ptr_ioctl`` helper can +be used instead::
- static const struct file_operations my_fops = {
....compat_ioctl = compat_ptr_ioctl,...- };
+``arg`` is never a pointer +~~~~~~~~~~~~~~~~~~~~~~~~~~
+If ``arg`` never represents a pointer, it can directly be passed to +the native handler, optionally cast to ``user_uintptr_t``.
+Similarly, in this situation, the ``compat_noptr_ioctl`` helper can be +used instead of writing a trivial ``file_operations`` compat handler::
- static const struct file_operations my_fops = {
....compat_ioctl = compat_noptr_ioctl,...- };
+``arg`` is sometimes a pointer +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+If ``arg`` represents a pointer for certain request types but not +others, then the compat handler should ensure that the appropriate +conversion is made depending on the request type, i.e. ``compat_ptr()`` +is used **if and only if arg is a pointer for the specific request +type.**
+Struct layout / 32-bit assumptions +----------------------------------
+Aside from the conversion of ``arg``, existing compat ``ioctl`` handlers +should be examined carefully as they typically include two different +types of transformations:
+1. Struct layout transformation, which may include pointer conversions. +2. 32-bit-specific transformations, e.g. for 32-bit time representation.
+The difficulty with supporting compat64 is that the first transformation +may still required, while the second transformation is not relevant and +should not be carried out in compat64 (some ``#ifdef CONFIG_COMPAT32`` +may be required).
+When a given request takes a pointer to a struct, and that struct +contains types that differ in compat, it is normally represented as +``struct compat_<name>`` in the compat handler. It may happen that some +of the types used in this struct are only appropriate for compat32. The +preferred approach in this case is to change these types to appropriate +``compat_*`` types, for instance ``compat_long`` instead of ``s32``. +This holds even if the entire transformation is unnecessary in compat64; +this is so that the compat handler is kept as generic as possible.
+When the struct contains pointers, they must be represented as +``compat_uptr_t`` (preferred) or ``compat_caddr_t`` in its compat +counterpart, and conversions between compat user pointers and native +user pointers must always be made using ``compat_ptr()`` and +``ptr_to_compat()``.
+``__user`` annotation fixups +============================
+Most complex types involving ``__user`` fail to build in PCuABI, notably +double user pointers (user pointer to a user pointer). This is because +using ``__capability`` as a prefix to ``*`` only has the intended +meaning in a limited number of situations. Otherwise, the compiler will +typically throw the following error::
- error: use of __capability is ambiguous
+A fixup is then required, as described in section "PCuABI-specific +changes" of the `user pointer documentation`_. For instance::
- diff --git a/net/socket.c b/net/socket.c
- index 8597fbacb089..ab2a610825cc 100644
- --- a/net/socket.c
- +++ b/net/socket.c
- @@ -3156,7 +3169,11 @@ void socket_seq_show(struct seq_file *seq)
- the next page isn't readable/writable, we get a fault. To prevent
- that, copy back and forth to the full size.
- */
- +#ifdef CONFIG_CHERI_PURECAP_UABI
- +int get_user_ifreq(struct ifreq *ifr, void * __capability *ifrdata, void __user *arg)
- +#else
- int get_user_ifreq(struct ifreq *ifr, void __user **ifrdata, void __user *arg)
- +#endif
- {
if (in_compat_syscall()) {struct compat_ifreq *ifr32 = (struct compat_ifreq *)ifr;+Fortunately, ``__user`` is mostly used in simple types, and such fixups +are rarely needed in driver code.
On 01/08/2022 15:53, Kevin Brodsky wrote:
Add a small guide documenting the most common changes required to get drivers (and potentially other subsystems) compatible with the pure-capability user ABI (PCuABI).
There is no obvious subdirectory for this type of document, and it is transient in nature, so add it at the top-level.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com
Rendered version: https://git.morello-project.org/morello/kernel/linux/-/blob/review/kevin/por...
Documentation/pcuabi-porting.rst | 188 +++++++++++++++++++++++++++++++ 1 file changed, 188 insertions(+) create mode 100644 Documentation/pcuabi-porting.rst
diff --git a/Documentation/pcuabi-porting.rst b/Documentation/pcuabi-porting.rst new file mode 100644 index 000000000000..9c78523e39b7 --- /dev/null +++ b/Documentation/pcuabi-porting.rst @@ -0,0 +1,188 @@ +================================= +Adding PCuABI support to drivers +=================================
+This document provides a non-exhaustive overview of the most common +changes required to support the pure-capability user ABI (PCuABI) in +arbitrary drivers. It may also be helpful for core subsystems, though +note that more extensive changes may be required than for drivers with +straightforward interactions with userspace.
Minor suggestion: s/than for/compared to/
+.. _user pointer documentation: core-api/user_ptr.rst
+User pointer representation and conversions +===========================================
+The appropriate API to represent and convert user pointers is described +in the `user pointer documentation`_. A few examples of modifications +required for PCuABI compliance:
++--------------------------------+------------------------------------+ +| Invalid code | Potential replacement | ++================================+====================================+ +| ``(unsigned long)uptr`` | ``user_ptr_addr(uptr)`` | ++--------------------------------+------------------------------------+ +| ``(void __user *)u64`` | | ``uaddr_to_user_ptr(u64)`` | +| | | ``as_user_ptr(u64)`` | ++--------------------------------+------------------------------------+ +| ``get_user(uptr, &uarg->ptr)`` | ``get_user_ptr(uptr, &uarg->ptr)`` | ++--------------------------------+------------------------------------+ +| ``IS_ERR(ubuf)`` | ``USER_PTR_IS_ERR(ubuf)`` | ++--------------------------------+------------------------------------+
Do you think it might be worth adding a note stating this list is not exhaustive? Even an additional row with '...' might do it. Just thinking at a glance it might be easy to miss "a few examples" and think this list is complete.
Otherwise looks good, no further comments.
Zach
+``ioctl`` handlers' third argument +==================================
+Traditionally, the type of the third argument of ``ioctl`` handlers is +``unsigned long``. Unfortunately this is no longer appropriate in PCuABI +in many cases, as this argument may represent a pointer. A larger type, +able to hold a capability, is therefore necessary: this type is +``user_uintptr_t``.
+The prototype of most ``ioctl`` methods has been modified to take +``user_uintptr_t`` as third argument (``arg``), notably +``struct file_operations::unlocked_ioctl`` and +``struct block_device_operations::ioctl``. As a consequence, any driver +implementing such methods must change the prototype of the callback +accordingly, that is replace ``unsigned long`` with ``user_uintptr_t``, +as the prototypes must match exactly.
+Additionally, if the handler passes down ``arg`` to other functions, +such functions must also represent this argument as ``user_uintptr_t`` +**if they handle any request type where arg is a pointer** - in other +words, if they cast ``arg`` to a user pointer, e.g. ``void __user *``. +Otherwise, it is acceptable to leave these functions unchanged (i.e. +keep taking an ``unsigned long``).
+Compat ``ioctl`` handlers +=========================
+Third argument +--------------
+Unlike native ``ioctl`` handlers, the type of the third argument of +compat ``ioctl`` handlers remains ``unsigned long``, as this remains an +appropriate type to represent compat pointers in both compat32 and +compat64. As a result, a native handler can no longer be used as a compat +handler, because their prototypes differ. An appropriate compat handler +must therefore always be chosen.
+Compat user pointers should always be converted to native user pointers +using ``compat_ptr()``. Unfortunately this is not always the case in +existing drivers, especially in compat ``ioctl`` handlers, which may +simply pass their arguments through to the native handler without using +the appropriate pointer conversion. This is complicated by the fact that +the actual type of the third argument (``arg``) may depend on the +request type, just like in native handlers. The subsections below cover +the various possible situations.
+``arg`` is always a pointer +~~~~~~~~~~~~~~~~~~~~~~~~~~~
+If ``arg`` always represents a pointer, it can be unconditionally +converted to a native pointer and passed to the native handler, for +instance::
- static long my_compat_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)- {
return my_ioctl(file, cmd, (user_uintptr_t)compat_ptr(arg));- }
+For such a trivial ``file_operations`` compat handler, there is in fact +no need to create a compat handler; the ``compat_ptr_ioctl`` helper can +be used instead::
- static const struct file_operations my_fops = {
....compat_ioctl = compat_ptr_ioctl,...- };
+``arg`` is never a pointer +~~~~~~~~~~~~~~~~~~~~~~~~~~
+If ``arg`` never represents a pointer, it can directly be passed to +the native handler, optionally cast to ``user_uintptr_t``.
+Similarly, in this situation, the ``compat_noptr_ioctl`` helper can be +used instead of writing a trivial ``file_operations`` compat handler::
- static const struct file_operations my_fops = {
....compat_ioctl = compat_noptr_ioctl,...- };
+``arg`` is sometimes a pointer +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+If ``arg`` represents a pointer for certain request types but not +others, then the compat handler should ensure that the appropriate +conversion is made depending on the request type, i.e. ``compat_ptr()`` +is used **if and only if arg is a pointer for the specific request +type.**
+Struct layout / 32-bit assumptions +----------------------------------
+Aside from the conversion of ``arg``, existing compat ``ioctl`` handlers +should be examined carefully as they typically include two different +types of transformations:
+1. Struct layout transformation, which may include pointer conversions. +2. 32-bit-specific transformations, e.g. for 32-bit time representation.
+The difficulty with supporting compat64 is that the first transformation +may still required, while the second transformation is not relevant and +should not be carried out in compat64 (some ``#ifdef CONFIG_COMPAT32`` +may be required).
+When a given request takes a pointer to a struct, and that struct +contains types that differ in compat, it is normally represented as +``struct compat_<name>`` in the compat handler. It may happen that some +of the types used in this struct are only appropriate for compat32. The +preferred approach in this case is to change these types to appropriate +``compat_*`` types, for instance ``compat_long`` instead of ``s32``. +This holds even if the entire transformation is unnecessary in compat64; +this is so that the compat handler is kept as generic as possible.
+When the struct contains pointers, they must be represented as +``compat_uptr_t`` (preferred) or ``compat_caddr_t`` in its compat +counterpart, and conversions between compat user pointers and native +user pointers must always be made using ``compat_ptr()`` and +``ptr_to_compat()``.
+``__user`` annotation fixups +============================
+Most complex types involving ``__user`` fail to build in PCuABI, notably +double user pointers (user pointer to a user pointer). This is because +using ``__capability`` as a prefix to ``*`` only has the intended +meaning in a limited number of situations. Otherwise, the compiler will +typically throw the following error::
- error: use of __capability is ambiguous
+A fixup is then required, as described in section "PCuABI-specific +changes" of the `user pointer documentation`_. For instance::
- diff --git a/net/socket.c b/net/socket.c
- index 8597fbacb089..ab2a610825cc 100644
- --- a/net/socket.c
- +++ b/net/socket.c
- @@ -3156,7 +3169,11 @@ void socket_seq_show(struct seq_file *seq)
- the next page isn't readable/writable, we get a fault. To prevent
- that, copy back and forth to the full size.
- */
- +#ifdef CONFIG_CHERI_PURECAP_UABI
- +int get_user_ifreq(struct ifreq *ifr, void * __capability *ifrdata, void __user *arg)
- +#else
- int get_user_ifreq(struct ifreq *ifr, void __user **ifrdata, void __user *arg)
- +#endif
- {
if (in_compat_syscall()) {struct compat_ifreq *ifr32 = (struct compat_ifreq *)ifr;+Fortunately, ``__user`` is mostly used in simple types, and such fixups +are rarely needed in driver code.
On 05/08/2022 10:34, Zachary Leaf wrote:
On 01/08/2022 15:53, Kevin Brodsky wrote:
Add a small guide documenting the most common changes required to get drivers (and potentially other subsystems) compatible with the pure-capability user ABI (PCuABI).
There is no obvious subdirectory for this type of document, and it is transient in nature, so add it at the top-level.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com
Rendered version: https://git.morello-project.org/morello/kernel/linux/-/blob/review/kevin/por...
Documentation/pcuabi-porting.rst | 188 +++++++++++++++++++++++++++++++ 1 file changed, 188 insertions(+) create mode 100644 Documentation/pcuabi-porting.rst
diff --git a/Documentation/pcuabi-porting.rst b/Documentation/pcuabi-porting.rst new file mode 100644 index 000000000000..9c78523e39b7 --- /dev/null +++ b/Documentation/pcuabi-porting.rst @@ -0,0 +1,188 @@ +================================= +Adding PCuABI support to drivers +=================================
+This document provides a non-exhaustive overview of the most common +changes required to support the pure-capability user ABI (PCuABI) in +arbitrary drivers. It may also be helpful for core subsystems, though +note that more extensive changes may be required than for drivers with +straightforward interactions with userspace.
Minor suggestion: s/than for/compared to/
I did feel that this was a bit askew, thanks!
+.. _user pointer documentation: core-api/user_ptr.rst
+User pointer representation and conversions +===========================================
+The appropriate API to represent and convert user pointers is described +in the `user pointer documentation`_. A few examples of modifications +required for PCuABI compliance:
++--------------------------------+------------------------------------+ +| Invalid code | Potential replacement | ++================================+====================================+ +| ``(unsigned long)uptr`` | ``user_ptr_addr(uptr)`` | ++--------------------------------+------------------------------------+ +| ``(void __user *)u64`` | | ``uaddr_to_user_ptr(u64)`` | +| | | ``as_user_ptr(u64)`` | ++--------------------------------+------------------------------------+ +| ``get_user(uptr, &uarg->ptr)`` | ``get_user_ptr(uptr, &uarg->ptr)`` | ++--------------------------------+------------------------------------+ +| ``IS_ERR(ubuf)`` | ``USER_PTR_IS_ERR(ubuf)`` | ++--------------------------------+------------------------------------+
Do you think it might be worth adding a note stating this list is not exhaustive? Even an additional row with '...' might do it. Just thinking at a glance it might be easy to miss "a few examples" and think this list is complete.
Agreed, it can't hurt to add a row with "..." to make that clear.
Otherwise looks good, no further comments.
Thanks for the review!
Kevin
Zach
+``ioctl`` handlers' third argument +==================================
+Traditionally, the type of the third argument of ``ioctl`` handlers is +``unsigned long``. Unfortunately this is no longer appropriate in PCuABI +in many cases, as this argument may represent a pointer. A larger type, +able to hold a capability, is therefore necessary: this type is +``user_uintptr_t``.
+The prototype of most ``ioctl`` methods has been modified to take +``user_uintptr_t`` as third argument (``arg``), notably +``struct file_operations::unlocked_ioctl`` and +``struct block_device_operations::ioctl``. As a consequence, any driver +implementing such methods must change the prototype of the callback +accordingly, that is replace ``unsigned long`` with ``user_uintptr_t``, +as the prototypes must match exactly.
+Additionally, if the handler passes down ``arg`` to other functions, +such functions must also represent this argument as ``user_uintptr_t`` +**if they handle any request type where arg is a pointer** - in other +words, if they cast ``arg`` to a user pointer, e.g. ``void __user *``. +Otherwise, it is acceptable to leave these functions unchanged (i.e. +keep taking an ``unsigned long``).
+Compat ``ioctl`` handlers +=========================
+Third argument +--------------
+Unlike native ``ioctl`` handlers, the type of the third argument of +compat ``ioctl`` handlers remains ``unsigned long``, as this remains an +appropriate type to represent compat pointers in both compat32 and +compat64. As a result, a native handler can no longer be used as a compat +handler, because their prototypes differ. An appropriate compat handler +must therefore always be chosen.
+Compat user pointers should always be converted to native user pointers +using ``compat_ptr()``. Unfortunately this is not always the case in +existing drivers, especially in compat ``ioctl`` handlers, which may +simply pass their arguments through to the native handler without using +the appropriate pointer conversion. This is complicated by the fact that +the actual type of the third argument (``arg``) may depend on the +request type, just like in native handlers. The subsections below cover +the various possible situations.
+``arg`` is always a pointer +~~~~~~~~~~~~~~~~~~~~~~~~~~~
+If ``arg`` always represents a pointer, it can be unconditionally +converted to a native pointer and passed to the native handler, for +instance::
- static long my_compat_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)- {
return my_ioctl(file, cmd, (user_uintptr_t)compat_ptr(arg));- }
+For such a trivial ``file_operations`` compat handler, there is in fact +no need to create a compat handler; the ``compat_ptr_ioctl`` helper can +be used instead::
- static const struct file_operations my_fops = {
....compat_ioctl = compat_ptr_ioctl,...- };
+``arg`` is never a pointer +~~~~~~~~~~~~~~~~~~~~~~~~~~
+If ``arg`` never represents a pointer, it can directly be passed to +the native handler, optionally cast to ``user_uintptr_t``.
+Similarly, in this situation, the ``compat_noptr_ioctl`` helper can be +used instead of writing a trivial ``file_operations`` compat handler::
- static const struct file_operations my_fops = {
....compat_ioctl = compat_noptr_ioctl,...- };
+``arg`` is sometimes a pointer +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+If ``arg`` represents a pointer for certain request types but not +others, then the compat handler should ensure that the appropriate +conversion is made depending on the request type, i.e. ``compat_ptr()`` +is used **if and only if arg is a pointer for the specific request +type.**
+Struct layout / 32-bit assumptions +----------------------------------
+Aside from the conversion of ``arg``, existing compat ``ioctl`` handlers +should be examined carefully as they typically include two different +types of transformations:
+1. Struct layout transformation, which may include pointer conversions. +2. 32-bit-specific transformations, e.g. for 32-bit time representation.
+The difficulty with supporting compat64 is that the first transformation +may still required, while the second transformation is not relevant and +should not be carried out in compat64 (some ``#ifdef CONFIG_COMPAT32`` +may be required).
+When a given request takes a pointer to a struct, and that struct +contains types that differ in compat, it is normally represented as +``struct compat_<name>`` in the compat handler. It may happen that some +of the types used in this struct are only appropriate for compat32. The +preferred approach in this case is to change these types to appropriate +``compat_*`` types, for instance ``compat_long`` instead of ``s32``. +This holds even if the entire transformation is unnecessary in compat64; +this is so that the compat handler is kept as generic as possible.
+When the struct contains pointers, they must be represented as +``compat_uptr_t`` (preferred) or ``compat_caddr_t`` in its compat +counterpart, and conversions between compat user pointers and native +user pointers must always be made using ``compat_ptr()`` and +``ptr_to_compat()``.
+``__user`` annotation fixups +============================
+Most complex types involving ``__user`` fail to build in PCuABI, notably +double user pointers (user pointer to a user pointer). This is because +using ``__capability`` as a prefix to ``*`` only has the intended +meaning in a limited number of situations. Otherwise, the compiler will +typically throw the following error::
- error: use of __capability is ambiguous
+A fixup is then required, as described in section "PCuABI-specific +changes" of the `user pointer documentation`_. For instance::
- diff --git a/net/socket.c b/net/socket.c
- index 8597fbacb089..ab2a610825cc 100644
- --- a/net/socket.c
- +++ b/net/socket.c
- @@ -3156,7 +3169,11 @@ void socket_seq_show(struct seq_file *seq)
- the next page isn't readable/writable, we get a fault. To prevent
- that, copy back and forth to the full size.
- */
- +#ifdef CONFIG_CHERI_PURECAP_UABI
- +int get_user_ifreq(struct ifreq *ifr, void * __capability *ifrdata, void __user *arg)
- +#else
- int get_user_ifreq(struct ifreq *ifr, void __user **ifrdata, void __user *arg)
- +#endif
- {
if (in_compat_syscall()) {struct compat_ifreq *ifr32 = (struct compat_ifreq *)ifr;+Fortunately, ``__user`` is mostly used in simple types, and such fixups +are rarely needed in driver code.
linux-morello mailing list -- linux-morello@op-lists.linaro.org To unsubscribe send an email to linux-morello-leave@op-lists.linaro.org
First - a good read. This needed writing no matter what and likely will need massaging over time as more drivers get attention.
Feedback as below in-line. :)
On 8/1/22 15:53, Kevin Brodsky wrote:
Add a small guide documenting the most common changes required to get drivers (and potentially other subsystems) compatible with the pure-capability user ABI (PCuABI).
There is no obvious subdirectory for this type of document, and it is transient in nature, so add it at the top-level.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com
Rendered version: https://git.morello-project.org/morello/kernel/linux/-/blob/review/kevin/por...
Documentation/pcuabi-porting.rst | 188 +++++++++++++++++++++++++++++++ 1 file changed, 188 insertions(+) create mode 100644 Documentation/pcuabi-porting.rst
diff --git a/Documentation/pcuabi-porting.rst b/Documentation/pcuabi-porting.rst new file mode 100644 index 000000000000..9c78523e39b7 --- /dev/null +++ b/Documentation/pcuabi-porting.rst @@ -0,0 +1,188 @@ +================================= +Adding PCuABI support to drivers +=================================
+This document provides a non-exhaustive overview of the most common +changes required to support the pure-capability user ABI (PCuABI) in +arbitrary drivers. It may also be helpful for core subsystems, though +note that more extensive changes may be required than for drivers with +straightforward interactions with userspace.
+.. _user pointer documentation: core-api/user_ptr.rst
+User pointer representation and conversions +===========================================
+The appropriate API to represent and convert user pointers is described +in the `user pointer documentation`_. A few examples of modifications +required for PCuABI compliance:
++--------------------------------+------------------------------------+ +| Invalid code | Potential replacement | ++================================+====================================+ +| ``(unsigned long)uptr`` | ``user_ptr_addr(uptr)`` | ++--------------------------------+------------------------------------+ +| ``(void __user *)u64`` | | ``uaddr_to_user_ptr(u64)`` | +| | | ``as_user_ptr(u64)`` | ++--------------------------------+------------------------------------+ +| ``get_user(uptr, &uarg->ptr)`` | ``get_user_ptr(uptr, &uarg->ptr)`` | ++--------------------------------+------------------------------------+ +| ``IS_ERR(ubuf)`` | ``USER_PTR_IS_ERR(ubuf)`` | ++--------------------------------+------------------------------------+
+``ioctl`` handlers' third argument +==================================
+Traditionally, the type of the third argument of ``ioctl`` handlers is +``unsigned long``. Unfortunately this is no longer appropriate in PCuABI +in many cases, as this argument may represent a pointer. A larger type, +able to hold a capability, is therefore necessary: this type is +``user_uintptr_t``.
+The prototype of most ``ioctl`` methods has been modified to take +``user_uintptr_t`` as third argument (``arg``), notably +``struct file_operations::unlocked_ioctl`` and +``struct block_device_operations::ioctl``. As a consequence, any driver +implementing such methods must change the prototype of the callback +accordingly, that is replace ``unsigned long`` with ``user_uintptr_t``, +as the prototypes must match exactly.
+Additionally, if the handler passes down ``arg`` to other functions, +such functions must also represent this argument as ``user_uintptr_t`` +**if they handle any request type where arg is a pointer** - in other +words, if they cast ``arg`` to a user pointer, e.g. ``void __user *``. +Otherwise, it is acceptable to leave these functions unchanged (i.e. +keep taking an ``unsigned long``).
+Compat ``ioctl`` handlers +=========================
+Third argument +--------------
+Unlike native ``ioctl`` handlers, the type of the third argument of +compat ``ioctl`` handlers remains ``unsigned long``, as this remains an +appropriate type to represent compat pointers in both compat32 and +compat64. As a result, a native handler can no longer be used as a compat +handler, because their prototypes differ. An appropriate compat handler +must therefore always be chosen.
I'm scratching my head above about compat32. I thought ... we had no 32bit compat? This is more regular aarch64/arm64 64bit compat vs purecap? My brain is going "shouldn't this be compa64 vs compat128?" (I know strictly its 129 bit as an extra cap bit is there too in memory).
I know we're USING the COMPAT32 ifdefs etc. really to mean 64bit non-purecap but it's confusing as above.
+Compat user pointers should always be converted to native user pointers +using ``compat_ptr()``. Unfortunately this is not always the case in +existing drivers, especially in compat ``ioctl`` handlers, which may +simply pass their arguments through to the native handler without using +the appropriate pointer conversion. This is complicated by the fact that +the actual type of the third argument (``arg``) may depend on the +request type, just like in native handlers. The subsections below cover +the various possible situations.
+``arg`` is always a pointer +~~~~~~~~~~~~~~~~~~~~~~~~~~~
+If ``arg`` always represents a pointer, it can be unconditionally +converted to a native pointer and passed to the native handler, for +instance::
- static long my_compat_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)- {
return my_ioctl(file, cmd, (user_uintptr_t)compat_ptr(arg));- }
+For such a trivial ``file_operations`` compat handler, there is in fact +no need to create a compat handler; the ``compat_ptr_ioctl`` helper can +be used instead::
- static const struct file_operations my_fops = {
....compat_ioctl = compat_ptr_ioctl,...- };
+``arg`` is never a pointer +~~~~~~~~~~~~~~~~~~~~~~~~~~
+If ``arg`` never represents a pointer, it can directly be passed to +the native handler, optionally cast to ``user_uintptr_t``.
+Similarly, in this situation, the ``compat_noptr_ioctl`` helper can be +used instead of writing a trivial ``file_operations`` compat handler::
- static const struct file_operations my_fops = {
....compat_ioctl = compat_noptr_ioctl,...- };
+``arg`` is sometimes a pointer +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+If ``arg`` represents a pointer for certain request types but not +others, then the compat handler should ensure that the appropriate +conversion is made depending on the request type, i.e. ``compat_ptr()`` +is used **if and only if arg is a pointer for the specific request +type.**
+Struct layout / 32-bit assumptions +----------------------------------
Again ... 32bit. I'm scratching my head a bit? Not related to 32bit amrv7 and before?
+Aside from the conversion of ``arg``, existing compat ``ioctl`` handlers +should be examined carefully as they typically include two different +types of transformations:
+1. Struct layout transformation, which may include pointer conversions. +2. 32-bit-specific transformations, e.g. for 32-bit time representation.
+The difficulty with supporting compat64 is that the first transformation +may still required, while the second transformation is not relevant and +should not be carried out in compat64 (some ``#ifdef CONFIG_COMPAT32`` +may be required).
+When a given request takes a pointer to a struct, and that struct +contains types that differ in compat, it is normally represented as +``struct compat_<name>`` in the compat handler. It may happen that some +of the types used in this struct are only appropriate for compat32. The +preferred approach in this case is to change these types to appropriate +``compat_*`` types, for instance ``compat_long`` instead of ``s32``. +This holds even if the entire transformation is unnecessary in compat64; +this is so that the compat handler is kept as generic as possible.
+When the struct contains pointers, they must be represented as +``compat_uptr_t`` (preferred) or ``compat_caddr_t`` in its compat +counterpart, and conversions between compat user pointers and native +user pointers must always be made using ``compat_ptr()`` and +``ptr_to_compat()``.
+``__user`` annotation fixups +============================
+Most complex types involving ``__user`` fail to build in PCuABI, notably +double user pointers (user pointer to a user pointer). This is because +using ``__capability`` as a prefix to ``*`` only has the intended +meaning in a limited number of situations. Otherwise, the compiler will +typically throw the following error::
- error: use of __capability is ambiguous
+A fixup is then required, as described in section "PCuABI-specific +changes" of the `user pointer documentation`_. For instance::
- diff --git a/net/socket.c b/net/socket.c
- index 8597fbacb089..ab2a610825cc 100644
- --- a/net/socket.c
- +++ b/net/socket.c
- @@ -3156,7 +3169,11 @@ void socket_seq_show(struct seq_file *seq)
- the next page isn't readable/writable, we get a fault. To prevent
- that, copy back and forth to the full size.
- */
- +#ifdef CONFIG_CHERI_PURECAP_UABI
- +int get_user_ifreq(struct ifreq *ifr, void * __capability *ifrdata, void __user *arg)
- +#else
- int get_user_ifreq(struct ifreq *ifr, void __user **ifrdata, void __user *arg)
- +#endif
- {
if (in_compat_syscall()) {struct compat_ifreq *ifr32 = (struct compat_ifreq *)ifr;+Fortunately, ``__user`` is mostly used in simple types, and such fixups +are rarely needed in driver code.
What would be very helpful is examples of conversions. With various common cases you mentioned in the above tables. Maybe 2 or 3 examples of each "type" of problem as above and the resulting code changes would be very helpful.
On 08/08/2022 14:05, Carsten Haitzler wrote:
First - a good read. This needed writing no matter what and likely will need massaging over time as more drivers get attention.
Thanks for having a look! And indeed it's not meant to be fully formed to start with, I'm sure we'll find more things to put in it :)
Feedback as below in-line. :)
On 8/1/22 15:53, Kevin Brodsky wrote:
Add a small guide documenting the most common changes required to get drivers (and potentially other subsystems) compatible with the pure-capability user ABI (PCuABI).
There is no obvious subdirectory for this type of document, and it is transient in nature, so add it at the top-level.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com
Rendered version: https://git.morello-project.org/morello/kernel/linux/-/blob/review/kevin/por...
Documentation/pcuabi-porting.rst | 188 +++++++++++++++++++++++++++++++ 1 file changed, 188 insertions(+) create mode 100644 Documentation/pcuabi-porting.rst
diff --git a/Documentation/pcuabi-porting.rst b/Documentation/pcuabi-porting.rst new file mode 100644 index 000000000000..9c78523e39b7 --- /dev/null +++ b/Documentation/pcuabi-porting.rst @@ -0,0 +1,188 @@ +================================= +Adding PCuABI support to drivers +=================================
+This document provides a non-exhaustive overview of the most common +changes required to support the pure-capability user ABI (PCuABI) in +arbitrary drivers. It may also be helpful for core subsystems, though +note that more extensive changes may be required than for drivers with +straightforward interactions with userspace.
+.. _user pointer documentation: core-api/user_ptr.rst
+User pointer representation and conversions +===========================================
+The appropriate API to represent and convert user pointers is described +in the `user pointer documentation`_. A few examples of modifications +required for PCuABI compliance:
++--------------------------------+------------------------------------+ +| Invalid code | Potential replacement | ++================================+====================================+ +| ``(unsigned long)uptr`` | ``user_ptr_addr(uptr)`` | ++--------------------------------+------------------------------------+ +| ``(void __user *)u64`` | | ``uaddr_to_user_ptr(u64)`` | +| | | ``as_user_ptr(u64)`` | ++--------------------------------+------------------------------------+ +| ``get_user(uptr, &uarg->ptr)`` | ``get_user_ptr(uptr, &uarg->ptr)`` | ++--------------------------------+------------------------------------+ +| ``IS_ERR(ubuf)`` | ``USER_PTR_IS_ERR(ubuf)`` | ++--------------------------------+------------------------------------+
+``ioctl`` handlers' third argument +==================================
+Traditionally, the type of the third argument of ``ioctl`` handlers is +``unsigned long``. Unfortunately this is no longer appropriate in PCuABI +in many cases, as this argument may represent a pointer. A larger type, +able to hold a capability, is therefore necessary: this type is +``user_uintptr_t``.
+The prototype of most ``ioctl`` methods has been modified to take +``user_uintptr_t`` as third argument (``arg``), notably +``struct file_operations::unlocked_ioctl`` and +``struct block_device_operations::ioctl``. As a consequence, any driver +implementing such methods must change the prototype of the callback +accordingly, that is replace ``unsigned long`` with ``user_uintptr_t``, +as the prototypes must match exactly.
+Additionally, if the handler passes down ``arg`` to other functions, +such functions must also represent this argument as ``user_uintptr_t`` +**if they handle any request type where arg is a pointer** - in other +words, if they cast ``arg`` to a user pointer, e.g. ``void __user *``. +Otherwise, it is acceptable to leave these functions unchanged (i.e. +keep taking an ``unsigned long``).
+Compat ``ioctl`` handlers +=========================
+Third argument +--------------
+Unlike native ``ioctl`` handlers, the type of the third argument of +compat ``ioctl`` handlers remains ``unsigned long``, as this remains an +appropriate type to represent compat pointers in both compat32 and +compat64. As a result, a native handler can no longer be used as a compat +handler, because their prototypes differ. An appropriate compat handler +must therefore always be chosen.
I'm scratching my head above about compat32. I thought ... we had no 32bit compat? This is more regular aarch64/arm64 64bit compat vs purecap? My brain is going "shouldn't this be compa64 vs compat128?" (I know strictly its 129 bit as an extra cap bit is there too in memory).
I know we're USING the COMPAT32 ifdefs etc. really to mean 64bit non-purecap but it's confusing as above.
Alright maybe a recap then. Good idea to start reading this section [1] of the core Morello doc first, especially the table. There are two possible native ABIs: N1. PCuABI AArch64 (pointer = capability) N2. Standard AArch64 (pointer = 64-bit integer)
There are then two possible compat ABIs: C1. Standard AArch64 (pointer = 64-bit integer), "compat64" C2. AArch32 (pointer = 32-bit integer), "compat32"
You can't actually mix and match, N1 goes with C1 and N2 goes with C2. N2 and C1 are supposed to be exactly the same ABI. There's nothing tricky in the naming, 64 means 64-bit pointers and 32 means 32-bit pointers :) PCuABI is always a native ABI so there is no "compat128".
We do not actually support compat32, as there is no AArch32 on Morello, but to keep things clean we preserve its behaviour by using CONFIG_COMPAT{32,64} (and check that it still builds).
Hopefully that clarifies things!
[1] https://git.morello-project.org/morello/kernel/linux/-/blob/morello/master/D...
+Compat user pointers should always be converted to native user pointers +using ``compat_ptr()``. Unfortunately this is not always the case in +existing drivers, especially in compat ``ioctl`` handlers, which may +simply pass their arguments through to the native handler without using +the appropriate pointer conversion. This is complicated by the fact that +the actual type of the third argument (``arg``) may depend on the +request type, just like in native handlers. The subsections below cover +the various possible situations.
+``arg`` is always a pointer +~~~~~~~~~~~~~~~~~~~~~~~~~~~
+If ``arg`` always represents a pointer, it can be unconditionally +converted to a native pointer and passed to the native handler, for +instance::
+ static long my_compat_ioctl(struct file *file, unsigned int cmd, + unsigned long arg) + { + return my_ioctl(file, cmd, (user_uintptr_t)compat_ptr(arg)); + }
+For such a trivial ``file_operations`` compat handler, there is in fact +no need to create a compat handler; the ``compat_ptr_ioctl`` helper can +be used instead::
+ static const struct file_operations my_fops = { + ... + .compat_ioctl = compat_ptr_ioctl, + ... + };
+``arg`` is never a pointer +~~~~~~~~~~~~~~~~~~~~~~~~~~
+If ``arg`` never represents a pointer, it can directly be passed to +the native handler, optionally cast to ``user_uintptr_t``.
+Similarly, in this situation, the ``compat_noptr_ioctl`` helper can be +used instead of writing a trivial ``file_operations`` compat handler::
+ static const struct file_operations my_fops = { + ... + .compat_ioctl = compat_noptr_ioctl, + ... + };
+``arg`` is sometimes a pointer +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+If ``arg`` represents a pointer for certain request types but not +others, then the compat handler should ensure that the appropriate +conversion is made depending on the request type, i.e. ``compat_ptr()`` +is used **if and only if arg is a pointer for the specific request +type.**
+Struct layout / 32-bit assumptions +----------------------------------
Again ... 32bit. I'm scratching my head a bit? Not related to 32bit amrv7 and before?
As well, but not specifically. We're talking about generic code so this is about any 32-bit architecture really, and the point is that compat handlers often do 32-bit-specific things that we need to skip for compat64.
+Aside from the conversion of ``arg``, existing compat ``ioctl`` handlers +should be examined carefully as they typically include two different +types of transformations:
+1. Struct layout transformation, which may include pointer conversions. +2. 32-bit-specific transformations, e.g. for 32-bit time representation.
+The difficulty with supporting compat64 is that the first transformation +may still required, while the second transformation is not relevant and +should not be carried out in compat64 (some ``#ifdef CONFIG_COMPAT32`` +may be required).
+When a given request takes a pointer to a struct, and that struct +contains types that differ in compat, it is normally represented as +``struct compat_<name>`` in the compat handler. It may happen that some +of the types used in this struct are only appropriate for compat32. The +preferred approach in this case is to change these types to appropriate +``compat_*`` types, for instance ``compat_long`` instead of ``s32``. +This holds even if the entire transformation is unnecessary in compat64; +this is so that the compat handler is kept as generic as possible.
+When the struct contains pointers, they must be represented as +``compat_uptr_t`` (preferred) or ``compat_caddr_t`` in its compat +counterpart, and conversions between compat user pointers and native +user pointers must always be made using ``compat_ptr()`` and +``ptr_to_compat()``.
+``__user`` annotation fixups +============================
+Most complex types involving ``__user`` fail to build in PCuABI, notably +double user pointers (user pointer to a user pointer). This is because +using ``__capability`` as a prefix to ``*`` only has the intended +meaning in a limited number of situations. Otherwise, the compiler will +typically throw the following error::
+ error: use of __capability is ambiguous
+A fixup is then required, as described in section "PCuABI-specific +changes" of the `user pointer documentation`_. For instance::
+ diff --git a/net/socket.c b/net/socket.c + index 8597fbacb089..ab2a610825cc 100644 + --- a/net/socket.c + +++ b/net/socket.c + @@ -3156,7 +3169,11 @@ void socket_seq_show(struct seq_file *seq) + * the next page isn't readable/writable, we get a fault. To prevent + * that, copy back and forth to the full size. + */ + +#ifdef CONFIG_CHERI_PURECAP_UABI + +int get_user_ifreq(struct ifreq *ifr, void * __capability *ifrdata, void __user *arg) + +#else + int get_user_ifreq(struct ifreq *ifr, void __user **ifrdata, void __user *arg) + +#endif + { + if (in_compat_syscall()) { + struct compat_ifreq *ifr32 = (struct compat_ifreq *)ifr;
+Fortunately, ``__user`` is mostly used in simple types, and such fixups +are rarely needed in driver code.
What would be very helpful is examples of conversions. With various common cases you mentioned in the above tables. Maybe 2 or 3 examples of each "type" of problem as above and the resulting code changes would be very helpful.
Makes sense, 2-3 of each is quite a lot but I can try to do one of each at least :) It's not so easy to find examples that are relatively straightforward (without extra complications) but I should manage to find one of each.
Kevin
Hey Kevin :)
On 8/9/22 14:21, Kevin Brodsky wrote:
On 08/08/2022 14:05, Carsten Haitzler wrote:
First - a good read. This needed writing no matter what and likely will need massaging over time as more drivers get attention.
Thanks for having a look! And indeed it's not meant to be fully formed to start with, I'm sure we'll find more things to put in it :)
Feedback as below in-line. :)
On 8/1/22 15:53, Kevin Brodsky wrote:
Add a small guide documenting the most common changes required to get drivers (and potentially other subsystems) compatible with the pure-capability user ABI (PCuABI).
There is no obvious subdirectory for this type of document, and it is transient in nature, so add it at the top-level.
Signed-off-by: Kevin Brodsky kevin.brodsky@arm.com
Rendered version: https://git.morello-project.org/morello/kernel/linux/-/blob/review/kevin/por...
Documentation/pcuabi-porting.rst | 188 +++++++++++++++++++++++++++++++ 1 file changed, 188 insertions(+) create mode 100644 Documentation/pcuabi-porting.rst
diff --git a/Documentation/pcuabi-porting.rst b/Documentation/pcuabi-porting.rst new file mode 100644 index 000000000000..9c78523e39b7 --- /dev/null +++ b/Documentation/pcuabi-porting.rst @@ -0,0 +1,188 @@ +================================= +Adding PCuABI support to drivers +=================================
+This document provides a non-exhaustive overview of the most common +changes required to support the pure-capability user ABI (PCuABI) in +arbitrary drivers. It may also be helpful for core subsystems, though +note that more extensive changes may be required than for drivers with +straightforward interactions with userspace.
+.. _user pointer documentation: core-api/user_ptr.rst
+User pointer representation and conversions +===========================================
+The appropriate API to represent and convert user pointers is described +in the `user pointer documentation`_. A few examples of modifications +required for PCuABI compliance:
++--------------------------------+------------------------------------+ +| Invalid code | Potential replacement | ++================================+====================================+ +| ``(unsigned long)uptr`` | ``user_ptr_addr(uptr)`` | ++--------------------------------+------------------------------------+ +| ``(void __user *)u64`` | | ``uaddr_to_user_ptr(u64)`` | +| | | ``as_user_ptr(u64)`` | ++--------------------------------+------------------------------------+ +| ``get_user(uptr, &uarg->ptr)`` | ``get_user_ptr(uptr, &uarg->ptr)`` | ++--------------------------------+------------------------------------+ +| ``IS_ERR(ubuf)`` | ``USER_PTR_IS_ERR(ubuf)`` | ++--------------------------------+------------------------------------+
+``ioctl`` handlers' third argument +==================================
+Traditionally, the type of the third argument of ``ioctl`` handlers is +``unsigned long``. Unfortunately this is no longer appropriate in PCuABI +in many cases, as this argument may represent a pointer. A larger type, +able to hold a capability, is therefore necessary: this type is +``user_uintptr_t``.
+The prototype of most ``ioctl`` methods has been modified to take +``user_uintptr_t`` as third argument (``arg``), notably +``struct file_operations::unlocked_ioctl`` and +``struct block_device_operations::ioctl``. As a consequence, any driver +implementing such methods must change the prototype of the callback +accordingly, that is replace ``unsigned long`` with ``user_uintptr_t``, +as the prototypes must match exactly.
+Additionally, if the handler passes down ``arg`` to other functions, +such functions must also represent this argument as ``user_uintptr_t`` +**if they handle any request type where arg is a pointer** - in other +words, if they cast ``arg`` to a user pointer, e.g. ``void __user *``. +Otherwise, it is acceptable to leave these functions unchanged (i.e. +keep taking an ``unsigned long``).
+Compat ``ioctl`` handlers +=========================
+Third argument +--------------
+Unlike native ``ioctl`` handlers, the type of the third argument of +compat ``ioctl`` handlers remains ``unsigned long``, as this remains an +appropriate type to represent compat pointers in both compat32 and +compat64. As a result, a native handler can no longer be used as a compat +handler, because their prototypes differ. An appropriate compat handler +must therefore always be chosen.
I'm scratching my head above about compat32. I thought ... we had no 32bit compat? This is more regular aarch64/arm64 64bit compat vs purecap? My brain is going "shouldn't this be compa64 vs compat128?" (I know strictly its 129 bit as an extra cap bit is there too in memory).
I know we're USING the COMPAT32 ifdefs etc. really to mean 64bit non-purecap but it's confusing as above.
Alright maybe a recap then. Good idea to start reading this section [1] of the core Morello doc first, especially the table. There are two possible native ABIs: N1. PCuABI AArch64 (pointer = capability) N2. Standard AArch64 (pointer = 64-bit integer)
There are then two possible compat ABIs: C1. Standard AArch64 (pointer = 64-bit integer), "compat64" C2. AArch32 (pointer = 32-bit integer), "compat32"
You can't actually mix and match, N1 goes with C1 and N2 goes with C2. N2 and C1 are supposed to be exactly the same ABI. There's nothing tricky in the naming, 64 means 64-bit pointers and 32 means 32-bit pointers :) PCuABI is always a native ABI so there is no "compat128".
We do not actually support compat32, as there is no AArch32 on Morello, but to keep things clean we preserve its behaviour by using CONFIG_COMPAT{32,64} (and check that it still builds).
Hopefully that clarifies things!
That's what I thought - in purecap mode we don't have a compat32. The problem is talking about compat32 and it makes you go "eeeh what?" - assuming the person read up a bit on morello and was wondering what was going on.
So what I was bringing up here is that this seems to begin discussing something I thought we were not doing (32bit compat). I can see why you might want to keep the code building in theory. It made me scratch my head and wonder if I had got that right. Perhaps some clarification here would be good?
[1] https://git.morello-project.org/morello/kernel/linux/-/blob/morello/master/D...
+Compat user pointers should always be converted to native user pointers +using ``compat_ptr()``. Unfortunately this is not always the case in +existing drivers, especially in compat ``ioctl`` handlers, which may +simply pass their arguments through to the native handler without using +the appropriate pointer conversion. This is complicated by the fact that +the actual type of the third argument (``arg``) may depend on the +request type, just like in native handlers. The subsections below cover +the various possible situations.
+``arg`` is always a pointer +~~~~~~~~~~~~~~~~~~~~~~~~~~~
+If ``arg`` always represents a pointer, it can be unconditionally +converted to a native pointer and passed to the native handler, for +instance::
+ static long my_compat_ioctl(struct file *file, unsigned int cmd, + unsigned long arg) + { + return my_ioctl(file, cmd, (user_uintptr_t)compat_ptr(arg)); + }
+For such a trivial ``file_operations`` compat handler, there is in fact +no need to create a compat handler; the ``compat_ptr_ioctl`` helper can +be used instead::
+ static const struct file_operations my_fops = { + ... + .compat_ioctl = compat_ptr_ioctl, + ... + };
+``arg`` is never a pointer +~~~~~~~~~~~~~~~~~~~~~~~~~~
+If ``arg`` never represents a pointer, it can directly be passed to +the native handler, optionally cast to ``user_uintptr_t``.
+Similarly, in this situation, the ``compat_noptr_ioctl`` helper can be +used instead of writing a trivial ``file_operations`` compat handler::
+ static const struct file_operations my_fops = { + ... + .compat_ioctl = compat_noptr_ioctl, + ... + };
+``arg`` is sometimes a pointer +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+If ``arg`` represents a pointer for certain request types but not +others, then the compat handler should ensure that the appropriate +conversion is made depending on the request type, i.e. ``compat_ptr()`` +is used **if and only if arg is a pointer for the specific request +type.**
+Struct layout / 32-bit assumptions +----------------------------------
Again ... 32bit. I'm scratching my head a bit? Not related to 32bit amrv7 and before?
As well, but not specifically. We're talking about generic code so this is about any 32-bit architecture really, and the point is that compat handlers often do 32-bit-specific things that we need to skip for compat64.
Handled above :)
+Aside from the conversion of ``arg``, existing compat ``ioctl`` handlers +should be examined carefully as they typically include two different +types of transformations:
+1. Struct layout transformation, which may include pointer conversions. +2. 32-bit-specific transformations, e.g. for 32-bit time representation.
+The difficulty with supporting compat64 is that the first transformation +may still required, while the second transformation is not relevant and +should not be carried out in compat64 (some ``#ifdef CONFIG_COMPAT32`` +may be required).
+When a given request takes a pointer to a struct, and that struct +contains types that differ in compat, it is normally represented as +``struct compat_<name>`` in the compat handler. It may happen that some +of the types used in this struct are only appropriate for compat32. The +preferred approach in this case is to change these types to appropriate +``compat_*`` types, for instance ``compat_long`` instead of ``s32``. +This holds even if the entire transformation is unnecessary in compat64; +this is so that the compat handler is kept as generic as possible.
+When the struct contains pointers, they must be represented as +``compat_uptr_t`` (preferred) or ``compat_caddr_t`` in its compat +counterpart, and conversions between compat user pointers and native +user pointers must always be made using ``compat_ptr()`` and +``ptr_to_compat()``.
+``__user`` annotation fixups +============================
+Most complex types involving ``__user`` fail to build in PCuABI, notably +double user pointers (user pointer to a user pointer). This is because +using ``__capability`` as a prefix to ``*`` only has the intended +meaning in a limited number of situations. Otherwise, the compiler will +typically throw the following error::
+ error: use of __capability is ambiguous
+A fixup is then required, as described in section "PCuABI-specific +changes" of the `user pointer documentation`_. For instance::
+ diff --git a/net/socket.c b/net/socket.c + index 8597fbacb089..ab2a610825cc 100644 + --- a/net/socket.c + +++ b/net/socket.c + @@ -3156,7 +3169,11 @@ void socket_seq_show(struct seq_file *seq) + * the next page isn't readable/writable, we get a fault. To prevent + * that, copy back and forth to the full size. + */ + +#ifdef CONFIG_CHERI_PURECAP_UABI + +int get_user_ifreq(struct ifreq *ifr, void * __capability *ifrdata, void __user *arg) + +#else + int get_user_ifreq(struct ifreq *ifr, void __user **ifrdata, void __user *arg) + +#endif + { + if (in_compat_syscall()) { + struct compat_ifreq *ifr32 = (struct compat_ifreq *)ifr;
+Fortunately, ``__user`` is mostly used in simple types, and such fixups +are rarely needed in driver code.
What would be very helpful is examples of conversions. With various common cases you mentioned in the above tables. Maybe 2 or 3 examples of each "type" of problem as above and the resulting code changes would be very helpful.
Makes sense, 2-3 of each is quite a lot but I can try to do one of each at least :) It's not so easy to find examples that are relatively straightforward (without extra complications) but I should manage to find one of each.
One of each would be a big step already. :)
On 11/08/2022 11:28, Carsten Haitzler wrote:
I'm scratching my head above about compat32. I thought ... we had no 32bit compat? This is more regular aarch64/arm64 64bit compat vs purecap? My brain is going "shouldn't this be compa64 vs compat128?" (I know strictly its 129 bit as an extra cap bit is there too in memory).
I know we're USING the COMPAT32 ifdefs etc. really to mean 64bit non-purecap but it's confusing as above.
Alright maybe a recap then. Good idea to start reading this section [1] of the core Morello doc first, especially the table. There are two possible native ABIs: N1. PCuABI AArch64 (pointer = capability) N2. Standard AArch64 (pointer = 64-bit integer)
There are then two possible compat ABIs: C1. Standard AArch64 (pointer = 64-bit integer), "compat64" C2. AArch32 (pointer = 32-bit integer), "compat32"
You can't actually mix and match, N1 goes with C1 and N2 goes with C2. N2 and C1 are supposed to be exactly the same ABI. There's nothing tricky in the naming, 64 means 64-bit pointers and 32 means 32-bit pointers :) PCuABI is always a native ABI so there is no "compat128".
We do not actually support compat32, as there is no AArch32 on Morello, but to keep things clean we preserve its behaviour by using CONFIG_COMPAT{32,64} (and check that it still builds).
Hopefully that clarifies things!
That's what I thought - in purecap mode we don't have a compat32. The problem is talking about compat32 and it makes you go "eeeh what?" - assuming the person read up a bit on morello and was wondering what was going on.
So what I was bringing up here is that this seems to begin discussing something I thought we were not doing (32bit compat). I can see why you might want to keep the code building in theory. It made me scratch my head and wonder if I had got that right. Perhaps some clarification here would be good?
Yes you're right - my thinking when writing all this was that it's fine to assume a certain level of understanding of the whole approach, but since compat{32,64} are not really documented anywhere, I suppose this document is not a bad place to give some recap, probably at the top. Another improvement for v2, thanks :)
Kevin
linux-morello@op-lists.linaro.org
-
Carsten Haitzler -
Kevin Brodsky -
Zachary Leaf