On 08-05-2023 08:37, Kevin Brodsky wrote:
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
I don't see fault_in_user_writeable() taking a const pointer, what am I missing...?
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.
This sounds good to me. In my perspective, there's nothing confusing with a "check" function to take a const pointer regardless of the type of the pointer expected.
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.
I agree with this idea, it would make the code cleaner at least in a few cases of Luca's explicit capability checks series.
Tudor
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/...