[uml-devel] [PATCH] fix writing into /dev/kmem

[uml-devel] [PATCH] fix writing into /dev/kmem

[Jeff Dike]

The patch below fixes a bug in the /dev/kmem driver which causes UML to
die if you write to it.  Of course, the kernel will die if you write random
crap to kernel memory, but at least it doesn't die in a UML-specific way.

This is a cut'n'paste from an xterm, so apply with patch -l.

				Jeff

--- drivers/char/mem.c~ 2004-01-05 11:23:32.000000000 -0500
+++ drivers/char/mem.c  2004-02-27 08:52:14.000000000 -0500
@@ -287,6 +287,7 @@
        ssize_t virtr = 0;
        char * kbuf; /* k-addr because vwrite() takes vmlist_lock rwlock */
 
+       p = (unsigned long) __va(p);
        if (p < (unsigned long) high_memory) {
                wrote = count;
                if (count > (unsigned long) high_memory - p)
@@ -321,7 +322,7 @@
                free_page((unsigned long)kbuf);
        }
 
-       *ppos = p;
+       *ppos = __pa((void *) p);
        return virtr + wrote;
 }


-------------------------------------------------------
SF.Net is sponsored by: Speed Start Your Linux Apps Now.
Build and deploy apps & Web services for Linux with
a free DVD software kit from IBM. Click Now!
http://ads.osdn.com/?ad_id=1356&alloc_id=3438&op=click
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Re: [uml-devel] [PATCH] fix writing into /dev/kmem

[BlaisorBlade]

Alle 03:36, domenica 29 febbraio 2004, Jeff Dike ha scritto:
> The patch below fixes a bug in the /dev/kmem driver which causes UML to
> die if you write to it.  Of course, the kernel will die if you write random
> crap to kernel memory, but at least it doesn't die in a UML-specific way.

> This is a cut'n'paste from an xterm, so apply with patch -l.

This patch is like the one for the read from /dev/kmem (i.e. the one fixing 
the panic when one does "cat /dev/kmem": you can reproduce this panic on 
Uml2.6): it makes it behave like /dev/mem (man 4 mem is indeed helpful!). So 
it is basically not *the right way*.

Also, someone got a panic with the read patch applied while klogd was reading 
something. The diagnosys was that the /dev/mem driver checks for overflows, 
while this patched /dev/kmem driver does not. To read the physical address 
0x1with /dev/kmem, you are supposed to read 0xc0000001; so with that patch 
any proper user of /dev/kmem will overflow p.

So, I propose to either return -EFAULT with not accessible addresses (which is 
the proper fix), or to (at least and as a workaround) correct the "fops" 
fields of /dev/kmem to redirect it on /dev/mem (at least this avoids the 
crash better than the current patches).
-- 
Paolo Giarrusso, aka Blaisorblade
Linux registered user n. 292729



-------------------------------------------------------
SF.Net is sponsored by: Speed Start Your Linux Apps Now.
Build and deploy apps & Web services for Linux with
a free DVD software kit from IBM. Click Now!
http://ads.osdn.com/?ad_id=1356&alloc_id=3438&op=click
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Re: [uml-devel] [PATCH] fix writing into /dev/kmem

[Jeff Dike]

blaisorblade_spam@yahoo.it said:
> This patch is like the one for the read from /dev/kmem (i.e. the one
> fixing  the panic when one does "cat /dev/kmem": you can reproduce
> this panic on  Uml2.6): it makes it behave like /dev/mem (man 4 mem is
> indeed helpful!). So  it is basically not *the right way*. 

Well, if you look at read_kmem, the thing that it reads is physical memory
plus kernel virtual memory.  I.e. /dev/kmem is /dev/mem + the kernel virtual
memory areas.

Addmittedly, I don't see how x86 gets away with dereferencing address 0, but
I don't see what's wrong with my fixes.  The file offsets being passed in
are effectively physical addresses, which need to be converted into virtual
addresses before being dereferenced, with is what __va() does, and __pa()
undoes.

Also, read_mem does the _va() correctly:
	if (copy_to_user(buf, __va(p), count))
		return -EFAULT;

while read_kmem does not:
	if (copy_to_user(buf, (char *)p, read))
		return -EFAULT;

The only out that I can see is that readers of kmem are supposed to pass in 
kernel virtual addresses, 0xc0000000 and up.  But, if so, those guys
fundamentally aren't going to work on UML because those addresses will be 
wrong.

It would be nice to see exactly what klogd is doing.

> To read the physical address  0x1with /dev/kmem, you are supposed to
> read 0xc0000001; so with that patch  any proper user of /dev/kmem will
> overflow p.

0xa0000001, actually, which is what __va does.

				Jeff



-------------------------------------------------------
SF.Net is sponsored by: Speed Start Your Linux Apps Now.
Build and deploy apps & Web services for Linux with
a free DVD software kit from IBM. Click Now!
http://ads.osdn.com/?ad_id=1356&alloc_id=3438&op=click
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Re: [uml-devel] [PATCH] fix writing into /dev/kmem

[BlaisorBlade]

Alle 02:31, martedì 2 marzo 2004, Jeff Dike ha scritto:
> blaisorblade_spam@yahoo.it said:
> > This patch is like the one for the read from /dev/kmem (i.e. the one
> > fixing  the panic when one does "cat /dev/kmem": you can reproduce
> > this panic on  Uml2.6): it makes it behave like /dev/mem (man 4 mem is
> > indeed helpful!). So  it is basically not *the right way*.
>
> Well, if you look at read_kmem, the thing that it reads is physical memory
> plus kernel virtual memory.  I.e. /dev/kmem is /dev/mem + the kernel
> virtual memory areas.

No, I do not agree (and man 4 mem agrees with me; I've quoted it below). As 
you later say after reading the code (which contains The Truth), the address 
passed to /dev/kmem as already been __va'ed. I.e., if those devices were 
array we could say:

 /dev/kmem[ __va (address) ] == /dev/mem [ address ];

> Addmittedly, I don't see how x86 gets away with dereferencing address 0

Well, physical address 0 does exist, for /dev/mem; /dev/kmem instead returns 
-EFAULT (try cat /dev/kmem), because copy_to_user returns a fault state.
Actually, 0 is derefereced, and the CPU throws an exception; but thanks to the 
__extable section and "search_exception_table", that exception is "caught". 
Read Documentation/exception.txt. I.e., copy_to_user actually checks both the 
"to" and "from" addresses.

Instead, in UML skas mode, possibly because the process VMA's are not mapped 
in the kernel thread, the page tables for the user are checked by hand (by 
um_virt_to_phys); but the check is done only for the userspace address (i.e. 
the "to" address for copy_to_user; everything applies even to copy_from_user, 
of course).

In tt mode it uses actually exception, even if they are coded by hand without 
using __extable (I see a flag here, probably set while catching SIGSEGV).

So, probably (to check), the kernel panic would happen only in tt mode.

Anyway, here comes that copy_{to,from}_user are buggy. Since fixing those 
would have a good performance hit (to test, but that will probably be 
sensitive; Documentation/exception.txt says exactly this, because most 
programs have no bug so checking the page tables has a big cost) and klogd 
would not work anyway, add an XXX comment, add it on your TODO and make 
/dev/kmem always return -EFAULT, as a workaround; or make it check the 
address with maybe_map (and #ifdef the change; even other archs have 
arch-specific code there).

> but I don't see what's wrong with my fixes.
[From below]
> The only out that I can see is that readers of kmem are supposed to pass in
> kernel virtual addresses, 0xc0000000 and up.
Yes, this is the reality.

So, __va(p) will overflow. That has *actually* happened (in fact I studied 
what I say in these mails when someone got a fault if he loaded the LVM 
modules). In fact, in the read_mem there is a check against the overflow:
       if (p >= end_mem)
                return 0;

Well, I said you to read mem(4), you didn't, so I quote:

       Mem  is  a character device file that is an image of the main memory of
       the computer.  It may be used, for example, to examine (and even patch)
       the system.

       Byte  addresses  in  mem  are interpreted as *physical* memory 
addresses.
[...]
       The file kmem is the same as mem, except that the kernel virtual memory
       rather than physical memory is accessed.

In fact, see this:

# cat /dev/kmem
cat: /dev/kmem: Bad address

> > To read the physical address  0x1with /dev/kmem, you are supposed to
> > read 0xc0000001; so with that patch  any proper user of /dev/kmem will
> > overflow p.
>
> 0xa0000001, actually, which is what __va does.

Well, on *i386* that is 0xc0000001; since klogd is built with i386 kernel 
headers, it will use those.
-- 
Paolo Giarrusso, aka Blaisorblade
Linux registered user n. 292729



-------------------------------------------------------
SF.Net is sponsored by: Speed Start Your Linux Apps Now.
Build and deploy apps & Web services for Linux with
a free DVD software kit from IBM. Click Now!
http://ads.osdn.com/?ad_id\x1356&alloc_id438&opÌk
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Re: [uml-devel] [PATCH] fix writing into /dev/kmem

[Jeff Dike]

blaisorblade_spam@yahoo.it said:
> Well, on *i386* that is 0xc0000001; since klogd is built with i386
> kernel  headers, it will use those. 

This was the stupidity that I was refusing to believe in.  This means that
if you take a normal klogd and move it to a 2G/2G system, where the kernel
starts at 0x80000000, it won't work.

> Anyway, here comes that copy_{to,from}_user are buggy. Since fixing
> those  would have a good performance hit (to test, but that will
> probably be  sensitive

In skas mode, I wasn't expecting any legitimate unfixable kernel mode access
faults, so I wasn't setting up the fault_catcher.  So, what I did was add
a wrapper around the guts of copy_user which does a setjmp, and returns
whether it got longjmp-ed back to.

Reading /dev/kmem now produces -EFAULT.  Writing to it still causes a hang
because write_kmem doesn't check the return value of do_write_mem.  With that
fixed, everything produces -EFAULT.

The fix will be in the next patch.

				Jeff



-------------------------------------------------------
This SF.Net email is sponsored by: IBM Linux Tutorials
Free Linux tutorial presented by Daniel Robbins, President and CEO of
GenToo technologies. Learn everything from fundamentals to system
administration.http://ads.osdn.com/?ad_id=1470&alloc_id=3638&op=click
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Re: [uml-devel] [PATCH] fix writing into /dev/kmem

[BlaisorBlade]

Alle 03:02, giovedì 4 marzo 2004, Jeff Dike ha scritto:
> blaisorblade_spam@yahoo.it said:
> > Well, on *i386* that is 0xc0000001; since klogd is built with i386
> > kernel  headers, it will use those.
>
> This was the stupidity that I was refusing to believe in.  This means that
> if you take a normal klogd and move it to a 2G/2G system, where the kernel
> starts at 0x80000000, it won't work.

Well, that can be stupid (and is), but needing to recompile iptables when 
upgrading kernel from 2.4.21-Mandrake (i.e. 2.4.21-rc#) to 2.4.21 is even 
more stupid, right? However that was real.

> > Anyway, here comes that copy_{to,from}_user are buggy. Since fixing
> > those  would have a good performance hit (to test, but that will
> > probably be  sensitive
>
> In skas mode, I wasn't expecting any legitimate unfixable kernel mode
> access faults, so I wasn't setting up the fault_catcher.  So, what I did
> was add a wrapper around the guts of copy_user which does a setjmp, and
> returns whether it got longjmp-ed back to.

Yes, fault_catcher (i.e. memory exceptions) works for kernel addresses at very 
little cost. Not for userspace addresses, sadly.

> Reading /dev/kmem now produces -EFAULT.

Well, if you only changed copy_user, there should be some working addresses to 
read there, right? Even if klogd would need recompilation to handle it right.

In the case /dev/kmem does not work at all we could live with it, but it is a 
bit unclean.

> Writing to it still causes a hang
> because write_kmem doesn't check the return value of do_write_mem.  With
> that fixed, everything produces -EFAULT.

I'm sending the return check fix to LKML+ Andrew Morton, together with some 
cleanup for the unused params of do_write_mem.
-- 
Paolo Giarrusso, aka Blaisorblade
Linux registered user n. 292729



-------------------------------------------------------
This SF.Net email is sponsored by: IBM Linux Tutorials
Free Linux tutorial presented by Daniel Robbins, President and CEO of
GenToo technologies. Learn everything from fundamentals to system
administration.http://ads.osdn.com/?ad_id\x1470&alloc_id638&opÌk
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Re: [uml-devel] [PATCH] fix writing into /dev/kmem

[BlaisorBlade]

[-- Attachment #1: Type: text/plain, Size: 1218 bytes --]

Alle 03:36, domenica 29 febbraio 2004, Jeff Dike ha scritto:
> The patch below fixes a bug in the /dev/kmem driver which causes UML to
> die if you write to it.

Also, I forgot one thing, about this Oops:

the i386 Oopses, normally, kill only the thread which triggered the fault - 
while instead Uml panics altogether. Cannot this be changed at 
arch/um/kernel/trap_kern.c:segv()?

        if(!is_user)
-                panic("Kernel mode fault at addr 0x%lx, ip 0x%lx",
-                      address, ip);
+		 die("Oops".....) //some printk calls... can be omitted
+		 do_exit(SIGKILL);

like arch/i386/mm/fault.c: do_page_fault() ?
With the above change, cat /dev/kmem could only create an Oops on the cat 
process (and possibly some deadlock, but better than a straight crash).

By the way: in handle_page_fault(), there is some dead code (attached is the 
removal patch).

I also do not understand the loop around handle_mm_fault call, but I could be 
wrong: are you sure that after a successful handle_mm_fault (i.e. a major or 
minor fault) pte_present(*pte) could be false? If handle_mm_fault is not 
successful, we exit the loop anyway.

-- 
Paolo Giarrusso, aka Blaisorblade
Linux registered user n. 292729

[-- Attachment #2: Dead-trap-code.patch --]
[-- Type: text/x-diff, Size: 444 bytes --]

--- ./arch/um/kernel/trap_kern.c.fix	2004-02-20 18:43:26.000000000 +0100
+++ ./arch/um/kernel/trap_kern.c	2004-02-29 17:12:49.000000000 +0100
@@ -74,13 +74,7 @@
 			err = -ENOMEM;
 			goto out_of_memory;
 		default:
-			if (current->pid == 1) {
-				up_read(&mm->mmap_sem);
-				yield();
-				down_read(&mm->mmap_sem);
-				goto survive;
-			}
-			goto out;
+			BUG();
 		}
 		pte = pte_offset_kernel(pmd, page);
 	} while(!pte_present(*pte));

Re: [uml-devel] [PATCH] fix writing into /dev/kmem

[Jeff Dike]

blaisorblade_spam@yahoo.it said:
> the i386 Oopses, normally, kill only the thread which triggered the
> fault -  while instead Uml panics altogether. Cannot this be changed
> at  arch/um/kernel/trap_kern.c:segv()? 

If there's an unexplained kernel mode access fault, anything could be wrong.

I don't see that it's reasonable to just kill the process when kernel data
could be corrupted as far as we know.

I don't know why x86 thinks it can just kill the process.

> By the way: in handle_page_fault(), there is some dead code (attached
> is the  removal patch).

That shouldn't be dead code.  I think it should be attached to the VM_FAULT_OOM
case - I copied it from x86, so check there to be sure.

> I also do not understand the loop around handle_mm_fault call, but I
> could be  wrong: are you sure that after a successful handle_mm_fault
> (i.e. a major or  minor fault) pte_present(*pte) could be false? If
> handle_mm_fault is not  successful, we exit the loop anyway. 

It's possible that the page could have been swapped out between fixing the
fault and the return from handle_mm_fault.  Way back in history, I had a 
check for the same fault happening twice in a row, and panicing if it happened.
This was when UML's fault handling still needed debugging.  To prevent the
case I just mentioned from triggering that panic, I added the loop to make sure
the page was good before returning to userspace.

				Jeff



-------------------------------------------------------
SF.Net is sponsored by: Speed Start Your Linux Apps Now.
Build and deploy apps & Web services for Linux with
a free DVD software kit from IBM. Click Now!
http://ads.osdn.com/?ad_id=1356&alloc_id=3438&op=click
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel

Re: [uml-devel] [PATCH] fix writing into /dev/kmem

[Jeff Dike]

-------------------------------------------------------
SF.Net is sponsored by: Speed Start Your Linux Apps Now.
Build and deploy apps & Web services for Linux with
a free DVD software kit from IBM. Click Now!
http://ads.osdn.com/?ad_id=1356&alloc_id=3438&op=click
_______________________________________________
User-mode-linux-devel mailing list
User-mode-linux-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/user-mode-linux-devel