On 02.09.24 17:26, Omar Sandoval wrote:
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.)
I assume the difference is mostly historically, because usually, when
modifying something (ftrace, live patch, kdb) you better know what you
want to modify actually exist and can be modified. IOW, you usually
read-before-weite.
In contrast, things like /proc/kcore, (I think) while limiting it to
sane addresses, might still read from areas where we remove entries from
the directmap (e.g., secretmem), I think.
Like, in a compiler, modifying a variable you didn't read before is
rather rare as well. If you would have tried to read it, the
copy_from_kernel_nofault() would have failed.
I agree that the difference is weird, and likely really "nobody ran into
this before in sane use cases".
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)/.
Ah, okay, that's x86 specifics detail in
copy_from_kernel_nofault_allowed(), thanks.
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.
Yes, because the write-without-read to kernel memory "you don't know
even exists" is rather ... weird :)
Anyhow, no strong opinion here. Patches look simple enough.
--
Cheers,
David / dhildenb
