Christophe Leroy <christophe.le...@c-s.fr> writes: > Le 20/03/2019 à 13:57, Michael Ellerman a écrit : >> Christophe Leroy <christophe.le...@c-s.fr> writes: >>> Le 08/03/2019 à 02:16, Michael Ellerman a écrit : >>>> From: Christophe Leroy <christophe.le...@c-s.fr> >>>> >>>> This patch implements a framework for Kernel Userspace Access >>>> Protection. >>>> >>>> Then subarches will have the possibility to provide their own >>>> implementation by providing setup_kuap() and >>>> allow/prevent_user_access(). >>>> >>>> Some platforms will need to know the area accessed and whether it is >>>> accessed from read, write or both. Therefore source, destination and >>>> size and handed over to the two functions. >>>> >>>> mpe: Rename to allow/prevent rather than unlock/lock, and add >>>> read/write wrappers. Drop the 32-bit code for now until we have an >>>> implementation for it. Add kuap to pt_regs for 64-bit as well as >>>> 32-bit. Don't split strings, use pr_crit_ratelimited(). >>>> >>>> Signed-off-by: Christophe Leroy <christophe.le...@c-s.fr> >>>> Signed-off-by: Russell Currey <rus...@russell.cc> >>>> Signed-off-by: Michael Ellerman <m...@ellerman.id.au> >>>> --- >>>> v5: Futex ops need read/write so use allow_user_acccess() there. >>>> Use #ifdef CONFIG_PPC64 in kup.h to fix build errors. >>>> Allow subarch to override allow_read/write_from/to_user(). >>> >>> Those little helpers that will just call allow_user_access() when >>> distinct read/write handling is not performed looks overkill to me. >>> >>> Can't the subarch do it by itself based on the nullity of from/to ? >>> >>> static inline void allow_user_access(void __user *to, const void __user >>> *from, >>> unsigned long size) >>> { >>> if (to & from) >>> set_kuap(0); >>> else if (to) >>> set_kuap(AMR_KUAP_BLOCK_READ); >>> else if (from) >>> set_kuap(AMR_KUAP_BLOCK_WRITE); >>> } >> >> You could implement it that way, but it reads better at the call sites >> if we have: >> >> allow_write_to_user(uaddr, sizeof(*uaddr)); >> vs: >> allow_user_access(uaddr, NULL, sizeof(*uaddr)); >> >> So I'm inclined to keep them. It should all end up inlined and generate >> the same code at the end of the day. >> > > I was not suggesting to completly remove allow_write_to_user(), I fully > agree that it reads better at the call sites. > > I was just thinking that allow_write_to_user() could remain generic and > call the subarch specific allow_user_access() instead of making multiple > subarch's allow_write_to_user()
Yep OK I see what you mean. Your suggestion above should work, and involves the least amount of ifdefs and so on. I'll try and get time to post a v6. cheers