[question] What's the difference between /dev/kmem and /dev/mem

[question] What's the difference between /dev/kmem and /dev/mem

[Steven Rostedt]

OK, I thought I use to know this. But what is the difference
between /dev/kmem and /dev/mem.  I thought that with /dev/kmem you could
use the actual kernel addresses to read from. 

For example, if I wanted to read the current variable X in the kernel, I
could look up the address of X in System.map, then mmaping to /dev/kmem
I could get to that variable using the address that I got from
System.map.  But this doesn't seem to work.

I'm getting an IO error on read. And looking at this I see:


static int mmap_kmem(struct file * file, struct vm_area_struct * vma)
{
        unsigned long long val;
	/*
	 * RED-PEN: on some architectures there is more mapped memory
	 * than available in mem_map which pfn_valid checks
	 * for. Perhaps should add a new macro here.
	 *
	 * RED-PEN: vmalloc is not supported right now.
	 */
	if (!pfn_valid(vma->vm_pgoff))
		return -EIO;
	val = (u64)vma->vm_pgoff << PAGE_SHIFT;
	vma->vm_pgoff = __pa(val) >> PAGE_SHIFT;
	return mmap_mem(file, vma);
}

I printed out the value in vma->vm_pgoff, and it still has the
0xc0000000 (but shifted >> 12). Isn't this suppose to also remove the
0xc?  Or am I just totally off here? 

Thanks,

-- Steve

[PATCH] Fix mmap_kmem (was: [question] What's the difference between /dev/kmem and /dev/mem)

[Steven Rostedt]

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

On Thu, 2005-08-11 at 17:36 -0400, Steven Rostedt wrote:
> OK, I thought I use to know this. But what is the difference
> between /dev/kmem and /dev/mem.  I thought that with /dev/kmem you could
> use the actual kernel addresses to read from. 
> 
> For example, if I wanted to read the current variable X in the kernel, I
> could look up the address of X in System.map, then mmaping to /dev/kmem
> I could get to that variable using the address that I got from
> System.map.  But this doesn't seem to work.
> 
> I'm getting an IO error on read. And looking at this I see:
> 
> 
> static int mmap_kmem(struct file * file, struct vm_area_struct * vma)
> {
>         unsigned long long val;
> 	/*
> 	 * RED-PEN: on some architectures there is more mapped memory
> 	 * than available in mem_map which pfn_valid checks
> 	 * for. Perhaps should add a new macro here.
> 	 *
> 	 * RED-PEN: vmalloc is not supported right now.
> 	 */
> 	if (!pfn_valid(vma->vm_pgoff))
> 		return -EIO;
> 	val = (u64)vma->vm_pgoff << PAGE_SHIFT;
> 	vma->vm_pgoff = __pa(val) >> PAGE_SHIFT;
> 	return mmap_mem(file, vma);
> }
> 
> I printed out the value in vma->vm_pgoff, and it still has the
> 0xc0000000 (but shifted >> 12). Isn't this suppose to also remove the
> 0xc?  Or am I just totally off here? 
> 
> Thanks,
> 
> -- Steve
> 


Found the problem.  It is a bug with mmap_kmem.  The order of checks is
wrong, so here's the patch.  Attached is a little program that reads the
System map looking for the variable modprobe_path.  If it finds it, then
it opens /dev/kmem for read only and mmaping it to read the contents of
modprobe_path.

Without this fix I get:

# ./tmap /boot/System.map
found modprobe_path at (0xc03647e0) c03647e0
mmap: Input/output error

On a machine with the patch, I now get:

# ./tmap /boot/System.map
found modprobe_path at (0xc03aa900) c03aa900
/sbin/modprobe

Note that the attached program does not handle the case of the string
crossing over a page.

-- Steve

Here's the simple patch:

Signed-off-by: Steven Rostedt <rostedt@goodmis.org>

--- linux-2.6.13-rc6-git1/drivers/char/mem.c.orig	2005-08-11 20:48:34.000000000 -0400
+++ linux-2.6.13-rc6-git1/drivers/char/mem.c	2005-08-11 20:48:48.000000000 -0400
@@ -269,10 +269,10 @@ static int mmap_kmem(struct file * file,
 	 *
 	 * RED-PEN: vmalloc is not supported right now.
 	 */
-	if (!pfn_valid(vma->vm_pgoff))
-		return -EIO;
 	val = (u64)vma->vm_pgoff << PAGE_SHIFT;
 	vma->vm_pgoff = __pa(val) >> PAGE_SHIFT;
+	if (!pfn_valid(vma->vm_pgoff))
+		return -EIO;
 	return mmap_mem(file, vma);
 }
 


[-- Attachment #2: tmap.c --]
[-- Type: text/x-csrc, Size: 1331 bytes --]

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <stdarg.h>
#include <fcntl.h>
#include <unistd.h>
#include <errno.h>

#include <sys/types.h>
#include <sys/stat.h>
#include <sys/poll.h>
#include <sys/mman.h>

int page_size;
#define PAGE_SIZE page_size
#define PAGE_MASK (~(PAGE_SIZE-1))

void get_var (unsigned long addr) {
	off_t ptr = addr & ~(PAGE_MASK);
	off_t offset = addr & PAGE_MASK;
	int i = 0;
	char *map;
	static int kfd = -1;

	kfd = open("/dev/kmem",O_RDONLY);
	if (kfd < 0) {
		perror("open");
		exit(0);
	}

	map = mmap(NULL,PAGE_SIZE,PROT_READ,MAP_SHARED,kfd,offset);
	if (map == MAP_FAILED) {
		perror("mmap");
		exit(-1);
	}
	printf("%s\n",map+ptr);

	return;
}

int main(int argc, char **argv)
{
	FILE *fp;
	char addr_str[11]="0x";
	char var[51];
	unsigned long addr;
	char ch;
	int r;
	
	if (argc != 2) {
		fprintf(stderr,"usage: %s System.map\n",argv[0]);
		exit(-1);
	}

	if ((fp = fopen(argv[1],"r")) == NULL) {
		perror("fopen");
		exit(-1);
	}

	do {
		r = fscanf(fp,"%8s %c %50s\n",&addr_str[2],&ch,var);
		if (strcmp(var,"modprobe_path")==0)
			break;
	} while(r > 0);
	if (r < 0) {
		printf("could not find modprobe_path\n");
		exit(-1);
	}
	page_size = getpagesize();
	addr = strtoul(addr_str,NULL,16);
	printf("found modprobe_path at (%s) %08lx\n",addr_str,addr);
	get_var(addr);
}

Re: [PATCH] Fix mmap_kmem (was: [question] What's the difference between /dev/kmem and /dev/mem)

[Hugh Dickins]

On Thu, 11 Aug 2005, Steven Rostedt wrote:
> 
> Found the problem.  It is a bug with mmap_kmem.  The order of checks is
> wrong, so here's the patch.
> -	if (!pfn_valid(vma->vm_pgoff))
> -		return -EIO;
>  	val = (u64)vma->vm_pgoff << PAGE_SHIFT;
>  	vma->vm_pgoff = __pa(val) >> PAGE_SHIFT;
> +	if (!pfn_valid(vma->vm_pgoff))
> +		return -EIO;
>  	return mmap_mem(file, vma);

Good find, looks right to me, so far as it goes (why does this check
pfn_valid just on the first? and remap_pfn_range will not behave as
you'd expect on most of kmem, not before Nick kills PageReserved;
and there's the red-penned issue of vmalloc'ed areas too).

Perhaps you're the first to mmap /dev/kmem: before those 2.6.11 changes,
going back beyond 2.4.0, it seems to have expected to caller to subtract
PAGE_OFFSET from the virtual address to give the file offset (when doing
mmap, but not when doing read/write - senseless, especially given the
variable behaviour of lseek to negative offset before the read/write).

Hugh

Re: [PATCH] Fix mmap_kmem (was: [question] What's the difference between /dev/kmem and /dev/mem)

[Linus Torvalds]

On Thu, 11 Aug 2005, Steven Rostedt wrote:
> 
> Found the problem.  It is a bug with mmap_kmem.  The order of checks is
> wrong, so here's the patch.  Attached is a little program that reads the
> System map looking for the variable modprobe_path.  If it finds it, then
> it opens /dev/kmem for read only and mmaping it to read the contents of
> modprobe_path.

I'm actually more inclined to try to deprecate /dev/kmem.. I don't think 
anybody has ever really used it except for some rootkits. It only exists 
in the first place because it's historical.

We do need to support /dev/mem for X, but even that might go away some 
day. 

So I'd be perfectly happy to fix this, but I'd be even happier if we made 
the whole kmem thing a config variable (maybe even default it to "off").

		Linus

Re: [PATCH] Fix mmap_kmem (was: [question] What's the difference between /dev/kmem and /dev/mem)

[Andi Kleen]

Linus Torvalds <torvalds@osdl.org> writes:
> 
> I'm actually more inclined to try to deprecate /dev/kmem.. I don't think 
> anybody has ever really used it except for some rootkits. 

I don't think that's true.
 
> So I'd be perfectly happy to fix this, but I'd be even happier if we made 
> the whole kmem thing a config variable (maybe even default it to "off").

Acessing vmalloc in /dev/mem would be pretty awkward. Yes it doesn't
also work in mmap of /dev/kmem, but at least in read/write.
There are quite a lot of scripts that use it for kernel debugging
like dumping variables. And for that you really want to access modules
and vmalloc. And it's much easier to parse than /proc/kcore

In fact I have some patches queued to fix it for x86-64 again
(people who used such scripts on i386 are complaining) 

-Andi

Re: [PATCH] Fix mmap_kmem (was: [question] What's the difference between /dev/kmem and /dev/mem)

[Dave Jones]

On Fri, Aug 12, 2005 at 09:35:03AM -0700, Linus Torvalds wrote:

 > On Thu, 11 Aug 2005, Steven Rostedt wrote:
 > > 
 > > Found the problem.  It is a bug with mmap_kmem.  The order of checks is
 > > wrong, so here's the patch.  Attached is a little program that reads the
 > > System map looking for the variable modprobe_path.  If it finds it, then
 > > it opens /dev/kmem for read only and mmaping it to read the contents of
 > > modprobe_path.
 > 
 > I'm actually more inclined to try to deprecate /dev/kmem.. I don't think 
 > anybody has ever really used it except for some rootkits. It only exists 
 > in the first place because it's historical.

We've had it disabled in Fedora for a long time, maybe as far
back as FC2, for exactly this reason.  The only things that broke,
were things that needed fixing anyway. (Something like gdm was
reading /dev/mem to get a source of random numbers of all things).

 > We do need to support /dev/mem for X, but even that might go away some 
 > day. 

We also restrict /dev/mem to be a 'need to know' basis. Trying
to read from certain regions of memory will fail.
Again, nothing that wasn't already broken broke with this change.

 > So I'd be perfectly happy to fix this, but I'd be even happier if we made 
 > the whole kmem thing a config variable (maybe even default it to "off").

The above patches were in -mm for a while, though they didn't
have a config option, they just 'did it', and some of the
changes were a bit unclean, but I can polish that up if you're
interested.

		Dave

Re: [PATCH] Fix mmap_kmem (was: [question] What's the difference between /dev/kmem and /dev/mem)

[Steven Rostedt]

On Fri, 12 Aug 2005, Linus Torvalds wrote:
>
> I'm actually more inclined to try to deprecate /dev/kmem.. I don't think
> anybody has ever really used it except for some rootkits. It only exists
> in the first place because it's historical.
>
> We do need to support /dev/mem for X, but even that might go away some
> day.
>
> So I'd be perfectly happy to fix this, but I'd be even happier if we made
> the whole kmem thing a config variable (maybe even default it to "off").
>

I don't mind it as a config option, but please don't deprecate it.  I
use it a lot (actually I've been using /dev/mem until now, that kmem seems
better) for debug tools that I write.  That is, user land programs to
monitor various parts of the kernel.

Thanks,

-- Steve

Re: [PATCH] Fix mmap_kmem (was: [question] What's the difference between /dev/kmem and /dev/mem)

[Steven Rostedt]

On Fri, 12 Aug 2005, Dave Jones wrote:
>
> The above patches were in -mm for a while, though they didn't
> have a config option, they just 'did it', and some of the
> changes were a bit unclean, but I can polish that up if you're
> interested.

Again, I'm asking to have it turned into a config option. Even default to
off. I understand that /dev/kmem shouldn't be in a production machine.
There's no reason for it.  But it is nice to have when debugging the
kernel.  I'll make the changes if need be, to make this into a config
option (placed in the kernel debug section).  I'll even maintain it to
keep it working.  But I don't want yet another thing I need to write
myself for debugging the kernel.

Thanks,

-- Steve

Re: [PATCH] Fix mmap_kmem (was: [question] What's the difference between /dev/kmem and /dev/mem)

[Arjan van de Ven]

On Fri, 2005-08-12 at 13:07 -0400, Steven Rostedt wrote:
> 
> On Fri, 12 Aug 2005, Dave Jones wrote:
> >
> > The above patches were in -mm for a while, though they didn't
> > have a config option, they just 'did it', and some of the
> > changes were a bit unclean, but I can polish that up if you're
> > interested.
> 
> Again, I'm asking to have it turned into a config option. Even default to
> off. I understand that /dev/kmem shouldn't be in a production machine.
> There's no reason for it.  But it is nice to have when debugging the
> kernel.  I'll make the changes if need be, to make this into a config
> option (placed in the kernel debug section).  I'll even maintain it to
> keep it working.  But I don't want yet another thing I need to write
> myself for debugging the kernel.

and /proc/kcore doesn't cut it for you?

Re: [PATCH] Fix mmap_kmem (was: [question] What's the difference between /dev/kmem and /dev/mem)

[Steven Rostedt]

On Fri, 12 Aug 2005, Arjan van de Ven wrote:
> > Again, I'm asking to have it turned into a config option. Even default to
> > off. I understand that /dev/kmem shouldn't be in a production machine.
> > There's no reason for it.  But it is nice to have when debugging the
> > kernel.  I'll make the changes if need be, to make this into a config
> > option (placed in the kernel debug section).  I'll even maintain it to
> > keep it working.  But I don't want yet another thing I need to write
> > myself for debugging the kernel.
>
> and /proc/kcore doesn't cut it for you?

I don't know.  Can I mmap kcore, set up a function that I want to try out
before putting in the kernel, have it modify the variables that are
mmapped, and see if all works?

Also, I've only used /proc/kcore with gdb, I never wrote my own core
parsing programs.

I like to do some strange things when adding stuff to the kernel, and
mapping /dev/mem to get to kernel globals from user space was one of
them.  I didn't just monitor, I did modify things as well.  But this was
all for R&D and testing. Never for the final product, so that's why I
don't mind a config option, even under kernel debug.

-- Steve

Re: [PATCH] Fix mmap_kmem (was: [question] What's the difference between /dev/kmem and /dev/mem)

[Arjan van de Ven]

On Fri, 2005-08-12 at 18:54 +0200, Andi Kleen wrote:
> Linus Torvalds <torvalds@osdl.org> writes:
> > 
> > I'm actually more inclined to try to deprecate /dev/kmem.. I don't think 
> > anybody has ever really used it except for some rootkits. 
> 
> I don't think that's true.

got any examples ?

>  
> > So I'd be perfectly happy to fix this, but I'd be even happier if we made 
> > the whole kmem thing a config variable (maybe even default it to "off").
> 
> Acessing vmalloc in /dev/mem would be pretty awkward. Yes it doesn't
> also work in mmap of /dev/kmem, but at least in read/write.
> There are quite a lot of scripts that use it for kernel debugging
> like dumping variables. And for that you really want to access modules
> and vmalloc. And it's much easier to parse than /proc/kcore

but you can stick gdb on /proc/kcore...

Re: [PATCH] Fix mmap_kmem (was: [question] What's the difference between /dev/kmem and /dev/mem)

[Andi Kleen]

On Fri, Aug 12, 2005 at 07:56:26PM +0200, Arjan van de Ven wrote:
> On Fri, 2005-08-12 at 18:54 +0200, Andi Kleen wrote:
> > Linus Torvalds <torvalds@osdl.org> writes:
> > > 
> > > I'm actually more inclined to try to deprecate /dev/kmem.. I don't think 
> > > anybody has ever really used it except for some rootkits. 
> > 
> > I don't think that's true.
> 
> got any examples ?

I wrote some hacks over the years, not sure it would be useful
to post them because they all were very special purpose. 

I know users are doing the same because they complain on x86-64
when it doesn't work.


> > > So I'd be perfectly happy to fix this, but I'd be even happier if we made 
> > > the whole kmem thing a config variable (maybe even default it to "off").
> > 
> > Acessing vmalloc in /dev/mem would be pretty awkward. Yes it doesn't
> > also work in mmap of /dev/kmem, but at least in read/write.
> > There are quite a lot of scripts that use it for kernel debugging
> > like dumping variables. And for that you really want to access modules
> > and vmalloc. And it's much easier to parse than /proc/kcore
> 
> but you can stick gdb on /proc/kcore...

That's much more complicated. Instead of a simple read you would
need to parse complex ASCII output. Also gdb normally doesn't
work with a single System.map or /proc/kallsyms. I know it could
be gotten to use that stuff, but that would be all very complicated.
Much more complicated than read/write on /dev/kmem.

-Andi

Re: [PATCH] Fix mmap_kmem (was: [question] What's the difference between /dev/kmem and /dev/mem)

[Ingo Oeser]

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

Hi Andi,

On Friday 12 August 2005 18:54, Andi Kleen wrote:
> Acessing vmalloc in /dev/mem would be pretty awkward. Yes it doesn't
> also work in mmap of /dev/kmem, but at least in read/write.
> There are quite a lot of scripts that use it for kernel debugging
> like dumping variables. And for that you really want to access modules
> and vmalloc. And it's much easier to parse than /proc/kcore

Perfect! So it should be under CONFIG_DEBUG_KERNEL and default to off.

So you can still debug and we raise the bar higher for rootkits, 
if they are the only other user.

Too simple?


Regards

Ingo Oeser


[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] Fix mmap_kmem (was: [question] What's the difference between /dev/kmem and /dev/mem)

[Andi Kleen]

> Perfect! So it should be under CONFIG_DEBUG_KERNEL and default to off.
> 
> So you can still debug and we raise the bar higher for rootkits, 
> if they are the only other user.
> 
> Too simple?

If you wanted to recompile your kernel to debug you could as well
add printks. But the whole point of kmem hacks is to avoid that
slow cycle.

-Andi

Re: [PATCH] Fix mmap kmem (was: [question] What's the difference between /dev/kmem and /dev/mem)

[Nicolas George]

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

Le quintidi 25 thermidor, an CCXIII, Linus Torvalds a écrit :
> We do need to support /dev/mem for X, but even that might go away some 
> day. 

If I may interfere, I would like to point another use of /dev/mem:
long-running memory tests that do not alter the service much. I have written
a small package which does such tests. The principle is to write
pseudo-random data in a page, and later read it back and compare it to what
it should be. For such a test, /dev/mem can be used two ways:

- to access memory pages that have been disabled by mem=/memmap= command
  lines arguments;

- to look for the physical address of an userland-allocated and mlock()ed
  page (by writing a known pattern in it).

(Well, I am not even sure that the physical address of a mlock()ed page can
not change.)

With the increasing cheapness but lack of reliability of noname RAM, such
tests become very useful for home and even professional usage. I could
easyly imagine a fully automated distribution that would enable such a test
tool for a month after the first installation, for example.

It can be noted that those tests do not require the full features of
/dev/mem, and especially not the security threatening ones. It would be
sufficient to have some sort of API to:

- map a particular physical page of memory in a process' address space,
  optionnally reallocating a previous usage of that page, and failing if it
  can not be done;

- ask the physical address of a maped page.

The second can be done with /proc/kcore, but it depends on internal
structures of the kernel. I believe it could easily be done with a
/proc/$PID/maps-like file.

The first could be implemented as a flag to mmap (maybe MAP_PHYSICAL) which
would cause the addr argument to be the physical address of the requested
page.

It may seem a lot of trouble for some very specific need, but I have
remarked that bad memory bits are an increasing problem, and ECC memory and
motherboards supporting it are not easy to find for non high-grade
professional hardware. A well-integrated software workaround would be really
useful I believe.

Regads,

-- 
  Nicolas George

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 185 bytes --]

Re: [PATCH] Fix mmap_kmem (was: [question] What's the difference between /dev/kmem and /dev/mem)

[Arjan van de Ven]

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

On Fri, 2005-08-12 at 09:35 -0700, Linus Torvalds wrote:
> 
> On Thu, 11 Aug 2005, Steven Rostedt wrote:
> > 
> > Found the problem.  It is a bug with mmap_kmem.  The order of checks is
> > wrong, so here's the patch.  Attached is a little program that reads the
> > System map looking for the variable modprobe_path.  If it finds it, then
> > it opens /dev/kmem for read only and mmaping it to read the contents of
> > modprobe_path.
> 
> I'm actually more inclined to try to deprecate /dev/kmem.. I don't think 
> anybody has ever really used it except for some rootkits. It only exists 
> in the first place because it's historical.
> 
> We do need to support /dev/mem for X, but even that might go away some 
> day. 
> 
> So I'd be perfectly happy to fix this, but I'd be even happier if we made 
> the whole kmem thing a config variable (maybe even default it to "off").

attached is a simple patch that does exactly this...


[-- Attachment #2: mem.patch --]
[-- Type: text/x-patch, Size: 3689 bytes --]

diff -purN linux-before/drivers/char/Kconfig linux-2.6.12/drivers/char/Kconfig
--- linux-before/drivers/char/Kconfig	2005-08-13 18:38:11.210821000 +0200
+++ linux-2.6.12/drivers/char/Kconfig	2005-08-13 18:44:31.182935339 +0200
@@ -1000,6 +1000,12 @@ config MMTIMER
 	  The mmtimer device allows direct userspace access to the
 	  Altix system timer.
 
+config DEVKMEM
+	tristate "Userspace access to virtual kernel memory (/dev/kmem)"
+	help
+	  The /dev/kmem device allows userspace access to kernel virtual
+	  memory, which can be useful for kernel debugging.
+
 source "drivers/char/tpm/Kconfig"
 
 endmenu
diff -purN linux-before/drivers/char/mem.c linux-2.6.12/drivers/char/mem.c
--- linux-before/drivers/char/mem.c	2005-08-13 18:38:11.261815000 +0200
+++ linux-2.6.12/drivers/char/mem.c	2005-08-13 18:41:09.002351297 +0200
@@ -259,22 +259,6 @@ static int mmap_mem(struct file * file, 
 	return 0;
 }
 
-static int mmap_kmem(struct file * file, struct vm_area_struct * vma)
-{
-        unsigned long long val;
-	/*
-	 * RED-PEN: on some architectures there is more mapped memory
-	 * than available in mem_map which pfn_valid checks
-	 * for. Perhaps should add a new macro here.
-	 *
-	 * RED-PEN: vmalloc is not supported right now.
-	 */
-	if (!pfn_valid(vma->vm_pgoff))
-		return -EIO;
-	val = (u64)vma->vm_pgoff << PAGE_SHIFT;
-	vma->vm_pgoff = __pa(val) >> PAGE_SHIFT;
-	return mmap_mem(file, vma);
-}
 
 #ifdef CONFIG_CRASH_DUMP
 /*
@@ -313,6 +297,25 @@ static ssize_t read_oldmem(struct file *
 extern long vread(char *buf, char *addr, unsigned long count);
 extern long vwrite(char *buf, char *addr, unsigned long count);
 
+#ifdef CONFIG_DEVKMEM
+
+static int mmap_kmem(struct file * file, struct vm_area_struct * vma)
+{
+        unsigned long long val;
+	/*
+	 * RED-PEN: on some architectures there is more mapped memory
+	 * than available in mem_map which pfn_valid checks
+	 * for. Perhaps should add a new macro here.
+	 *
+	 * RED-PEN: vmalloc is not supported right now.
+	 */
+	if (!pfn_valid(vma->vm_pgoff))
+		return -EIO;
+	val = (u64)vma->vm_pgoff << PAGE_SHIFT;
+	vma->vm_pgoff = __pa(val) >> PAGE_SHIFT;
+	return mmap_mem(file, vma);
+}
+
 /*
  * This function reads the *virtual* memory as seen by the kernel.
  */
@@ -521,6 +524,8 @@ static ssize_t write_kmem(struct file * 
  	return virtr + wrote;
 }
 
+#endif
+
 #if (defined(CONFIG_ISA) || !defined(__mc68000__)) && (!defined(CONFIG_PPC_ISERIES) || defined(CONFIG_PCI))
 static ssize_t read_port(struct file * file, char __user * buf,
 			 size_t count, loff_t *ppos)
@@ -768,6 +773,7 @@ static struct file_operations mem_fops =
 	.open		= open_mem,
 };
 
+#ifdef CONFIG_DEVKMEM
 static struct file_operations kmem_fops = {
 	.llseek		= memory_lseek,
 	.read		= read_kmem,
@@ -775,6 +781,7 @@ static struct file_operations kmem_fops 
 	.mmap		= mmap_kmem,
 	.open		= open_kmem,
 };
+#endif
 
 static struct file_operations null_fops = {
 	.llseek		= null_lseek,
@@ -843,9 +850,11 @@ static int memory_open(struct inode * in
 		case 1:
 			filp->f_op = &mem_fops;
 			break;
+#ifdef CONFIG_DEVKMEM
 		case 2:
 			filp->f_op = &kmem_fops;
 			break;
+#endif
 		case 3:
 			filp->f_op = &null_fops;
 			break;
@@ -894,7 +903,9 @@ static const struct {
 	struct file_operations	*fops;
 } devlist[] = { /* list of minor devices */
 	{1, "mem",     S_IRUSR | S_IWUSR | S_IRGRP, &mem_fops},
+#ifdef CONFIG_DEVKMEM
 	{2, "kmem",    S_IRUSR | S_IWUSR | S_IRGRP, &kmem_fops},
+#endif
 	{3, "null",    S_IRUGO | S_IWUGO,           &null_fops},
 #if (defined(CONFIG_ISA) || !defined(__mc68000__)) && (!defined(CONFIG_PPC_ISERIES) || defined(CONFIG_PCI))
 	{4, "port",    S_IRUSR | S_IWUSR | S_IRGRP, &port_fops},

Re: [PATCH] Fix mmap_kmem (was: [question] What's the difference between /dev/kmem and /dev/mem)

[Linus Torvalds]

On Sat, 13 Aug 2005, Arjan van de Ven wrote:
> > 
> > So I'd be perfectly happy to fix this, but I'd be even happier if we made 
> > the whole kmem thing a config variable (maybe even default it to "off").
> 
> attached is a simple patch that does exactly this...

Well, you should have fixed the bug that Steven had at the same time. Now 
his patch won't even apply any more, I suspect ;)

		Linus

Re: [PATCH] Fix mmap_kmem (was: [question] What's the difference between /dev/kmem and /dev/mem)

[Joshua Hudson]

On 8/13/05, Arjan van de Ven <arjan@infradead.org> wrote:
> On Fri, 2005-08-12 at 09:35 -0700, Linus Torvalds wrote:
> >
> > On Thu, 11 Aug 2005, Steven Rostedt wrote:
> > >
> > > Found the problem.  It is a bug with mmap_kmem.  The order of checks is
> > > wrong, so here's the patch.  Attached is a little program that reads the
> > > System map looking for the variable modprobe_path.  If it finds it, then
> > > it opens /dev/kmem for read only and mmaping it to read the contents of
> > > modprobe_path.
> >
> > I'm actually more inclined to try to deprecate /dev/kmem.. I don't think
> > anybody has ever really used it except for some rootkits. It only exists
> > in the first place because it's historical.
> >
> > We do need to support /dev/mem for X, but even that might go away some
> > day.
> >
> > So I'd be perfectly happy to fix this, but I'd be even happier if we made
> > the whole kmem thing a config variable (maybe even default it to "off").
> 
I believe rootkit detectors, as well as some versions of ps (wchan
field) use kmem.

Re: [PATCH] Fix mmap_kmem (was: [question] What's the difference between /dev/kmem and /dev/mem)

[Arjan van de Ven]

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

On Sat, 2005-08-13 at 09:56 -0700, Linus Torvalds wrote:
> 
> On Sat, 13 Aug 2005, Arjan van de Ven wrote:
> > > 
> > > So I'd be perfectly happy to fix this, but I'd be even happier if we made 
> > > the whole kmem thing a config variable (maybe even default it to "off").
> > 
> > attached is a simple patch that does exactly this...
> 
> Well, you should have fixed the bug that Steven had at the same time. Now 
> his patch won't even apply any more, I suspect ;)

Patch is forgiving ;)

attached is the same patch but now with Steven's change made as well


[-- Attachment #2: mem.patch --]
[-- Type: text/x-patch, Size: 3689 bytes --]

diff -purN linux-before/drivers/char/Kconfig linux-2.6.12/drivers/char/Kconfig
--- linux-before/drivers/char/Kconfig	2005-08-13 18:38:11.210821000 +0200
+++ linux-2.6.12/drivers/char/Kconfig	2005-08-13 18:44:31.182935339 +0200
@@ -1000,6 +1000,12 @@ config MMTIMER
 	  The mmtimer device allows direct userspace access to the
 	  Altix system timer.
 
+config DEVKMEM
+	tristate "Userspace access to virtual kernel memory (/dev/kmem)"
+	help
+	  The /dev/kmem device allows userspace access to kernel virtual
+	  memory, which can be useful for kernel debugging.
+
 source "drivers/char/tpm/Kconfig"
 
 endmenu
diff -purN linux-before/drivers/char/mem.c linux-2.6.12/drivers/char/mem.c
--- linux-before/drivers/char/mem.c	2005-08-13 18:38:11.261815000 +0200
+++ linux-2.6.12/drivers/char/mem.c	2005-08-13 18:41:09.002351297 +0200
@@ -259,22 +259,6 @@ static int mmap_mem(struct file * file, 
 	return 0;
 }
 
-static int mmap_kmem(struct file * file, struct vm_area_struct * vma)
-{
-        unsigned long long val;
-	/*
-	 * RED-PEN: on some architectures there is more mapped memory
-	 * than available in mem_map which pfn_valid checks
-	 * for. Perhaps should add a new macro here.
-	 *
-	 * RED-PEN: vmalloc is not supported right now.
-	 */
-	if (!pfn_valid(vma->vm_pgoff))
-		return -EIO;
-	val = (u64)vma->vm_pgoff << PAGE_SHIFT;
-	vma->vm_pgoff = __pa(val) >> PAGE_SHIFT;
-	return mmap_mem(file, vma);
-}
 
 #ifdef CONFIG_CRASH_DUMP
 /*
@@ -313,6 +297,25 @@ static ssize_t read_oldmem(struct file *
 extern long vread(char *buf, char *addr, unsigned long count);
 extern long vwrite(char *buf, char *addr, unsigned long count);
 
+#ifdef CONFIG_DEVKMEM
+
+static int mmap_kmem(struct file * file, struct vm_area_struct * vma)
+{
+        unsigned long long val;
+	/*
+	 * RED-PEN: on some architectures there is more mapped memory
+	 * than available in mem_map which pfn_valid checks
+	 * for. Perhaps should add a new macro here.
+	 *
+	 * RED-PEN: vmalloc is not supported right now.
+	 */
+	val = (u64)vma->vm_pgoff << PAGE_SHIFT;
+	vma->vm_pgoff = __pa(val) >> PAGE_SHIFT;
+	if (!pfn_valid(vma->vm_pgoff))
+		return -EIO;
+	return mmap_mem(file, vma);
+}
+
 /*
  * This function reads the *virtual* memory as seen by the kernel.
  */
@@ -521,6 +524,8 @@ static ssize_t write_kmem(struct file * 
  	return virtr + wrote;
 }
 
+#endif
+
 #if (defined(CONFIG_ISA) || !defined(__mc68000__)) && (!defined(CONFIG_PPC_ISERIES) || defined(CONFIG_PCI))
 static ssize_t read_port(struct file * file, char __user * buf,
 			 size_t count, loff_t *ppos)
@@ -768,6 +773,7 @@ static struct file_operations mem_fops =
 	.open		= open_mem,
 };
 
+#ifdef CONFIG_DEVKMEM
 static struct file_operations kmem_fops = {
 	.llseek		= memory_lseek,
 	.read		= read_kmem,
@@ -775,6 +781,7 @@ static struct file_operations kmem_fops 
 	.mmap		= mmap_kmem,
 	.open		= open_kmem,
 };
+#endif
 
 static struct file_operations null_fops = {
 	.llseek		= null_lseek,
@@ -843,9 +850,11 @@ static int memory_open(struct inode * in
 		case 1:
 			filp->f_op = &mem_fops;
 			break;
+#ifdef CONFIG_DEVKMEM
 		case 2:
 			filp->f_op = &kmem_fops;
 			break;
+#endif
 		case 3:
 			filp->f_op = &null_fops;
 			break;
@@ -894,7 +903,9 @@ static const struct {
 	struct file_operations	*fops;
 } devlist[] = { /* list of minor devices */
 	{1, "mem",     S_IRUSR | S_IWUSR | S_IRGRP, &mem_fops},
+#ifdef CONFIG_DEVKMEM
 	{2, "kmem",    S_IRUSR | S_IWUSR | S_IRGRP, &kmem_fops},
+#endif
 	{3, "null",    S_IRUGO | S_IWUGO,           &null_fops},
 #if (defined(CONFIG_ISA) || !defined(__mc68000__)) && (!defined(CONFIG_PPC_ISERIES) || defined(CONFIG_PCI))
 	{4, "port",    S_IRUSR | S_IWUSR | S_IRGRP, &port_fops},

Re: [PATCH] Fix mmap_kmem (was: [question] What's the difference between /dev/kmem and /dev/mem)

[Arjan van de Ven]

> I believe rootkit detectors, as well as some versions of ps (wchan
> field) use kmem.

ps doesn't use kmem, and besides, in 2.6 we export that directly in
proc. poking in /dev/mem or /dev/kmem is NOT something you "just do".
THere are lots of pitfalls, things like PCI space, memory sizes/holes,
cachability aliases etc etc that can ruin your breakfast if you
use /dev/[k]mem... 

Re: [PATCH] Fix mmap_kmem (was: [question] What's the difference between /dev/kmem and /dev/mem)

[Linus Torvalds]

On Sat, 13 Aug 2005, Arjan van de Ven wrote:
> 
> attached is the same patch but now with Steven's change made as well

Actually, the more I looked at that mmap_kmem() function, the less I liked 
it.  Let's get that sucker fixed better first. It's still not wonderful, 
but at least now it tries to verify the whole _range_ of the mapping.

Steven, does this "alternate mmap_kmem fix" patch work for you?

		Linus
----
diff --git a/drivers/char/mem.c b/drivers/char/mem.c
--- a/drivers/char/mem.c
+++ b/drivers/char/mem.c
@@ -261,7 +261,11 @@ static int mmap_mem(struct file * file, 
 
 static int mmap_kmem(struct file * file, struct vm_area_struct * vma)
 {
-        unsigned long long val;
+	unsigned long pfn, size;
+
+	/* Turn a kernel-virtual address into a physical page frame */
+	pfn = __pa((u64)vma->vm_pgoff << PAGE_SHIFT) >> PAGE_SHIFT;
+
 	/*
 	 * RED-PEN: on some architectures there is more mapped memory
 	 * than available in mem_map which pfn_valid checks
@@ -269,10 +273,14 @@ static int mmap_kmem(struct file * file,
 	 *
 	 * RED-PEN: vmalloc is not supported right now.
 	 */
-	if (!pfn_valid(vma->vm_pgoff))
+	if (!pfn_valid(pfn))
 		return -EIO;
-	val = (u64)vma->vm_pgoff << PAGE_SHIFT;
-	vma->vm_pgoff = __pa(val) >> PAGE_SHIFT;
+
+	size = (vma->vm_end - vma->vm_start) >> PAGE_SHIFT;
+	if (!pfn_valid(pfn + size - 1))
+		return -EIO;
+
+	vma->vm_pgoff = pfn;
 	return mmap_mem(file, vma);
 }
 

Re: [PATCH] Fix mmap_kmem (was: [question] What's the difference between /dev/kmem and /dev/mem)

[Arjan van de Ven]

On Sat, 2005-08-13 at 10:37 -0700, Linus Torvalds wrote:
> 
> On Sat, 13 Aug 2005, Arjan van de Ven wrote:
> > 
> > attached is the same patch but now with Steven's change made as well
> 
> Actually, the more I looked at that mmap_kmem() function, the less I liked 
> it.  Let's get that sucker fixed better first. It's still not wonderful, 
> but at least now it tries to verify the whole _range_ of the mapping.

actually if that is your goal this just isn't enough... assume the
situation of a 1 page "forbidden gap", if you mmap 3 pages with the gap
in the middle.... then the code you send still doesn't cope. At which
point... it gets messy...

Re: [PATCH] Fix mmap_kmem (was: [question] What's the difference between /dev/kmem and /dev/mem)

[Martin J. Bligh]

--Arjan van de Ven <arjan@infradead.org> wrote (on Saturday, August 13, 2005 18:50:10 +0200):

> On Fri, 2005-08-12 at 09:35 -0700, Linus Torvalds wrote:
>> 
>> On Thu, 11 Aug 2005, Steven Rostedt wrote:
>> > 
>> > Found the problem.  It is a bug with mmap_kmem.  The order of checks is
>> > wrong, so here's the patch.  Attached is a little program that reads the
>> > System map looking for the variable modprobe_path.  If it finds it, then
>> > it opens /dev/kmem for read only and mmaping it to read the contents of
>> > modprobe_path.
>> 
>> I'm actually more inclined to try to deprecate /dev/kmem.. I don't think 
>> anybody has ever really used it except for some rootkits. It only exists 
>> in the first place because it's historical.
>> 
>> We do need to support /dev/mem for X, but even that might go away some 
>> day. 
>> 
>> So I'd be perfectly happy to fix this, but I'd be even happier if we made 
>> the whole kmem thing a config variable (maybe even default it to "off").
> 
> attached is a simple patch that does exactly this...

Whilst there's no normal legitimite usage for it, it is useful for debugging.
One thing I often do is create a circular log buffer, then fish it back 
out by mmaping /dev/mem or /dev/kmem, and going by system.map offsets.
No, nobody could claim it was clean or elegant, but it *is* useful.
M.

Re: [PATCH] Fix mmap_kmem (was: [question] What's the difference between /dev/kmem and /dev/mem)

[Olaf Hering]

On Sat, Aug 13, 2005 at 10:37:03AM -0700, Linus Torvalds wrote:
> 
> 
> On Sat, 13 Aug 2005, Arjan van de Ven wrote:
> > 
> > attached is the same patch but now with Steven's change made as well
> 
> Actually, the more I looked at that mmap_kmem() function, the less I liked 
> it.  Let's get that sucker fixed better first. It's still not wonderful, 
> but at least now it tries to verify the whole _range_ of the mapping.
> 
> Steven, does this "alternate mmap_kmem fix" patch work for you?
> 
> 		Linus
> ----
> diff --git a/drivers/char/mem.c b/drivers/char/mem.c
> --- a/drivers/char/mem.c
> +++ b/drivers/char/mem.c
> @@ -261,7 +261,11 @@ static int mmap_mem(struct file * file, 
>  
>  static int mmap_kmem(struct file * file, struct vm_area_struct * vma)
>  {
> -        unsigned long long val;
> +	unsigned long pfn, size;
> +
> +	/* Turn a kernel-virtual address into a physical page frame */
> +	pfn = __pa((u64)vma->vm_pgoff << PAGE_SHIFT) >> PAGE_SHIFT;
> +

ARCH=um doesnt like your version, but mine.

drivers/char/mem.c:267: error: invalid operands to binary <<

        pfn = (__pa((u64)vma->vm_pgoff) << PAGE_SHIFT) >> PAGE_SHIFT;

Re: [PATCH] Fix mmap_kmem (was: [question] What's the difference between /dev/kmem and /dev/mem)

[Jeff Dike]

On Mon, Aug 15, 2005 at 09:33:07PM +0200, Olaf Hering wrote:
> ARCH=um doesnt like your version, but mine.
> 
> drivers/char/mem.c:267: error: invalid operands to binary <<
> 
>         pfn = (__pa((u64)vma->vm_pgoff) << PAGE_SHIFT) >> PAGE_SHIFT;

My page.h was missing some parens.  Try the patch below.

				Jeff

Index: linux-2.6.13-rc6/include/asm-um/page.h
===================================================================
--- linux-2.6.13-rc6.orig/include/asm-um/page.h	2005-08-15 16:57:55.000000000 -0400
+++ linux-2.6.13-rc6/include/asm-um/page.h	2005-08-15 17:16:57.000000000 -0400
@@ -104,8 +104,8 @@
  * casting is the right thing, but 32-bit UML can't have 64-bit virtual
  * addresses
  */
-#define __pa(virt) to_phys((void *) (unsigned long) virt)
-#define __va(phys) to_virt((unsigned long) phys)
+#define __pa(virt) to_phys((void *) (unsigned long) (virt))
+#define __va(phys) to_virt((unsigned long) (phys))
 
 #define page_to_pfn(page) ((page) - mem_map)
 #define pfn_to_page(pfn) (mem_map + (pfn))

Re: [PATCH] Fix mmap_kmem (was: [question] What's the difference between /dev/kmem and /dev/mem)

[Olaf Hering]

 On Mon, Aug 15, Jeff Dike wrote:

> On Mon, Aug 15, 2005 at 09:33:07PM +0200, Olaf Hering wrote:
> > ARCH=um doesnt like your version, but mine.
> > 
> > drivers/char/mem.c:267: error: invalid operands to binary <<
> > 
> >         pfn = (__pa((u64)vma->vm_pgoff) << PAGE_SHIFT) >> PAGE_SHIFT;
> 
> My page.h was missing some parens.  Try the patch below.

compiles ok now, I hope it works.

Re: [PATCH] Fix mmap_kmem (was: [question] What's the difference between /dev/kmem and /dev/mem)

[Linus Torvalds]

On Mon, 15 Aug 2005, Olaf Hering wrote:
> 
> ARCH=um doesnt like your version, but mine.

Yours is broken. As is arch-um.

The fix would _seem_ to be something like the appended. Can you verify?

		Linus
----
diff --git a/include/asm-um/page.h b/include/asm-um/page.h
--- a/include/asm-um/page.h
+++ b/include/asm-um/page.h
@@ -104,8 +104,8 @@ extern void *to_virt(unsigned long phys)
  * casting is the right thing, but 32-bit UML can't have 64-bit virtual
  * addresses
  */
-#define __pa(virt) to_phys((void *) (unsigned long) virt)
-#define __va(phys) to_virt((unsigned long) phys)
+#define __pa(virt) to_phys((void *) (unsigned long) (virt))
+#define __va(phys) to_virt((unsigned long) (phys))
 
 #define page_to_pfn(page) ((page) - mem_map)
 #define pfn_to_page(pfn) (mem_map + (pfn))

Re: [PATCH] Fix mmap_kmem (was: [question] What's the difference between /dev/kmem and /dev/mem)

[Steven Rostedt]

On Sat, 2005-08-13 at 10:37 -0700, Linus Torvalds wrote:
> 
> On Sat, 13 Aug 2005, Arjan van de Ven wrote:
> > 
> > attached is the same patch but now with Steven's change made as well
> 
> Actually, the more I looked at that mmap_kmem() function, the less I liked 
> it.  Let's get that sucker fixed better first. It's still not wonderful, 
> but at least now it tries to verify the whole _range_ of the mapping.
> 
> Steven, does this "alternate mmap_kmem fix" patch work for you?
> 

Sorry for the late reply, my wife's Grandmother just passed away a few
days ago (at 98 years old) and if I went within 6 feet of the computer
she would have killed me!

I just tried out the patch, and it worked fine for me.

Thanks,

-- Steve

Re: [PATCH] Fix mmap_kmem (was: [question] What's the difference between /dev/kmem and /dev/mem)

[Steven Rostedt]

On Mon, 2005-08-15 at 21:16 -0400, Steven Rostedt wrote:

> Sorry for the late reply, my wife's Grandmother just passed away a few
> days ago (at 98 years old) and if I went within 6 feet of the computer
> she would have killed me!

Just to clearify, "she" as in my wife would have killed me. Not her late
grandmother.

-- Steve

Re: [PATCH] Fix mmap_kmem (was: [question] What's the difference between /dev/kmem and /dev/mem)

[Linus Torvalds]

On Mon, 15 Aug 2005, Steven Rostedt wrote:

> On Mon, 2005-08-15 at 21:16 -0400, Steven Rostedt wrote:
> 
> > Sorry for the late reply, my wife's Grandmother just passed away a few
> > days ago (at 98 years old) and if I went within 6 feet of the computer
> > she would have killed me!
> 
> Just to clearify, "she" as in my wife would have killed me. Not her late
> grandmother.

Thanks for the clarification. We were starting to worry about your family.

		Linus

Re: [PATCH] Fix mmap_kmem (was: [question] What's the difference between /dev/kmem and /dev/mem)

[Greg Edwards]

On Sat, Aug 13, 2005 at 08:18:07PM +0200, Arjan van de Ven wrote:
| On Sat, 2005-08-13 at 10:37 -0700, Linus Torvalds wrote:
| > Actually, the more I looked at that mmap_kmem() function, the less I liked 
| > it.  Let's get that sucker fixed better first. It's still not wonderful, 
| > but at least now it tries to verify the whole _range_ of the mapping.
| 
| actually if that is your goal this just isn't enough... assume the
| situation of a 1 page "forbidden gap", if you mmap 3 pages with the gap
| in the middle.... then the code you send still doesn't cope. At which
| point... it gets messy...

mmap_mem suffers from a lack of proper checks as well.  For example, on
Altix page 0 of each node is reserved for prom and a read or write to it
will cause an MCA.  mmaping /dev/mem with offset 0 will nicely explode.
Would adding a pfn_valid test in mmap_mem be the best bet, or could we
consolidate the checks currently in mmap_kmem into mmap_mem?

Greg

Re: [PATCH] Fix mmap_kmem (was: [question] What's the difference between /dev/kmem and /dev/mem)

[Alan Cox]

On Maw, 2005-08-16 at 17:12 -0500, Greg Edwards wrote:
> mmap_mem suffers from a lack of proper checks as well.  For example, on
> Altix page 0 of each node is reserved for prom and a read or write to it
> will cause an MCA.  mmaping /dev/mem with offset 0 will nicely explode.
> Would adding a pfn_valid test in mmap_mem be the best bet, or could we
> consolidate the checks currently in mmap_kmem into mmap_mem?

If you use /dev/mem you should know what you are doing. Even with
"checks" dd if=/dev/zero of=/dev/mem will do bad things. Trapping
obviously bad cases is fine, but complete sanity checking may actually
be counter productive.

Re: [PATCH] Fix mmap_kmem (was: [question] What's the difference between /dev/kmem and /dev/mem)

[Steven Rostedt]

On Wed, 2005-08-17 at 00:33 +0100, Alan Cox wrote:
> 
> If you use /dev/mem you should know what you are doing. Even with
> "checks" dd if=/dev/zero of=/dev/mem will do bad things. Trapping
> obviously bad cases is fine, but complete sanity checking may actually
> be counter productive.
> 

Sometimes "dd if=/dev/zero of=/dev/mem" is helpful. When I was in Spain
for some time, I needed to transfer lots of pictures to my home machine.
But all I had access to was a broken Windows box that I could ssh but
not scp?  So I (stupidly) started a ftp daemon and started transfering
them that way. I thought that creating a temp account and then changing
the password via ssh would work.  

Well, the next day I was completely rooted (thank god it was only a box
in my DMZ). Still, I was thousands of miles away and needed to kill the
box. If I can't use it, I certainly wont let someone else use it.  They
took over pretty much all control to shutdown the machine remotely.  So
I finally was able to do the duty with "dd if=/dev/zero of=/dev/mem".  

And that's my story where that can be your friend :-)

-- Steve

Re: [PATCH] Fix mmap_kmem (was: [question] What's the difference between /dev/kmem and /dev/mem)

[Arjan van de Ven]

> Whilst there's no normal legitimite usage for it, it is useful for debugging.
> One thing I often do is create a circular log buffer, then fish it back 
> out by mmaping /dev/mem or /dev/kmem, and going by system.map offsets.
> No, nobody could claim it was clean or elegant, but it *is* useful.

relayfs.

Re: [PATCH] Fix mmap_kmem (was: [question] What's the difference between /dev/kmem and /dev/mem)

[Steven Rostedt]

On Thu, 2005-08-18 at 16:07 +0200, Arjan van de Ven wrote:
> > Whilst there's no normal legitimite usage for it, it is useful for debugging.
> > One thing I often do is create a circular log buffer, then fish it back 
> > out by mmaping /dev/mem or /dev/kmem, and going by system.map offsets.
> > No, nobody could claim it was clean or elegant, but it *is* useful.
> 
> relayfs.

It's still not in the vanilla kernel. Besides, mapping kmem is more
fun :-)

-- Steve