On 09/11/2023 18:30, Teo Couprie Diaz wrote:

+
+#if (ELF_COMPAT == 0)
+	str_ptr = cheri_perms_and(ustr,
+		(CHERI_PERM_GLOBAL | CHERI_PERM_STORE | CHERI_PERM_LOAD));
+	str_ptr = cheri_bounds_set_exact(str_ptr, len);
+	if (put_user_ptr(str_ptr, stack_item++))
+		return -EFAULT;
+#else
+	if (elf_stack_put_user(ustr, stack_item++))

Nit: to reduce duplication some more and make it even clearer what's
happening, this line could be common to the two paths, simply setting
str_ptr to ustr in compat.

I wasn't sure when preparing v3 as I got a compile time assertion when
using what I felt was the most generic way to put the pointer on the
stack,
with put_user_ptr.

It seems to build when using elf_stack_put_user instead and gets rid
of the
warning introduced. Testing it looks to be just fine as well, so I
just didn't
understand those two properly :) 

I recently simplified elf_stack_put_user() [1], so it should be a little
easier to follow what it does. Overall, it should do what you want, that
is storing some value in a pointer-sized "stack slot", in both native
and compat. In native, it is equivalent to put_user_ptr(), though with a
cast so that the first argument can also be some arbitrary integer. In
compat, it is an elf_addr_t-sized put_user(), and the casts allow a
(native) user pointer to be passed (in PCuABI / compat64 this means that
the address will be stored). Using put_user_ptr() directly is not valid
in compat, as compat stack slots are smaller than a native user pointer.

[1]
https://op-lists.linaro.org/archives/list/linux-morello@op-lists.linaro.org/message/PBCCHFWE6EVCFTCHCSYUXZD35OKPIAE7/

Makes sense, thanks for going into details.

Changed to have elf_stack_put_user outside the #ifdef.

+		return -EFAULT;
+#endif
+
+	/*
+	 * If right after the end of the argument length we have a zero,
+	 * that means the argument alignment was adjusted in order to create a
+	 * representable capability in purecap, even if we are not loading a
+	 * purecap binary. This padding is added at the end, so find the real
+	 * end by going through the padding.
+	 */
+	if (get_user(c, ustr + len))
+		return -EFAULT;
+	if (c == '\0') {
+		size_t pad_len = 0;
+
+		while (pad_len < MAX_ARG_STRLEN && c == '\0') {
+			pad_len++;

I think there's an off-by-one, since we increment pad_len, then call
get_user(), and then only after that check that pad_len <
MAX_ARG_STRLEN. I was actually going to suggest using a for loop
instead, which makes such issues less likely.

I'd argue that in this case there's even an off-by len+1 ! I don't think
it makes sense to do use pad_len by itself here.

Otherwise agree, there is a mistake.

Also, I'm not sure we need to have that initial get_user() + if. We can
directly have a loop, which may exit right away (need to be careful
about limit conditions of course).

However I am not sure of the shape you would want for the for loop,
specifically if it were to remove the if check and get_user ?

There's two conditions there, is the padding too large and is the
character null.

Would you be happy with the following ?

  for (pad_len = 0; len + pad_len < MAX_ARG_STRLEN ; pad_len += 1)

With the get_user and a break condtion on != '\0' inside the loop body ?

But that doesn't exit right away, it goes through an iteration (which
makes sense to be fair).

So not sure of what you'd expect :)

I suppose more or less what you suggest! It's pretty standard to have
such a for loop (the last part can simply be pad_len++), and then a
conditional break inside. My point about exiting right away was probably
misleading - I meant that the first get_user() could break out of the
loop, but of course that first iteration will always be run.

Ah right, ok, I tried to look too deep into it ! Will simply do that then. Thanks for the clarification, should send v4 soon.

Kevin