Michael Ellerman <[email protected]> a écrit :
Christophe Leroy <[email protected]> writes:
Le 08/02/2021 à 14:57, Michael Ellerman a écrit :
We have a might_fault() check in __unsafe_put_user_goto(), but that is
dangerous as it potentially calls lots of code while user access is
enabled.
It also triggers the check Alexey added to the irq restore path to
catch cases like that:
WARNING: CPU: 30 PID: 1 at
arch/powerpc/include/asm/book3s/64/kup.h:324
arch_local_irq_restore+0x160/0x190
NIP arch_local_irq_restore+0x160/0x190
LR lock_is_held_type+0x140/0x200
Call Trace:
0xc00000007f392ff8 (unreliable)
___might_sleep+0x180/0x320
__might_fault+0x50/0xe0
filldir64+0x2d0/0x5d0
call_filldir+0xc8/0x180
ext4_readdir+0x948/0xb40
iterate_dir+0x1ec/0x240
sys_getdents64+0x80/0x290
system_call_exception+0x160/0x280
system_call_common+0xf0/0x27c
So remove the might fault check from unsafe_put_user().
Any call to unsafe_put_user() must be inside a region that's had user
access enabled with user_access_begin(), so move the might_fault() in
there. That also allows us to drop the is_kernel_addr() test, because
there should be no code using user_access_begin() in order to access a
kernel address.
x86 and mips only have might_fault() on get_user() and put_user(),
neither on __get_user() nor on __put_user() nor on the unsafe
alternative.
Yeah, that's their choice, or perhaps it's by accident.
arm64 on the other hand has might_fault() in all variants.
A __get_user() can fault just as much as a get_user(), so there's no
reason the check should be omitted from __get_user(), other than perhaps
some historical argument about __get_user() being the "fast" case.
When have might_fault() in __get_user_nocheck() that is used by
__get_user() and __get_user_allowed() ie by unsafe_get_user().
Shoudln't those be dropped as well ?
That was handled by Alexey's patch, which I ended up merging with this
one:
https://git.kernel.org/torvalds/c/7d506ca97b665b95e698a53697dad99fae813c1a
ie. we still have might_fault() in __get_user_nocheck(), but it's
guarded by a check of do_allow, so we won't call it for
__get_user_allowed().
So I think the code (in my next branch) is correct, we don't have any
might_fault() calls in unsafe regions.
But I'd still be happier if we could simplify our uaccess.h more, it's a
bit of a rats nest. We could start by making __get/put_user() ==
get/put_user() the same way arm64 did.
I agree there are several easy simplifications to do there. I'll look
at that in the coming weeks.
I'm not sure it is good to make __get/put_user equal get/put_user as
it would mean calling access_ok() everytime. But we could most likely
make something simpler with get_user() calling access_ok() then
__get_user().
I think we should also audit our use of the _inatomic variants.
might_fault() voids when pagefault is disabled so I think the inatomic
variants should be needed. As far as I can see, powerpc is the only
arch having that.
Need to also check why we still need that is_kernel_addr() check
because since the removal of set_fs(), __get/put_user() helpers
shouldn't be used anymore for kernel addresses
Christophe
