On 9/1/22 09:12, Kevin Brodsky wrote:
On 31/08/2022 16:37, Vincenzo Frascino wrote:
With the introduction of capabilities and PCuABI being enabled when dealing with the user pointers does expect a capability.
I failed to parse the sentence, I think the subject of "does" is missing :)
Yes, it is missing an "it". I will rephrase it to make it clearer.
Address the compilation warning below triggered by otherwise implicit conversion that might lead to unexpected behaviour when operating on capabilities.
make[1]: linux/kernel/bpf/helpers.c: warning: the following conversion will result in a CToPtr operation; the behaviour of CToPtr can be confusing since using CToPtr on an untagged capability will give 0 instead of the integer value and should therefore be explicitly annotated [-Wcheri-pointer-conversion] ret = access_process_vm(tsk, (unsigned long)user_ptr, dst,size, 0);
Signed-off-by: Vincenzo Frascino vincenzo.frascino@arm.com
kernel/bpf/helpers.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c index 315053ef6a75..33058bc56243 100644 --- a/kernel/bpf/helpers.c +++ b/kernel/bpf/helpers.c @@ -680,7 +680,7 @@ BPF_CALL_5(bpf_copy_from_user_task, void *, dst, u32, size, if (unlikely(!size)) return 0; - ret = access_process_vm(tsk, (unsigned long)user_ptr, dst, size, 0); + ret = access_process_vm(tsk, (user_uintptr_t)user_ptr, dst, size, 0);
This works, but since the desired operation here is to obtain the address of the user pointer, using user_ptr_addr() is preferred, see [1].
Adding a "TODO [PCuABI]" comment, like in 6914d33e4bcc ("fs/io_uring: Fix user pointer downcast") for instance, would also be good as it looks like we might be able to check the uaccess against the capability.
Kevin
Good point, I will address this in v2.
[1] https://git.morello-project.org/morello/kernel/linux/-/blob/morello/master/D...
if (ret == size) return 0;