On 27/04/2023 15:22, Luca Vizzarro wrote:
On 24/03/2023 16:20, Kevin Brodsky wrote:
+bool check_user_ptr_read(void __user *ptr, size_t len); +bool check_user_ptr_write(void __user *ptr, size_t len); +bool check_user_ptr_rw(void __user *ptr, size_t len);
Hi Kevin,
I've been testing your patch with some explicit checking and I see that this may not work in every situation. In some cases the capability to be checked has a const qualifier, resulting in a compiler error when passsing it along to one of these.
Best, Luca
Indeed. This is uncontroversial for check_user_ptr_read(), but I will also add it to the others, specifically because of fault_in_user_writeable(), which takes a const pointer for some obscure reason. It does not hurt to have all these functions take a const pointer, but it can be a bit confusing, so I will add a comment on that topic.
Looking at your explicit check patch, I am thinking about making another change: we could potentially return to a single check_user_ptr() helper that takes a permissions argument, but this time round using READ and WRITE from linux/kernel.h instead of creating a new type like in the original RFC [1]. I am suggesting this as it would make it easier to check for variable permissions, like in lib/iov_iter.c, and would be generally more consistent with helpers that check for permissions. I did not want to use PROT_READ / PROT_WRITE as it would create confusion with mapping permissions, but READ and WRITE are generic so it should be fine from that perspective.
Any opinion on this? Interested in opinions from everyone, especially Luca and Beata (who first proposed having multiple helpers instead of a permissions argument).
Kevin
[1] https://op-lists.linaro.org/archives/list/linux-morello@op-lists.linaro.org/...