On 12/06/2023 10:35, Beata Michalska wrote:
On Fri, May 12, 2023 at 07:34:16PM +0200, Kevin Brodsky wrote:
On 08/05/2023 09:37, Kevin Brodsky wrote:
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 have just realised this idea does not make sense, because READ / WRITE are not actually bitfield values, since they are respectively 0 and 1. Although some code does use them this way, READ | WRITE is meaningless, as it is the same as WRITE.
I'd therefore lean towards keeping the situation unchanged, to avoid having to introduce yet another kind of READ/WRITE flags. That said, we may actually have to do that if we want to check for bounds but not permissions (passing 0 as permission flags), which is one possible outcome of the io_uring-related discussion under the explicit capability check series. To be continued!
I am probably missing smth (for which I do apologize) but why not simply adding another variant for checking bounds only: check_usr_ptr_bounds() ? It might get bit clunky but that would allow checking permissions only.
Of course, that's an option. In fact there is no need for this in the end, at least none that we have found yet. In other words we always check both bounds and permissions at the same time.
I'm still bit torn apart between having single entry with different dedicated flags vs having dedicated entry with no flags. I guess at this point it all comes down to usability and AFAIU , we are having issues with the currently proposed solution ?
See the new thread I started [1] for a summary of the situation. Overall I think the current interface is fine. It is a little annoying when the permissions are only known at runtime, but that doesn't happen so often.
Kevin
[1] https://op-lists.linaro.org/archives/list/linux-morello@op-lists.linaro.org/...