On Mon, Sep 02, 2024 at 10:56:27AM +0200, David Hildenbrand wrote: > On 02.09.24 08:31, Omar Sandoval wrote: > > On Mon, Sep 02, 2024 at 08:19:33AM +0200, Christophe Leroy wrote: > > > > > > > > > Le 02/09/2024 à 07:31, Omar Sandoval a écrit : > > > > [Vous ne recevez pas souvent de courriers de osandov@xxxxxxxxxxx. Découvrez pourquoi ceci est important à https://aka.ms/LearnAboutSenderIdentification ] > > > > > > > > From: Omar Sandoval <osandov@xxxxxx> > > > > > > > > Hi, > > > > > > > > I hit a case where copy_to_kernel_nofault() will fault (lol): if the > > > > destination address is in userspace and x86 Supervisor Mode Access > > > > Prevention is enabled. Patch 2 has the details and the fix. Patch 1 > > > > renames a helper function so that its use in patch 2 makes more sense. > > > > If the rename is too intrusive, I can drop it. > > > > > > The name of the function is "copy_to_kernel". If the destination is a user > > > address, it is not a copy to kernel but a copy to user and you already have > > > the function copy_to_user() for that. copy_to_user() properly handles SMAP. > > > > I'm not trying to copy to user. I am (well, KDB is) trying to copy to an > > arbitrary address, and I want it to return an error instead of crashing > > if the address is not a valid kernel address. As far as I can tell, that > > is the whole point of copy_to_kernel_nofault(). > > The thing is that you (well, KDB) triggers something that would be > considered a real BUG when triggered from "ordinary" (non-debugging) code. If that's the case, then it's a really weird inconsistency that it's OK to call copy_from_kernel_nofault() with an invalid address but a bug to call copy_to_kernel_nofault() on the same address. Again, isn't the whole point of these functions to fail gracefully instead of crashing on invalid addresses? (Modulo the offline and hwpoison cases you mention for /proc/kcore.) > But now I am confused: "if the destination address is in userspace" does not > really make sense in the context of KDB, no? > > [15]kdb> mm 0 1234 > [ 94.652476] BUG: kernel NULL pointer dereference, address: > 0000000000000000 > > Why is address 0 in "user space"? "Which" user space? Sure, it's not really user space, but it's below TASK_SIZE_MAX, so things like handle_page_fault() and fault_in_kernel_space() treat it as if it were a user address. I could s/userspace address/address that is less than TASK_SIZE_MAX or is_vsyscall_vaddr(address)/. > Isn't the problem here that KDB lets you blindly write to any non-existing > memory address? > > > Likely it should do some proper filtering like we do in fs/proc/kcore.c: > > Take a look at the KCORE_RAM case where we make sure the page exists, is > online and may be accessed. Only then, we trigger a > copy_from_kernel_nofault(). Note that the KCORE_USER is a corner case only > for some special thingies on x86 (vsyscall), and can be ignored for our case > here. Sure, it would be better to harden KDB against all of these special cases. But you can break things in all sorts of fun ways with a debugger, anyways. The point of this patch is that it's nonsense that a function named copy_to_kernel_nofault() does indeed fault in a trivial case like address < TASK_SIZE_MAX. Thanks, Omar
