On 13/11/2023 10:31, Kevin Brodsky wrote:
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/...
Makes sense, thanks for going into details.
It does indeed do what I expect with elf_stack_put_user, so all good.
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