On Mon, Nov 21, 2022 at 10:57:10AM +0000, Tudor Cretu wrote:

Hi Beata,

On 18-11-2022 00:05, Beata Michalska wrote:

...

Make copy_struct_from_user capability-aware by switching the actual copying routine to copy_to_user_with_ptr, one that can preserve capability tags throughout the process.

Just a short question: we're keeping both copy_from_user and copy_from_user_with_ptr because we don't want to preserve capability tags by default and each time this is done should be intentional, right? Is there anything specific to copy_struct_from_user that would make it fine to preserve capability tags by default? I think the alternative would be to have two versions: copy_struct_from_user and copy_struct_from_user_with_ptr so that we don't preserve tags by default.

The 'intentional' part here should be applied when copying to userspace, not the other way round, with the stress point being on not providing valid capabilities to user space when not indented, unless I am missing smth (?)

--- BR B.

Thanks, Tudor

...

Signed-off-by: Beata Michalska beata.michalska@arm.com

include/linux/uaccess.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h index 88b2224e85c3..ec31478634cc 100644 --- a/include/linux/uaccess.h +++ b/include/linux/uaccess.h @@ -419,7 +419,7 @@ copy_struct_from_user(void *dst, size_t ksize, const void __user *src, return ret ?: -E2BIG; } /* Copy the interoperable parts of the struct. */

• if (copy_from_user(dst, src, size))

• if (copy_from_user_with_ptr(dst, src, size)) return -EFAULT; return 0; }

On 21/11/2022 12:08, Tudor Cretu wrote:

On 21-11-2022 11:40, Beata Michalska wrote:

...

On Mon, Nov 21, 2022 at 10:57:10AM +0000, Tudor Cretu wrote:

...

Hi Beata,

On 18-11-2022 00:05, Beata Michalska wrote:

...

Make copy_struct_from_user capability-aware by switching the actual copying routine to copy_to_user_with_ptr, one that can preserve capability tags throughout the process.

Just a short question: we're keeping both copy_from_user and copy_from_user_with_ptr because we don't want to preserve capability tags by default and each time this is done should be intentional, right? Is there anything specific to copy_struct_from_user that would make it fine to preserve capability tags by default? I think the alternative would be to have two versions: copy_struct_from_user and copy_struct_from_user_with_ptr so that we don't preserve tags by default.

The 'intentional' part here should be applied when copying to userspace, not the other way round, with the stress point being on not providing valid capabilities to user space when not indented, unless I am missing smth (?)

Alright, I guess because there isn't a copy_struct_to_user as well, then we can just do your change without creating confusion. LGMT, thanks for the clarification!

Right, just so it's clear in my mind - if we do copy_*from*_user without _with_ptr here, then we do not preserve cap tags, that means the kernel would not be able to access or otherwise use that pointer?

I can't think of a scenario where you'd pass a pointer into to kernel but want that pointer unreadable or otherwise unusable.

The copy_*to*_user is the bit where care is needed.

Thanks, Zach

Thanks, Tudor

...

---

BR B.

...

Thanks, Tudor

...

Signed-off-by: Beata Michalska beata.michalska@arm.com

include/linux/uaccess.h | 2 +-    1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h index 88b2224e85c3..ec31478634cc 100644 --- a/include/linux/uaccess.h +++ b/include/linux/uaccess.h @@ -419,7 +419,7 @@ copy_struct_from_user(void *dst, size_t ksize, const void __user *src,                return ret ?: -E2BIG;        }        /* Copy the interoperable parts of the struct. */ -    if (copy_from_user(dst, src, size)) +    if (copy_from_user_with_ptr(dst, src, size))            return -EFAULT;        return 0;    }

---

linux-morello mailing list -- linux-morello@op-lists.linaro.org To unsubscribe send an email to linux-morello-leave@op-lists.linaro.org

On Fri, Nov 25, 2022 at 12:13:26PM +0000, Zachary Leaf wrote:

On 21/11/2022 12:08, Tudor Cretu wrote:

...

On 21-11-2022 11:40, Beata Michalska wrote:

...

On Mon, Nov 21, 2022 at 10:57:10AM +0000, Tudor Cretu wrote:

...

Hi Beata,

On 18-11-2022 00:05, Beata Michalska wrote:

...

Make copy_struct_from_user capability-aware by switching the actual copying routine to copy_to_user_with_ptr, one that can preserve capability tags throughout the process.

Just a short question: we're keeping both copy_from_user and copy_from_user_with_ptr because we don't want to preserve capability tags by default and each time this is done should be intentional, right? Is there anything specific to copy_struct_from_user that would make it fine to preserve capability tags by default? I think the alternative would be to have two versions: copy_struct_from_user and copy_struct_from_user_with_ptr so that we don't preserve tags by default.

The 'intentional' part here should be applied when copying to userspace, not the other way round, with the stress point being on not providing valid capabilities to user space when not indented, unless I am missing smth (?)

Alright, I guess because there isn't a copy_struct_to_user as well, then we can just do your change without creating confusion. LGMT, thanks for the clarification!

Right, just so it's clear in my mind - if we do copy_*from*_user without _with_ptr here, then we do not preserve cap tags, that means the kernel would not be able to access or otherwise use that pointer?

Yes, regular memcpy routines are not aware of capability registers nor the alignment requirements, so the capability tags (if any) will be lost on loads from the memory. The '_with_ptr' variants use capability registers and take care of the access alignment so the tags can be carried over through loads and stores. As a result the capabilities copied over with those routines are fully preserved. As long as valid capability has been provided (where it was expected), that capability can be used by the kernel to access given memory area. If the regular variants of memcpy were to be used, any memory access through such capability would result in capability tag fault.

I can't think of a scenario where you'd pass a pointer into to kernel but want that pointer unreadable or otherwise unusable.

Capabilities can be used to protect non-pointer types (there are software-defined bits in the permissions set that can be used for that purpose). though we are not dealing with those, at least not at this point. That said, such capabilities should not turn up where actual pointers are expected, not on purpose that is. What led you to thinking about invalid capability passed as a pointer? Not sure I caught your thought here.

--- BR B.

The copy_*to*_user is the bit where care is needed.

Thanks, Zach

...

Thanks, Tudor

...

---

BR B.

...

Thanks, Tudor

...

Signed-off-by: Beata Michalska beata.michalska@arm.com

include/linux/uaccess.h | 2 +-    1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/uaccess.h b/include/linux/uaccess.h index 88b2224e85c3..ec31478634cc 100644 --- a/include/linux/uaccess.h +++ b/include/linux/uaccess.h @@ -419,7 +419,7 @@ copy_struct_from_user(void *dst, size_t ksize, const void __user *src,                return ret ?: -E2BIG;        }        /* Copy the interoperable parts of the struct. */ -    if (copy_from_user(dst, src, size)) +    if (copy_from_user_with_ptr(dst, src, size))            return -EFAULT;        return 0;    }

---

linux-morello mailing list -- linux-morello@op-lists.linaro.org To unsubscribe send an email to linux-morello-leave@op-lists.linaro.org

---

linux-morello mailing list -- linux-morello@op-lists.linaro.org To unsubscribe send an email to linux-morello-leave@op-lists.linaro.org

On Fri, Nov 25, 2022 at 12:13:38PM +0000, Zachary Leaf wrote:

On 18/11/2022 00:05, Beata Michalska wrote:

...

The clone3 syscall provides a superset of the functionality compared to its clone forerunner, encapsulating all the various arguments within a single, dedicated clone_args structure, with some of its members representing user pointers. Currently those are specified as being of __u64 type (alignment aside) which is problematic for some architectures, where that type cannot actually hold said pointers. In order to add support for the latter, the relevant clone_args struct members get redefined with the appropriate __kernel_uintptr_t type. This though, brings its own drawbacks. The structure itself gains revised signature with explicit paddings for cases where sizeof(__kernel_uintptr_t) > sizeof(__u64), which results in noticeable growth in the structure's size. Furthermore, for affected architectures, as a consequence clone3 is no longer compat mode agnostic, and as such, requires repurposing the main syscall handler at a cost of marginal overhead though in favour of avoiding code duplication.

Signed-off-by: Beata Michalska beata.michalska@arm.com

include/uapi/linux/sched.h | 36 ++++++++--- kernel/fork.c | 123 +++++++++++++++++++++++++++++-------- 2 files changed, 122 insertions(+), 37 deletions(-)

diff --git a/include/uapi/linux/sched.h b/include/uapi/linux/sched.h index 3bac0a8ceab2..56ca73ca763c 100644 --- a/include/uapi/linux/sched.h +++ b/include/uapi/linux/sched.h @@ -87,26 +87,42 @@

• The structure is versioned by size and thus extensible.
• New struct members must go at the end of the struct and

• • must be properly 64bit aligned.

• • must be properly aligned on at least 64bit boundary.
• • See below on explicit padding when:
• • ((__alignof__(__kernel_uintptr_t) > __alignof__(__u64))

I think this would still work without the explicit padding, right? What's the reason for adding it explicitly? This is related to the issue of struct holes at https://git.morello-project.org/morello/kernel/linux/-/issues/38

It is somewhat related, though does not suffer from the risk of potentially leaking data as, in this case, we are moving memory only in one direction: from user-space into kernel space. And yes, it would work without explicit padding - the only thing that is happening here is making the padding more visible.

Seems to me it adds some extra complexity as Kristina mentioned in above issue, but might be missing something here.

It's mainly visualizing the padding - there is no additional complexity here. And as this struct is extendable with size versioning it might actually be convenient when specifying the size of future extensions and verifying struct's layout. Though if you believe it is still too 'disruptive' I can remove those.

...

• */

+#define __rsvd(tag) \

• __u8 __rsvd_##tag[sizeof(__kernel_uintptr_t) - sizeof(__u64)]

struct clone_args { __aligned_u64 flags;

• __aligned_u64 pidfd;
• __aligned_u64 child_tid;
• __aligned_u64 parent_tid;

• __rsvd(v0_1);
• __kernel_aligned_uintptr_t pidfd;
• __kernel_aligned_uintptr_t child_tid;
• __kernel_aligned_uintptr_t parent_tid; __aligned_u64 exit_signal;

• __aligned_u64 stack;

• __rsvd(v0_2);
• __kernel_aligned_uintptr_t stack; __aligned_u64 stack_size;

• __aligned_u64 tls;
• __aligned_u64 set_tid;

• __rsvd(v0_3);
• __kernel_aligned_uintptr_t tls;
• /* VER0 boundary */
• __kernel_aligned_uintptr_t set_tid; __aligned_u64 set_tid_size;
• /* VER1 boundary */ __aligned_u64 cgroup;
• /* VER2 boundary */

}; #endif -#define CLONE_ARGS_SIZE_VER0 64 /* sizeof first published struct */ -#define CLONE_ARGS_SIZE_VER1 80 /* sizeof second published struct */ -#define CLONE_ARGS_SIZE_VER2 88 /* sizeof third published struct */ +#define CLONE_ARGS_SIZE_VER0 /* sizeof first published struct */ \

• ((__alignof__(__kernel_uintptr_t) > __alignof__(__u64)) ? 128 : 64)

+#define CLONE_ARGS_SIZE_VER1 /* sizeof second published struct */ \

• ((__alignof__(__kernel_uintptr_t) > __alignof__(__u64)) ? 152 : 80)

+#define CLONE_ARGS_SIZE_VER2 /* sizeof third published struct */ \

• ((__alignof__(__kernel_uintptr_t) > __alignof__(__u64)) ? 160 : 88)

This is just hardcoding in the struct sizes which is fine, but just as a

That's how this struct is being versioned.

general question, would it ever be possible for __alignof__(__kernel_uintptr_t) to be different or more than 16?

If __kernel_uintptr_t is 64bits then yes, the alignment will be on 8-byte boundaries. Additionally, as __kernel_uintptr_t is defined as __u64 for !PCuABI it can be aligned on 4-byte boundaries for some 32-bit platforms, which is why this struct is forcing the alignment on it's members to be (now at least) 8 bytes.

Seems like what we've got here is just a proxy for: ((__alignof__(__kernel_uintptr_t) == 16) ? 160 : 88)

Yes, although I do prefer the verbosity here. Do you believe this is not readable and should be changed ?

--- BR B.

Thanks, Zach

...

/*

• Scheduling policies

diff --git a/kernel/fork.c b/kernel/fork.c index 181771571599..72de1187a586 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -2759,67 +2759,136 @@ SYSCALL_DEFINE5(clone, unsigned long, clone_flags, user_uintptr_t, newsp, #ifdef __ARCH_WANT_SYS_CLONE3 +#define __clone_args_size_ver(ver, pfx) \

• pfx##CLONE_ARGS_SIZE_VER##ver

+#ifdef CONFIG_COMPAT64 +struct compat_clone_args {

• __aligned_u64 flags;
• __aligned_u64 pidfd;
• __aligned_u64 child_tid;
• __aligned_u64 parent_tid;
• __aligned_u64 exit_signal;
• __aligned_u64 stack;
• __aligned_u64 stack_size;
• __aligned_u64 tls;
• __aligned_u64 set_tid;
• __aligned_u64 set_tid_size;
• __aligned_u64 cgroup;

+};

+#define COMPAT_CLONE_ARGS_SIZE_VER0 64 /* sizeof first published struct */ +#define COMPAT_CLONE_ARGS_SIZE_VER1 80 /* sizeof second published struct */ +#define COMPAT_CLONE_ARGS_SIZE_VER2 88 /* sizeof third published struct */

+#define clone_args_size_ver(ver) \

• (in_compat_syscall() ? __clone_args_size_ver(ver, COMPAT_) \

• 	     : __clone_args_size_ver(ver, ))
  

+#define clone_args_get(args, member) \

• (in_compat_syscall() ? (args)->__compat_args.member \

• 	     : (args)->__args.member)
  

+#define clone_args_as_user_ptr(args, member) \

• (in_compat_syscall() ? compat_ptr(clone_args_get(args, member)) \

• 	     : as_user_ptr(clone_args_get(args, member)))
  

+#else /* CONFIG_COMPAT64 */ +#define clone_args_size_ver(ver __clone_args_size_ver(ver, ) +#define clone_args_get(args, member) ((args)->member) +#define clone_args_as_user_ptr(args, member) \

• as_user_ptr(clone_args_get(args, member))

+#endif /* CONFIG_COMPAT64 */

+static inline void clone_args_validate_static(void) +{ +#define CLONE_ARGS_BUILD_BUG_ON(type, pfx) \ +do { \

• BUILD_BUG_ON(offsetofend(type, tls) != pfx##_SIZE_VER0); \
• BUILD_BUG_ON(offsetofend(type, set_tid_size) != pfx##_SIZE_VER1);\
• BUILD_BUG_ON(offsetofend(type, cgroup) != pfx##_SIZE_VER2); \
• BUILD_BUG_ON(sizeof(type) != pfx##_SIZE_VER2); \

+} while (0)

• CLONE_ARGS_BUILD_BUG_ON(struct clone_args, CLONE_ARGS);

+#ifdef CONFIG_COMPAT64

• CLONE_ARGS_BUILD_BUG_ON(struct compat_clone_args, COMPAT_CLONE_ARGS);
• BUILD_BUG_ON(sizeof(struct clone_args) < sizeof(struct compat_clone_args));

+#endif +}

noinline static int copy_clone_args_from_user(struct kernel_clone_args *kargs, struct clone_args __user *uargs, size_t usize) { int err; +#ifdef CONFIG_COMPAT64

• union {

• struct clone_args	 __args;
  

• struct compat_clone_args __compat_args;
  

• } args;

+#else struct clone_args args; +#endif pid_t *kset_tid = kargs->set_tid;

• BUILD_BUG_ON(offsetofend(struct clone_args, tls) !=

•      CLONE_ARGS_SIZE_VER0);
  

• BUILD_BUG_ON(offsetofend(struct clone_args, set_tid_size) !=

•      CLONE_ARGS_SIZE_VER1);
  

• BUILD_BUG_ON(offsetofend(struct clone_args, cgroup) !=

•      CLONE_ARGS_SIZE_VER2);
  

• BUILD_BUG_ON(sizeof(struct clone_args) != CLONE_ARGS_SIZE_VER2);

• clone_args_validate_static();

if (unlikely(usize > PAGE_SIZE)) return -E2BIG;

• if (unlikely(usize < CLONE_ARGS_SIZE_VER0))

• if (unlikely(usize < clone_args_size_ver(0))) return -EINVAL;

+#ifdef CONFIG_COMPAT64

• err = copy_struct_from_user(&args, (in_compat_syscall() ?

• 		    sizeof(struct compat_clone_args) :
  

• 		    sizeof(args)),> +				    uargs, usize);
  

+#else err = copy_struct_from_user(&args, sizeof(args), uargs, usize); +#endif

• if (err) return err;

• if (unlikely(args.set_tid_size > MAX_PID_NS_LEVEL))

• if (unlikely(clone_args_get(&args, set_tid_size) > MAX_PID_NS_LEVEL)) return -EINVAL;

• if (unlikely(!args.set_tid && args.set_tid_size > 0))

• if (unlikely(!clone_args_get(&args, set_tid) &&

•      clone_args_get(&args, set_tid_size) > 0))
  

  return -EINVAL;

• if (unlikely(args.set_tid && args.set_tid_size == 0))

• if (unlikely(clone_args_get(&args, set_tid) &&

•      clone_args_get(&args, set_tid_size) == 0))
  

  return -EINVAL;

/* * Verify that higher 32bits of exit_signal are unset and that * it is a valid signal */

• if (unlikely((args.exit_signal & ~((u64)CSIGNAL)) ||

•      !valid_signal(args.exit_signal)))
  

• if (unlikely((clone_args_get(&args, exit_signal) & ~((u64)CSIGNAL)) ||

•      !valid_signal(clone_args_get(&args, exit_signal))))
  

  return -EINVAL;

• if ((args.flags & CLONE_INTO_CGROUP) &&

•    (args.cgroup > INT_MAX || usize < CLONE_ARGS_SIZE_VER2))
  

• if ((clone_args_get(&args, flags) & CLONE_INTO_CGROUP) &&

•    (clone_args_get(&args, cgroup) > INT_MAX ||
  

•     usize < clone_args_size_ver(2)))
  

  return -EINVAL;

*kargs = (struct kernel_clone_args){

• .flags		= args.flags,
  

• .pidfd		= u64_to_user_ptr(args.pidfd),
  

• .child_tid	= u64_to_user_ptr(args.child_tid),
  

• .parent_tid	= u64_to_user_ptr(args.parent_tid),
  

• .exit_signal	= args.exit_signal,
  

• .stack		= args.stack,
  

• .stack_size	= args.stack_size,
  

• .tls		= args.tls,
  

• .set_tid_size	= args.set_tid_size,
  

• .cgroup		= args.cgroup,
  

• .flags		= clone_args_get(&args, flags),
  

• .pidfd		= clone_args_as_user_ptr(&args, pidfd),
  

• .child_tid	= clone_args_as_user_ptr(&args, child_tid),
  

• .parent_tid	= clone_args_as_user_ptr(&args, parent_tid),
  

• .exit_signal	= clone_args_get(&args, exit_signal),
  

• .stack		= clone_args_get(&args, stack),
  

• .stack_size	= clone_args_get(&args, stack_size),
  

• .tls		= clone_args_get(&args, tls),
  

• .set_tid_size	= clone_args_get(&args, set_tid_size),
  

• .cgroup		= clone_args_get(&args, cgroup),
  

  };

• if (args.set_tid &&

• copy_from_user(kset_tid, u64_to_user_ptr(args.set_tid),
  

• if (clone_args_get(&args, set_tid) &&

• copy_from_user(kset_tid, clone_args_as_user_ptr(&args, set_tid),
  (kargs->set_tid_size * sizeof(pid_t))))
  

  return -EFAULT;

---

linux-morello mailing list -- linux-morello@op-lists.linaro.org To unsubscribe send an email to linux-morello-leave@op-lists.linaro.org

On 30/11/2022 20:26, Beata Michalska wrote:

On Fri, Nov 25, 2022 at 12:13:38PM +0000, Zachary Leaf wrote:

...

On 18/11/2022 00:05, Beata Michalska wrote:

...

The clone3 syscall provides a superset of the functionality compared to its clone forerunner, encapsulating all the various arguments within a single, dedicated clone_args structure, with some of its members representing user pointers. Currently those are specified as being of __u64 type (alignment aside) which is problematic for some architectures, where that type cannot actually hold said pointers. In order to add support for the latter, the relevant clone_args struct members get redefined with the appropriate __kernel_uintptr_t type. This though, brings its own drawbacks. The structure itself gains revised signature with explicit paddings for cases where sizeof(__kernel_uintptr_t) > sizeof(__u64), which results in noticeable growth in the structure's size. Furthermore, for affected architectures, as a consequence clone3 is no longer compat mode agnostic, and as such, requires repurposing the main syscall handler at a cost of marginal overhead though in favour of avoiding code duplication.

Signed-off-by: Beata Michalska beata.michalska@arm.com

include/uapi/linux/sched.h | 36 ++++++++--- kernel/fork.c | 123 +++++++++++++++++++++++++++++-------- 2 files changed, 122 insertions(+), 37 deletions(-)

diff --git a/include/uapi/linux/sched.h b/include/uapi/linux/sched.h index 3bac0a8ceab2..56ca73ca763c 100644 --- a/include/uapi/linux/sched.h +++ b/include/uapi/linux/sched.h @@ -87,26 +87,42 @@

• The structure is versioned by size and thus extensible.
• New struct members must go at the end of the struct and

• • must be properly 64bit aligned.

• • must be properly aligned on at least 64bit boundary.
• • See below on explicit padding when:
• • ((__alignof__(__kernel_uintptr_t) > __alignof__(__u64))

I think this would still work without the explicit padding, right? What's the reason for adding it explicitly? This is related to the issue of struct holes at https://git.morello-project.org/morello/kernel/linux/-/issues/38

It is somewhat related, though does not suffer from the risk of potentially leaking data as, in this case, we are moving memory only in one direction: from user-space into kernel space. And yes, it would work without explicit padding - the only thing that is happening here is making the padding more visible.

...

Seems to me it adds some extra complexity as Kristina mentioned in above issue, but might be missing something here.

It's mainly visualizing the padding - there is no additional complexity here. And as this struct is extendable with size versioning it might actually be convenient when specifying the size of future extensions and verifying struct's layout. Though if you believe it is still too 'disruptive' I can remove those.

...

...

• */

+#define __rsvd(tag) \

• __u8 __rsvd_##tag[sizeof(__kernel_uintptr_t) - sizeof(__u64)]

struct clone_args { __aligned_u64 flags;

• __aligned_u64 pidfd;
• __aligned_u64 child_tid;
• __aligned_u64 parent_tid;

• __rsvd(v0_1);
• __kernel_aligned_uintptr_t pidfd;
• __kernel_aligned_uintptr_t child_tid;
• __kernel_aligned_uintptr_t parent_tid; __aligned_u64 exit_signal;

• __aligned_u64 stack;

• __rsvd(v0_2);
• __kernel_aligned_uintptr_t stack; __aligned_u64 stack_size;

• __aligned_u64 tls;
• __aligned_u64 set_tid;

• __rsvd(v0_3);
• __kernel_aligned_uintptr_t tls;
• /* VER0 boundary */
• __kernel_aligned_uintptr_t set_tid; __aligned_u64 set_tid_size;
• /* VER1 boundary */ __aligned_u64 cgroup;
• /* VER2 boundary */

}; #endif -#define CLONE_ARGS_SIZE_VER0 64 /* sizeof first published struct */ -#define CLONE_ARGS_SIZE_VER1 80 /* sizeof second published struct */ -#define CLONE_ARGS_SIZE_VER2 88 /* sizeof third published struct */ +#define CLONE_ARGS_SIZE_VER0 /* sizeof first published struct */ \

• ((__alignof__(__kernel_uintptr_t) > __alignof__(__u64)) ? 128 : 64)

+#define CLONE_ARGS_SIZE_VER1 /* sizeof second published struct */ \

• ((__alignof__(__kernel_uintptr_t) > __alignof__(__u64)) ? 152 : 80)

+#define CLONE_ARGS_SIZE_VER2 /* sizeof third published struct */ \

• ((__alignof__(__kernel_uintptr_t) > __alignof__(__u64)) ? 160 : 88)

This is just hardcoding in the struct sizes which is fine, but just as a

That's how this struct is being versioned.

...

general question, would it ever be possible for __alignof__(__kernel_uintptr_t) to be different or more than 16?

If __kernel_uintptr_t is 64bits then yes, the alignment will be on 8-byte boundaries. Additionally, as __kernel_uintptr_t is defined as __u64 for !PCuABI it can be aligned on 4-byte boundaries for some 32-bit platforms, which is why this struct is forcing the alignment on it's members to be (now at least) 8 bytes.

...

Seems like what we've got here is just a proxy for: ((__alignof__(__kernel_uintptr_t) == 16) ? 160 : 88)

Yes, although I do prefer the verbosity here. Do you believe this is not readable and should be changed ?

In hindsight I think it's fine.

My initial concern/question here was if ever in future we might have have __kernel_uintptr_t being 32B/256-bit or something like that. Then it might be better to be explicit about what values of alignof(x) relate to what struct sizes instead of specifying it just as a relation of x > y.

In that case we've got more than 2 options and what I suggested wouldn't work either anyway and would need additional cases.

Thanks, Zach

---

BR B.

...

Thanks, Zach

...

/*

• Scheduling policies

diff --git a/kernel/fork.c b/kernel/fork.c index 181771571599..72de1187a586 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -2759,67 +2759,136 @@ SYSCALL_DEFINE5(clone, unsigned long, clone_flags, user_uintptr_t, newsp, #ifdef __ARCH_WANT_SYS_CLONE3 +#define __clone_args_size_ver(ver, pfx) \

• pfx##CLONE_ARGS_SIZE_VER##ver

+#ifdef CONFIG_COMPAT64 +struct compat_clone_args {

• __aligned_u64 flags;
• __aligned_u64 pidfd;
• __aligned_u64 child_tid;
• __aligned_u64 parent_tid;
• __aligned_u64 exit_signal;
• __aligned_u64 stack;
• __aligned_u64 stack_size;
• __aligned_u64 tls;
• __aligned_u64 set_tid;
• __aligned_u64 set_tid_size;
• __aligned_u64 cgroup;

+};

+#define COMPAT_CLONE_ARGS_SIZE_VER0 64 /* sizeof first published struct */ +#define COMPAT_CLONE_ARGS_SIZE_VER1 80 /* sizeof second published struct */ +#define COMPAT_CLONE_ARGS_SIZE_VER2 88 /* sizeof third published struct */

+#define clone_args_size_ver(ver) \

• (in_compat_syscall() ? __clone_args_size_ver(ver, COMPAT_) \

• 	     : __clone_args_size_ver(ver, ))
  

+#define clone_args_get(args, member) \

• (in_compat_syscall() ? (args)->__compat_args.member \

• 	     : (args)->__args.member)
  

+#define clone_args_as_user_ptr(args, member) \

• (in_compat_syscall() ? compat_ptr(clone_args_get(args, member)) \

• 	     : as_user_ptr(clone_args_get(args, member)))
  

+#else /* CONFIG_COMPAT64 */ +#define clone_args_size_ver(ver __clone_args_size_ver(ver, ) +#define clone_args_get(args, member) ((args)->member) +#define clone_args_as_user_ptr(args, member) \

• as_user_ptr(clone_args_get(args, member))

+#endif /* CONFIG_COMPAT64 */

+static inline void clone_args_validate_static(void) +{ +#define CLONE_ARGS_BUILD_BUG_ON(type, pfx) \ +do { \

• BUILD_BUG_ON(offsetofend(type, tls) != pfx##_SIZE_VER0); \
• BUILD_BUG_ON(offsetofend(type, set_tid_size) != pfx##_SIZE_VER1);\
• BUILD_BUG_ON(offsetofend(type, cgroup) != pfx##_SIZE_VER2); \
• BUILD_BUG_ON(sizeof(type) != pfx##_SIZE_VER2); \

+} while (0)

• CLONE_ARGS_BUILD_BUG_ON(struct clone_args, CLONE_ARGS);

+#ifdef CONFIG_COMPAT64

• CLONE_ARGS_BUILD_BUG_ON(struct compat_clone_args, COMPAT_CLONE_ARGS);
• BUILD_BUG_ON(sizeof(struct clone_args) < sizeof(struct compat_clone_args));

+#endif +}

noinline static int copy_clone_args_from_user(struct kernel_clone_args *kargs, struct clone_args __user *uargs, size_t usize) { int err; +#ifdef CONFIG_COMPAT64

• union {

• struct clone_args	 __args;
  

• struct compat_clone_args __compat_args;
  

• } args;

+#else struct clone_args args; +#endif pid_t *kset_tid = kargs->set_tid;

• BUILD_BUG_ON(offsetofend(struct clone_args, tls) !=

•      CLONE_ARGS_SIZE_VER0);
  

• BUILD_BUG_ON(offsetofend(struct clone_args, set_tid_size) !=

•      CLONE_ARGS_SIZE_VER1);
  

• BUILD_BUG_ON(offsetofend(struct clone_args, cgroup) !=

•      CLONE_ARGS_SIZE_VER2);
  

• BUILD_BUG_ON(sizeof(struct clone_args) != CLONE_ARGS_SIZE_VER2);

• clone_args_validate_static();

if (unlikely(usize > PAGE_SIZE)) return -E2BIG;

• if (unlikely(usize < CLONE_ARGS_SIZE_VER0))

• if (unlikely(usize < clone_args_size_ver(0))) return -EINVAL;

+#ifdef CONFIG_COMPAT64

• err = copy_struct_from_user(&args, (in_compat_syscall() ?

• 		    sizeof(struct compat_clone_args) :
  

• 		    sizeof(args)),> +				    uargs, usize);
  

+#else err = copy_struct_from_user(&args, sizeof(args), uargs, usize); +#endif

• if (err) return err;

• if (unlikely(args.set_tid_size > MAX_PID_NS_LEVEL))

• if (unlikely(clone_args_get(&args, set_tid_size) > MAX_PID_NS_LEVEL)) return -EINVAL;

• if (unlikely(!args.set_tid && args.set_tid_size > 0))

• if (unlikely(!clone_args_get(&args, set_tid) &&

•      clone_args_get(&args, set_tid_size) > 0))
  

  return -EINVAL;

• if (unlikely(args.set_tid && args.set_tid_size == 0))

• if (unlikely(clone_args_get(&args, set_tid) &&

•      clone_args_get(&args, set_tid_size) == 0))
  

  return -EINVAL;

/* * Verify that higher 32bits of exit_signal are unset and that * it is a valid signal */

• if (unlikely((args.exit_signal & ~((u64)CSIGNAL)) ||

•      !valid_signal(args.exit_signal)))
  

• if (unlikely((clone_args_get(&args, exit_signal) & ~((u64)CSIGNAL)) ||

•      !valid_signal(clone_args_get(&args, exit_signal))))
  

  return -EINVAL;

• if ((args.flags & CLONE_INTO_CGROUP) &&

•    (args.cgroup > INT_MAX || usize < CLONE_ARGS_SIZE_VER2))
  

• if ((clone_args_get(&args, flags) & CLONE_INTO_CGROUP) &&

•    (clone_args_get(&args, cgroup) > INT_MAX ||
  

•     usize < clone_args_size_ver(2)))
  

  return -EINVAL;

*kargs = (struct kernel_clone_args){

• .flags		= args.flags,
  

• .pidfd		= u64_to_user_ptr(args.pidfd),
  

• .child_tid	= u64_to_user_ptr(args.child_tid),
  

• .parent_tid	= u64_to_user_ptr(args.parent_tid),
  

• .exit_signal	= args.exit_signal,
  

• .stack		= args.stack,
  

• .stack_size	= args.stack_size,
  

• .tls		= args.tls,
  

• .set_tid_size	= args.set_tid_size,
  

• .cgroup		= args.cgroup,
  

• .flags		= clone_args_get(&args, flags),
  

• .pidfd		= clone_args_as_user_ptr(&args, pidfd),
  

• .child_tid	= clone_args_as_user_ptr(&args, child_tid),
  

• .parent_tid	= clone_args_as_user_ptr(&args, parent_tid),
  

• .exit_signal	= clone_args_get(&args, exit_signal),
  

• .stack		= clone_args_get(&args, stack),
  

• .stack_size	= clone_args_get(&args, stack_size),
  

• .tls		= clone_args_get(&args, tls),
  

• .set_tid_size	= clone_args_get(&args, set_tid_size),
  

• .cgroup		= clone_args_get(&args, cgroup),
  

  };

• if (args.set_tid &&

• copy_from_user(kset_tid, u64_to_user_ptr(args.set_tid),
  

• if (clone_args_get(&args, set_tid) &&

• copy_from_user(kset_tid, clone_args_as_user_ptr(&args, set_tid),
  (kargs->set_tid_size * sizeof(pid_t))))
  

  return -EFAULT;

---

linux-morello mailing list -- linux-morello@op-lists.linaro.org To unsubscribe send an email to linux-morello-leave@op-lists.linaro.org

On 18/11/2022 01:05, Beata Michalska wrote:

The clone3 syscall provides a superset of the functionality compared to its clone forerunner, encapsulating all the various arguments within a single, dedicated clone_args structure, with some of its members representing user pointers. Currently those are specified as being of __u64 type (alignment aside) which is problematic for some architectures, where that type cannot actually hold said pointers. In order to add support for the latter, the relevant clone_args struct members get redefined with the appropriate __kernel_uintptr_t type. This though, brings its own drawbacks. The structure itself gains revised signature with explicit paddings for cases where sizeof(__kernel_uintptr_t) > sizeof(__u64), which results in noticeable growth in the structure's size. Furthermore, for affected architectures, as a consequence clone3 is no longer compat mode agnostic, and as such, requires repurposing the main syscall handler at a cost of marginal overhead though in favour of avoiding code duplication.

Might want to break that down into a couple of paragraphs to give the avid reader some space to breathe :)

Signed-off-by: Beata Michalska beata.michalska@arm.com

include/uapi/linux/sched.h | 36 ++++++++--- kernel/fork.c | 123 +++++++++++++++++++++++++++++-------- 2 files changed, 122 insertions(+), 37 deletions(-)

diff --git a/include/uapi/linux/sched.h b/include/uapi/linux/sched.h index 3bac0a8ceab2..56ca73ca763c 100644 --- a/include/uapi/linux/sched.h +++ b/include/uapi/linux/sched.h @@ -87,26 +87,42 @@

• The structure is versioned by size and thus extensible.
• New struct members must go at the end of the struct and

• • must be properly 64bit aligned.

• • must be properly aligned on at least 64bit boundary.
• • See below on explicit padding when:
• • ((__alignof__(__kernel_uintptr_t) > __alignof__(__u64))
• */

+#define __rsvd(tag) \

• __u8 __rsvd_##tag[sizeof(__kernel_uintptr_t) - sizeof(__u64)]

struct clone_args { __aligned_u64 flags;

• __aligned_u64 pidfd;
• __aligned_u64 child_tid;
• __aligned_u64 parent_tid;

• __rsvd(v0_1);
• __kernel_aligned_uintptr_t pidfd;
• __kernel_aligned_uintptr_t child_tid;
• __kernel_aligned_uintptr_t parent_tid; __aligned_u64 exit_signal;

• __aligned_u64 stack;

• __rsvd(v0_2);
• __kernel_aligned_uintptr_t stack; __aligned_u64 stack_size;

• __aligned_u64 tls;
• __aligned_u64 set_tid;

• __rsvd(v0_3);
• __kernel_aligned_uintptr_t tls;
• /* VER0 boundary */
• __kernel_aligned_uintptr_t set_tid; __aligned_u64 set_tid_size;
• /* VER1 boundary */ __aligned_u64 cgroup;
• /* VER2 boundary */

}; #endif -#define CLONE_ARGS_SIZE_VER0 64 /* sizeof first published struct */ -#define CLONE_ARGS_SIZE_VER1 80 /* sizeof second published struct */ -#define CLONE_ARGS_SIZE_VER2 88 /* sizeof third published struct */ +#define CLONE_ARGS_SIZE_VER0 /* sizeof first published struct */ \

• ((__alignof__(__kernel_uintptr_t) > __alignof__(__u64)) ? 128 : 64)

+#define CLONE_ARGS_SIZE_VER1 /* sizeof second published struct */ \

• ((__alignof__(__kernel_uintptr_t) > __alignof__(__u64)) ? 152 : 80)

+#define CLONE_ARGS_SIZE_VER2 /* sizeof third published struct */ \

• ((__alignof__(__kernel_uintptr_t) > __alignof__(__u64)) ? 160 : 88)

/*

• Scheduling policies

diff --git a/kernel/fork.c b/kernel/fork.c index 181771571599..72de1187a586 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -2759,67 +2759,136 @@ SYSCALL_DEFINE5(clone, unsigned long, clone_flags, user_uintptr_t, newsp, #ifdef __ARCH_WANT_SYS_CLONE3 +#define __clone_args_size_ver(ver, pfx) \

• pfx##CLONE_ARGS_SIZE_VER##ver

+#ifdef CONFIG_COMPAT64 +struct compat_clone_args {

• __aligned_u64 flags;
• __aligned_u64 pidfd;
• __aligned_u64 child_tid;
• __aligned_u64 parent_tid;
• __aligned_u64 exit_signal;
• __aligned_u64 stack;
• __aligned_u64 stack_size;
• __aligned_u64 tls;
• __aligned_u64 set_tid;
• __aligned_u64 set_tid_size;
• __aligned_u64 cgroup;

+};

+#define COMPAT_CLONE_ARGS_SIZE_VER0 64 /* sizeof first published struct */ +#define COMPAT_CLONE_ARGS_SIZE_VER1 80 /* sizeof second published struct */ +#define COMPAT_CLONE_ARGS_SIZE_VER2 88 /* sizeof third published struct */

+#define clone_args_size_ver(ver) \

• (in_compat_syscall() ? __clone_args_size_ver(ver, COMPAT_) \

We are ending up with really a lot of calls to in_compat_syscall() in copy_clone_args_from_user(). It looks like in_compat_syscall() is completely implemented inline, so maybe the compiler can decide that it can just call it once and cache the result, but that's definitely worth checking.

• 	     : __clone_args_size_ver(ver, ))
  

+#define clone_args_get(args, member) \

• (in_compat_syscall() ? (args)->__compat_args.member \

• 	     : (args)->__args.member)
  

+#define clone_args_as_user_ptr(args, member) \

• (in_compat_syscall() ? compat_ptr(clone_args_get(args, member)) \

• 	     : as_user_ptr(clone_args_get(args, member)))
  

+#else /* CONFIG_COMPAT64 */ +#define clone_args_size_ver(ver __clone_args_size_ver(ver, ) +#define clone_args_get(args, member) ((args)->member) +#define clone_args_as_user_ptr(args, member) \

• as_user_ptr(clone_args_get(args, member))

I was going to say that this does not do what you want, but, having just looked again at the implementation of as_user_ptr(), I stand corrected! In any case, this was not the intention for as_user_ptr(), it is meant to signal that an arbitrary integer is being stored into a user pointer (see Documentation/core-api/user_ptr.rst). This is not what happens here, you just want to get a void __user * from a __kernel_uintptr_t. Since this does not involve any representation change [1], a simple cast is appropriate. This is what we now have in aio_setup_rw() for instance.

To avoid any confusion I would also rename the macro, maybe clone_args_get_user_ptr()?

[1] Writing this I realise this is not strictly true: in 32-bit, __kernel_uintptr_t is 64-bit but void __user * is 32-bit. This is not a concern though as the compiler is happy to cast a 64-bit integer to a 32-bit pointer.

+#endif /* CONFIG_COMPAT64 */

+static inline void clone_args_validate_static(void) +{ +#define CLONE_ARGS_BUILD_BUG_ON(type, pfx) \

Nit: might as well add an extra hard tab to the indentation of , better than just a space.

+do { \

• BUILD_BUG_ON(offsetofend(type, tls) != pfx##_SIZE_VER0); \
• BUILD_BUG_ON(offsetofend(type, set_tid_size) != pfx##_SIZE_VER1);\

Nit: a stray hard tab after != on that line, and two spaces after != on the line above.

• BUILD_BUG_ON(offsetofend(type, cgroup) != pfx##_SIZE_VER2); \
• BUILD_BUG_ON(sizeof(type) != pfx##_SIZE_VER2); \

Why not use that nice __clone_args_size_ver() you introduced?

+} while (0)

• CLONE_ARGS_BUILD_BUG_ON(struct clone_args, CLONE_ARGS);

+#ifdef CONFIG_COMPAT64

• CLONE_ARGS_BUILD_BUG_ON(struct compat_clone_args, COMPAT_CLONE_ARGS);
• BUILD_BUG_ON(sizeof(struct clone_args) < sizeof(struct compat_clone_args));

+#endif +}

noinline static int copy_clone_args_from_user(struct kernel_clone_args *kargs, struct clone_args __user *uargs, size_t usize) { int err; +#ifdef CONFIG_COMPAT64

• union {

• struct clone_args	 __args;
  

• struct compat_clone_args __compat_args;
  

• } args;

+#else struct clone_args args; +#endif pid_t *kset_tid = kargs->set_tid;

• BUILD_BUG_ON(offsetofend(struct clone_args, tls) !=

•      CLONE_ARGS_SIZE_VER0);
  

• BUILD_BUG_ON(offsetofend(struct clone_args, set_tid_size) !=

•      CLONE_ARGS_SIZE_VER1);
  

• BUILD_BUG_ON(offsetofend(struct clone_args, cgroup) !=

•      CLONE_ARGS_SIZE_VER2);
  

• BUILD_BUG_ON(sizeof(struct clone_args) != CLONE_ARGS_SIZE_VER2);

• clone_args_validate_static();

if (unlikely(usize > PAGE_SIZE)) return -E2BIG;

• if (unlikely(usize < CLONE_ARGS_SIZE_VER0))

• if (unlikely(usize < clone_args_size_ver(0))) return -EINVAL;

+#ifdef CONFIG_COMPAT64

• err = copy_struct_from_user(&args, (in_compat_syscall() ?

• 		    sizeof(struct compat_clone_args) :
  

• 		    sizeof(args)),
  

• 		    uargs, usize);
  

+#else err = copy_struct_from_user(&args, sizeof(args), uargs, usize); +#endif

• if (err) return err;

• if (unlikely(args.set_tid_size > MAX_PID_NS_LEVEL))

• if (unlikely(clone_args_get(&args, set_tid_size) > MAX_PID_NS_LEVEL)) return -EINVAL;

• if (unlikely(!args.set_tid && args.set_tid_size > 0))

• if (unlikely(!clone_args_get(&args, set_tid) &&

•      clone_args_get(&args, set_tid_size) > 0))
  

  return -EINVAL;

• if (unlikely(args.set_tid && args.set_tid_size == 0))

• if (unlikely(clone_args_get(&args, set_tid) &&

•      clone_args_get(&args, set_tid_size) == 0))
  

  return -EINVAL;

/* * Verify that higher 32bits of exit_signal are unset and that * it is a valid signal */

• if (unlikely((args.exit_signal & ~((u64)CSIGNAL)) ||

•      !valid_signal(args.exit_signal)))
  

• if (unlikely((clone_args_get(&args, exit_signal) & ~((u64)CSIGNAL)) ||

•      !valid_signal(clone_args_get(&args, exit_signal))))
  

  return -EINVAL;

• if ((args.flags & CLONE_INTO_CGROUP) &&

•    (args.cgroup > INT_MAX || usize < CLONE_ARGS_SIZE_VER2))
  

• if ((clone_args_get(&args, flags) & CLONE_INTO_CGROUP) &&

•    (clone_args_get(&args, cgroup) > INT_MAX ||
  

•     usize < clone_args_size_ver(2)))
  

  return -EINVAL;

*kargs = (struct kernel_clone_args){

• .flags		= args.flags,
  

• .pidfd		= u64_to_user_ptr(args.pidfd),
  

• .child_tid	= u64_to_user_ptr(args.child_tid),
  

• .parent_tid	= u64_to_user_ptr(args.parent_tid),
  

• .exit_signal	= args.exit_signal,
  

• .stack		= args.stack,
  

• .stack_size	= args.stack_size,
  

• .tls		= args.tls,
  

• .set_tid_size	= args.set_tid_size,
  

• .cgroup		= args.cgroup,
  

• .flags		= clone_args_get(&args, flags),
  

• .pidfd		= clone_args_as_user_ptr(&args, pidfd),
  

• .child_tid	= clone_args_as_user_ptr(&args, child_tid),
  

• .parent_tid	= clone_args_as_user_ptr(&args, parent_tid),
  

• .exit_signal	= clone_args_get(&args, exit_signal),
  

• .stack		= clone_args_get(&args, stack),
  

• .stack_size	= clone_args_get(&args, stack_size),
  

• .tls		= clone_args_get(&args, tls),
  

• .set_tid_size	= clone_args_get(&args, set_tid_size),
  

• .cgroup		= clone_args_get(&args, cgroup),
  

  };

• if (args.set_tid &&

• copy_from_user(kset_tid, u64_to_user_ptr(args.set_tid),
  

• if (clone_args_get(&args, set_tid) &&

• copy_from_user(kset_tid, clone_args_as_user_ptr(&args, set_tid),
  (kargs->set_tid_size * sizeof(pid_t))))
  

  return -EFAULT;

On 18/11/2022 01:05, Beata Michalska wrote:

Add a bunch of test-cases for clone3 syscall, focusing mainly on handling struct clone_args versioning and sizing, alongside the obvious req for respecting capabilities.

Signed-off-by: Beata Michalska beata.michalska@arm.com

tools/testing/selftests/arm64/morello/clone.c | 269 +++++++++++++++++- 1 file changed, 267 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/arm64/morello/clone.c b/tools/testing/selftests/arm64/morello/clone.c index 8217840ba504..13c84dc7ca77 100644 --- a/tools/testing/selftests/arm64/morello/clone.c +++ b/tools/testing/selftests/arm64/morello/clone.c @@ -1,15 +1,18 @@ // SPDX-License-Identifier: GPL-2.0 // Copyright (C) 2021 Arm Limited +#include "freestanding.h" #include <linux/mman.h> #include <linux/resource.h> #include <linux/wait.h> #include <linux/signal.h> #include <linux/sched.h> #include <linux/errno.h> +#include <linux/signal.h> +#include <limits.h> #include <cheriintrin.h> -#include "freestanding.h" -#define STACK_SIZE 1024*1024 +#define STACK_SIZE 1024*1024 +#define DEFAULT_PAGE_SIZE 4096 #define in_restricted() \ (!(cheri_perms_get(cheri_pcc_get()) & ARM_CAP_PERMISSION_EXECUTIVE)) @@ -17,6 +20,10 @@ #define MAGIC_NR 0x3562 /* whatever really ..... */ +#ifndef MAX_PID_NS_LEVEL +#define MAX_PID_NS_LEVEL 32 +#endif

/* Cloned thread result */ #define CTR_SUCCESS 1 #define CTR_FAILED -1 @@ -256,6 +263,263 @@ TEST(test_wait_rusage) RUN_WITH_FIXTURE(data, CLONE_TH_RUSAGE, wait_single); }

+#define GET_ERRNO(err) (((err) + MAX_ERRNO) ^ MAX_ERRNO)

This magic vaguely rings a bell but I'm not entirely sure why we need it here, could you elaborate?

+#define DECLARE_DESC(arg, val) \

• { \

• offsetof(struct clone_args, arg),	\
  

• sizeof(((struct clone_args *)0)->arg),	\
  

• (uintcap_t)(val)			\
  

• }

+struct clone3_fixture {

• size_t args_size;
• struct {

• unsigned int offset;
  

• unsigned int size;
  

• uintcap_t    value;
  

This is quite a complicated way to initialise a struct per-test. Having directly struct clone_args here would make things a lot simpler (and it would take less space in fact).

I understand that what I'm suggesting is not quite the same, as instead of overriding the default values here, you would be providing the defaults and run_clone3() would need to figure out what to provide itself. AFAICT this can simply be done by checking if the field is 0 (e.g. .flags), with the notable exception of .stack. You could have a .custom_stack field or similar to tell run_clone3() not to touch .stack / .stack_size.

It's worth noting that the current approach does not really work for the stack either: in case 6, you end up leaking the whole stack as the munmap() in run_clone3() will get NULL instead of the originally allocated stack. Therefore it seems to me that something like a .custom_stack field is required anyway.

• /* This will get extra size but that keeps things on the safe side */
• } const args_desc[sizeof(struct clone_args)/sizeof(__u64)];
• int n_desc;
• int e_result;

+} const clone3_data[] = {

• /* BEGIN_SECTION: expected failure */
• /* size of clone_args smaller than CLONE_ARGS_SIZE_VER0 */
• {

• .args_size = offsetof(struct clone_args, tls),
  

• .args_desc = {},
  

• .n_desc = 0,
  

• .e_result = -EINVAL
  

• }, /* @{0} */
• /* invalid set_tid array size */
• {

• .args_size = sizeof(struct clone_args),
  

• .args_desc = {
  

• 		DECLARE_DESC(set_tid_size, MAX_PID_NS_LEVEL + 1),
  

• 	     },
  

• .n_desc = 1,
  

• .e_result = -EINVAL
  

• }, /* @{1} */
• /* Invalid combination of set_tid & set_tid_size */
• {

• .args_size = sizeof(struct clone_args),
  

• .args_desc = {
  

• 		DECLARE_DESC(set_tid, (uintcap_t) NULL),
  

• 		DECLARE_DESC(set_tid_size, 1)
  

• 	     },
  

• .n_desc = 2,
  

• .e_result = -EINVAL
  

• }, /* @{2} */
• /* Invalid exit_signal */
• {

• .args_size = sizeof(struct clone_args),
  

• .args_desc = {
  

• 		DECLARE_DESC(exit_signal, _NSIG + 1),
  

• 	     },
  

• .n_desc = 1,
  

• .e_result = -EINVAL
  

• }, /* @{3} */
• /* Invalid cgroup number */
• {

• .args_size = sizeof(struct clone_args),
  

• .args_desc = {
  

• 		DECLARE_DESC(flags, CLONE_INTO_CGROUP),
  

• 		DECLARE_DESC(cgroup, (__u64)INT_MAX + 1),
  

• 	     },
  

• .n_desc = 2,
  

• .e_result = -EINVAL
  

• }, /* @{4} */
• /* Invalid size for clone_args with cgroup */
• {

• .args_size = offsetof(struct clone_args, cgroup),
  

• .args_desc = {
  

• 		DECLARE_DESC(flags, CLONE_INTO_CGROUP),
  

• 		DECLARE_DESC(cgroup, 1),
  

• 	     },
  

• .n_desc = 2,
  

• .e_result = -EINVAL
  

• }, /* @{5} */
• /* Invalid stack & stack_size combination */
• {

• .args_size = sizeof(struct clone_args),
  

• .args_desc = {
  

• 		DECLARE_DESC(stack, NULL),
  

• 		DECLARE_DESC(stack_size, 4096),
  

• 	     },
  

• .n_desc = 2,
  

• .e_result = -EINVAL
  

• }, /* @{6} */
• {

• .args_size = sizeof(struct clone_args),
  

• .args_desc = {
  

• 		DECLARE_DESC(stack_size, 0),
  

• 	     },
  

• .n_desc = 1,
  

• .e_result = -EINVAL
  

• }, /* @{7} */
• {

• .args_size = sizeof(struct clone_args),
  

• .args_desc = {
  

• 		DECLARE_DESC(set_tid, &(pid_t){1}),
  

• 		DECLARE_DESC(set_tid_size, 1),
  

• 	     },
  

• .n_desc = 2,
  

• .e_result = -EEXIST
  

• }, /* @{8} */
• /* END_SECTION: expected failure */
• {

• .args_size = sizeof(struct clone_args),
  

• .args_desc = {
  

• 		DECLARE_DESC(flags, CLONE_PIDFD |
  

• 			     CLONE_CHILD_SETTID |
  

• 			     CLONE_CHILD_CLEARTID),
  

• 	     },
  

• .n_desc = 1,
  

• .e_result = 0
  

• }, /* @{9} */
• {

• .args_size = sizeof(struct clone_args),
  

• .args_desc = {
  

• 		DECLARE_DESC(flags, CLONE_PARENT_SETTID |
  

• 			     CLONE_CHILD_SETTID),
  

• 	     },
  

• .n_desc = 1,
  

• .e_result = 0
  

• }, /* @{10} */
• {

• .args_size = sizeof(struct clone_args),
  

• .args_desc = {
  

• 		DECLARE_DESC(flags, CLONE_SETTLS),
  

• 	     },
  

• .n_desc = 1,
  

• .e_result = 0
  

• }, /* @{11} */

+};

+__thread int tls_data;

Nit: could be static and local to run_child().

+static __attribute__((noinline)) void run_child(struct clone_args *args) +{

• int count = 1000;
• if (args->flags & CLONE_CHILD_SETTID) {

• pid_t current_pid = (pid_t)syscall(__NR_getpid);
  

We've got a getpid() wrapper in freestanding.h, could use it here.

• ASSERT_FALSE(current_pid != *(pid_t *)args->child_tid);
  

• }
• if (args->flags & CLONE_SETTLS && args->tls) {

• ptraddr_t base_addr = cheri_address_get(args->tls);
  

• ptraddr_t ref_addr = cheri_address_get(&tls_data);
  

• size_t length = cheri_length_get(args->tls);
  

• ASSERT_TRUE(cheri_tag_get(args->tls));
  

• ASSERT_FALSE(ref_addr < base_addr || ref_addr >= base_addr + length);
  

Nit: I think the reverse condition with ASSERT_TRUE() is a bit more readable.

• }
• /*

• * This is grisly ....
  

• * Spin for a while to give a chance for the parent to be
  

• * scheduled in after clone and before this thread terminates
  

• * (not entirely reliable though)
  

• */
  

• while (--count)

• sleep_cycles(&(int){1000});
  

• syscall(__NR_exit, 0);

+}

+static inline __attribute__((always_inline)) +void run_clone3(const struct clone3_fixture *data) +{

• int pidfd, parent_tid, child_tid;
• siginfo_t *wait_si = &(siginfo_t){0};

Can't we just have `siginfo_t wait_si;` (no need to initialise it either as it's never read)?

• int result;
• pid_t pid;
• struct clone_args args = {

• .flags = 0,
  

• .pidfd = (uintcap_t)&pidfd,
  

• .parent_tid = (uintcap_t)&parent_tid,
  

• .child_tid = (uintcap_t)&child_tid,
  

• .exit_signal = SIGCHLD,
  

• .stack = (uintcap_t) mmap_verified(NULL, STACK_SIZE,
  

• 				   PROT_READ | PROT_WRITE,
  

• 				   MAP_ANONYMOUS | MAP_PRIVATE,
  

• 				   -1, 0, STACK_REQ_PERMS),
  

• .stack_size = STACK_SIZE,
  

• };
• for (int i = 0; i < data->n_desc; ++i) {

• unsigned char *arg;
  

• arg = (unsigned char *)&args + data->args_desc[i].offset;
  

• if (data->args_desc[i].size != sizeof(uintcap_t))
  

• 	*(__u64 *)arg = (__u64)data->args_desc[i].value;
  

• else
  

• 	*(uintcap_t *)arg = data->args_desc[i].value;
  

• }
• /* Don't bother with the full set-up for expected failures */
• if (data->e_result) {

• result = syscall(__NR_clone3, &args, data->args_size);
  

• ASSERT_EQ(result, data->e_result) {
  

ASSERT_* causes the whole program to exit, so you probably want EXPECT_EQ() here.

• 	TH_LOG("Expected: %d while %d was received",
  

• 	       GET_ERRNO(data->e_result), GET_ERRNO(result));
  

• }
  

• munmap((void *)args.stack, STACK_SIZE);
  

• return;
  

• }
• args.flags |= CLONE_VM;
• if (args.flags & CLONE_SETTLS) {

• void *addr = mmap_verified(NULL, DEFAULT_PAGE_SIZE,
  

Not a big fan of calling this DEFAULT_PAGE_SIZE, as we shouldn't make any assumption on the page size. Fortunately the page size doesn't actually matter here as both mmap() and munmap() will align up the length to the page size. Maybe simply TLS_SIZE?

• 			   PROT_READ | PROT_WRITE,
  

• 			   MAP_ANONYMOUS | MAP_PRIVATE, -1, 0,
  

• 			   CAP_LOAD_PERMS | CAP_STORE_PERMS);
  

• args.tls = (uintcap_t)cheri_bounds_set(addr, DEFAULT_PAGE_SIZE);
  

• }
• pid = syscall(__NR_clone3, &args, data->args_size);
• ASSERT_GE(pid, 0);
• if (!pid)

• run_child(&args);
  

• if (args.flags & CLONE_PIDFD) {

• /* Signal handling is not the test target here: valid pidfd is */
  

• result = syscall(__NR_pidfd_send_signal, pidfd, SIGUSR1, NULL, 0);
  

• ASSERT_NE(result, -EBADF);
  

Agreed that we're not testing signal handling here, in fact for that reason I'd suggest using a signal that is ignored by default like SIGCHLD, not SIGUSR1 which causes termination by default. Alternatively, you could use sigwaitinfo() in the child, which also means you wouldn't need the busy loop.

OTOH I think it wouldn't hurt to check that pidfd_send_signal() didn't fail (in any way), as if it does that would suggest something went wrong with the test setup.

• }
• waitid(P_PID, pid, wait_si, WEXITED, NULL);

Should check that this one doesn't fail. This also makes me think, can we now get rid of the scary wait substitute in clone_single(), since we're not bothering with it here?

• /* child_tid set once the thread gets scheduled */
• if (args.flags & CLONE_PARENT_SETTID && args.flags & CLONE_CHILD_SETTID

•    && !(args.flags & CLONE_CHILD_CLEARTID))
  

• ASSERT_EQ(*(pid_t *)args.parent_tid, *(pid_t *)args.child_tid)
  

Considering that none of the testcases override either argument, that could be simply written as ASSERT_EQ(parent_tid, child_tid).

For extra robustness, maybe also 0-init both variables and check that the value is not 0 (i.e. the kernel didn't perform either write)?

• if (args.flags & CLONE_CHILD_CLEARTID)

• ASSERT_EQ(*(pid_t *)args.child_tid, 0);
  

• munmap((void *)args.stack, STACK_SIZE);
• if (args.flags & CLONE_SETTLS)

• munmap((void *)args.tls, STACK_SIZE);
  

s/STACK_SIZE/DEFAULT_PAGE_SIZE/

Kevin

+}

+TEST(test_clone3) +{

• int ncount = sizeof(clone3_data)/sizeof(clone3_data[0]);
• for (int i = 0; i < ncount; ++i) {

• TH_LOG("Validating clone3 @{%d}", i);
  

• run_clone3(&clone3_data[i]);
  

• }

+}

void run_restricted(uintcap_t entry_point) { void *new_stack = allocate_mem_raw(STACK_SIZE); @@ -290,5 +554,6 @@ int main(void) run_restricted((uintcap_t)test_clone_tls_restricted); test_wait_raw(); test_wait_rusage();

• test_clone3(); return 0;

}

On 30/11/2022 22:27, Beata Michalska wrote:

On Wed, Nov 30, 2022 at 03:49:38PM +0100, Kevin Brodsky wrote:

...

On 18/11/2022 01:05, Beata Michalska wrote:

...

Add a bunch of test-cases for clone3 syscall, focusing mainly on handling struct clone_args versioning and sizing, alongside the obvious req for respecting capabilities.

Signed-off-by: Beata Michalska beata.michalska@arm.com

tools/testing/selftests/arm64/morello/clone.c | 269 +++++++++++++++++- 1 file changed, 267 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/arm64/morello/clone.c b/tools/testing/selftests/arm64/morello/clone.c index 8217840ba504..13c84dc7ca77 100644 --- a/tools/testing/selftests/arm64/morello/clone.c +++ b/tools/testing/selftests/arm64/morello/clone.c @@ -1,15 +1,18 @@ // SPDX-License-Identifier: GPL-2.0 // Copyright (C) 2021 Arm Limited +#include "freestanding.h" #include <linux/mman.h> #include <linux/resource.h> #include <linux/wait.h> #include <linux/signal.h> #include <linux/sched.h> #include <linux/errno.h> +#include <linux/signal.h> +#include <limits.h> #include <cheriintrin.h> -#include "freestanding.h" -#define STACK_SIZE 1024*1024 +#define STACK_SIZE 1024*1024 +#define DEFAULT_PAGE_SIZE 4096 #define in_restricted() \ (!(cheri_perms_get(cheri_pcc_get()) & ARM_CAP_PERMISSION_EXECUTIVE)) @@ -17,6 +20,10 @@ #define MAGIC_NR 0x3562 /* whatever really ..... */ +#ifndef MAX_PID_NS_LEVEL +#define MAX_PID_NS_LEVEL 32 +#endif

/* Cloned thread result */ #define CTR_SUCCESS 1 #define CTR_FAILED -1 @@ -256,6 +263,263 @@ TEST(test_wait_rusage) RUN_WITH_FIXTURE(data, CLONE_TH_RUSAGE, wait_single); }

+#define GET_ERRNO(err) (((err) + MAX_ERRNO) ^ MAX_ERRNO)

This magic vaguely rings a bell but I'm not entirely sure why we need it here, could you elaborate?

Getting the absolute value to report the errno. It's not strictly necessary, though I think I found that convenient at some point....

Why not, I'm not sure it helps that much though, in fact it feels like "ASSERT_EQ(result, data->e_result)" is enough on its own without the explicit print after.

[...]

...

...

• int result;
• pid_t pid;
• struct clone_args args = {

• .flags = 0,
  

• .pidfd = (uintcap_t)&pidfd,
  

• .parent_tid = (uintcap_t)&parent_tid,
  

• .child_tid = (uintcap_t)&child_tid,
  

• .exit_signal = SIGCHLD,
  

• .stack = (uintcap_t) mmap_verified(NULL, STACK_SIZE,
  

• 				   PROT_READ | PROT_WRITE,
  

• 				   MAP_ANONYMOUS | MAP_PRIVATE,
  

• 				   -1, 0, STACK_REQ_PERMS),
  

• .stack_size = STACK_SIZE,
  

• };
• for (int i = 0; i < data->n_desc; ++i) {

• unsigned char *arg;
  

• arg = (unsigned char *)&args + data->args_desc[i].offset;
  

• if (data->args_desc[i].size != sizeof(uintcap_t))
  

• 	*(__u64 *)arg = (__u64)data->args_desc[i].value;
  

• else
  

• 	*(uintcap_t *)arg = data->args_desc[i].value;
  

• }
• /* Don't bother with the full set-up for expected failures */
• if (data->e_result) {

• result = syscall(__NR_clone3, &args, data->args_size);
  

• ASSERT_EQ(result, data->e_result) {
  

ASSERT_* causes the whole program to exit, so you probably want EXPECT_EQ() here.

That would mean all ASSERT_x should be replaced with EXPECT_x counterparts ?

Sorry I'm realising I didn't think this straight, if you exit the following munmap() is irrelevant, so there is no particular reason to use EXPECT here. I don't have much opinion on ASSERT vs EXPECT, I don't think it really matters for these tests, so no problem leaving it unchanged.

...

...

• 	TH_LOG("Expected: %d while %d was received",
  

• 	       GET_ERRNO(data->e_result), GET_ERRNO(result));
  

• }
  

• munmap((void *)args.stack, STACK_SIZE);
  

• return;
  

• }
• args.flags |= CLONE_VM;
• if (args.flags & CLONE_SETTLS) {

• void *addr = mmap_verified(NULL, DEFAULT_PAGE_SIZE,
  

Not a big fan of calling this DEFAULT_PAGE_SIZE, as we shouldn't make any assumption on the page size. Fortunately the page size doesn't actually matter here as both mmap() and munmap() will align up the length to the page size. Maybe simply TLS_SIZE?

Yeah, I did hesitate on that one for a moment, but as this one is not literally expecting PAGE_SIZE I went for it. I'll change that to TLS_SIZE instead.

...

...

• 			   PROT_READ | PROT_WRITE,
  

• 			   MAP_ANONYMOUS | MAP_PRIVATE, -1, 0,
  

• 			   CAP_LOAD_PERMS | CAP_STORE_PERMS);
  

• args.tls = (uintcap_t)cheri_bounds_set(addr, DEFAULT_PAGE_SIZE);
  

• }
• pid = syscall(__NR_clone3, &args, data->args_size);
• ASSERT_GE(pid, 0);
• if (!pid)

• run_child(&args);
  

• if (args.flags & CLONE_PIDFD) {

• /* Signal handling is not the test target here: valid pidfd is */
  

• result = syscall(__NR_pidfd_send_signal, pidfd, SIGUSR1, NULL, 0);
  

• ASSERT_NE(result, -EBADF);
  

Agreed that we're not testing signal handling here, in fact for that reason I'd suggest using a signal that is ignored by default like SIGCHLD, not SIGUSR1 which causes termination by default. Alternatively, you could use sigwaitinfo() in the child, which also means you wouldn't need the busy loop.

True, though I was trying to avoid using signal handling here on purpose, to not to rely on other 'functionality' to align with our initial approach to kselftests - test with minimal setup to avoid the test failing on things that are not the test subject themselves.

...

OTOH I think it wouldn't hurt to check that pidfd_send_signal() didn't fail (in any way), as if it does that would suggest something went wrong with the test setup.

... or smth is wrong with handling signals which relates to my comment above. I can change both if we want to relax our kselftests.

I'd go for that, assuming that fundamental subsystems like signals cannot be relied on creates a lot of overhead, so I'd only do that for the most basic tests, and I don't consider clone3 one of them.

...

...

• }
• waitid(P_PID, pid, wait_si, WEXITED, NULL);

Should check that this one doesn't fail. This also makes me think, can we now get rid of the scary wait substitute in clone_single(), since we're not bothering with it here?

We could though at this point, the wait testcases are done, which means we can safely assume wait is working as expected and it's safe to rely on it. Again - exercising limited trust.

Yep makes sense.

Kevin