2.5.42: remove capable(CAP_SYS_RAWIO) check from open_kmem

RFC PATCH In review Olaf Dietsche 3 participants 6 messages Oct 13, 2002 – Oct 17, 2002 1 message not in the archive

Subsystems (2)

Char and misc drivers Capabilities

lore

Share & tools

lore

Olaf Dietsche

author

2002-10-13 12:40 UTC #0 lore raw

RFC PATCH 1/1 2.5.42: remove capable(CAP_SYS_RAWIO) check from open_kmem

In drivers/char/mem.c there's open_port(), which is used as open_mem() and open_kmem() as well. I don't see the benefit of this, since /dev/mem and /dev/kmem are already protected by filesystem permissions.

mem.c, line 526:
static int open_port(struct inode * inode, struct file * filp)

{
	return capable(CAP_SYS_RAWIO) ? 0 : -EPERM;
}

If anyone knows, why this is done this way, please let me know. Otherwise, I suggest the patch below.

Regards, Olaf.

drivers/char/mem.c +0 -4

@@ -533,15 +533,12 @@
 #define full_lseek      null_lseek
 #define write_zero	write_null
 #define read_full       read_zero
-#define open_mem	open_port
-#define open_kmem	open_mem
 
 static struct file_operations mem_fops = {
 	llseek:		memory_lseek,
 	read:		read_mem,
 	write:		write_mem,
 	mmap:		mmap_mem,
-	open:		open_mem,
 };
 
 static struct file_operations kmem_fops = {

@@ -549,7 +546,6 @@
 	read:		read_kmem,
 	write:		write_kmem,
 	mmap:		mmap_kmem,
-	open:		open_kmem,
 };
 
 static struct file_operations null_fops = {

Olaf Dietsche

author

2002-10-17 10:54 UTC #1 lore raw

Quoted 5 lines · Olaf Dietsche

In drivers/char/mem.c there's open_port(), which is used as open_mem() and open_kmem() as well. I don't see the benefit of this, since /dev/mem and /dev/kmem are already protected by filesystem permissions.

Quoted 8 lines

mem.c, line 526:
static int open_port(struct inode * inode, struct file * filp)
{
	return capable(CAP_SYS_RAWIO) ? 0 : -EPERM;
}
If anyone knows, why this is done this way, please let me know. Otherwise, I suggest the patch below.

I haven't got a convincing answer against this patch, so far. The patch applies to 2.5.43 as well.
Linus, please apply.

Regards, Olaf.

drivers/char/mem.c +0 -4

@@ -533,15 +533,12 @@
 #define full_lseek      null_lseek
 #define write_zero	write_null
 #define read_full       read_zero
-#define open_mem	open_port
-#define open_kmem	open_mem
 
 static struct file_operations mem_fops = {
 	llseek:		memory_lseek,
 	read:		read_mem,
 	write:		write_mem,
 	mmap:		mmap_mem,
-	open:		open_mem,
 };
 
 static struct file_operations kmem_fops = {

@@ -549,7 +546,6 @@
 	read:		read_kmem,
 	write:		write_kmem,
 	mmap:		mmap_kmem,
-	open:		open_kmem,
 };
 
 static struct file_operations null_fops = {

Chris Evans

2002-10-17 11:26 UTC #2 lore raw

Hi,

No, please DON'T apply.

If /dev/mem and /dev/kmem are only protected by filesystem permissions, then:

1) Ownership of either will confer CAP_SYS_RAWIO
2) CAP_DAC_OVERRIDE, etc. will confer CAP_SYS_RAWIO
3) Ability to mknod() one of the these will confer CAP_SYS_RAWIO (I think to make one you only need CAP_SYS_ADMIN, could be wrong).

CAP_SYS_RAWIO is extremely dangerous and must not be leverageable via other system capabilities or filesystem permissions.

There are many people who have systems with elaborate security setups where root access is limited in what it can do. If we allow a root user to get CAP_SYS_RAWIO easily, this is lost.

Cheers
Chris

Quoting Olaf Dietsche <olaf.dietsche#[email protected]>:

Quoted 1 line

Olaf Dietsche

Quoted 2 lines · <olaf.dietsche#[email protected]> writes:

Quoted 2 lines
In drivers/char/mem.c there's open_port(), which is

used as open_mem()

Quoted 1 line

Quoted 1 line
and open_kmem() as well. I don't see the benefit of

this, since

Quoted 1 line

Quoted 1 line
/dev/mem and /dev/kmem are already protected by

filesystem

Quoted 4 lines

Quoted 4 lines
permissions.
mem.c, line 526:
static int open_port(struct inode * inode, struct

file * filp)

Quoted 5 lines

Quoted 5 lines
{
	return capable(CAP_SYS_RAWIO) ? 0 : -EPERM;
}
If anyone knows, why this is done this way, please

let me

Quoted 3 lines

Quoted 1 line
know. Otherwise, I suggest the patch below.
I haven't got a convincing answer against this patch,

so far. The

Quoted 32 lines

patch applies to 2.5.43 as well.
Linus, please apply.

Regards, Olaf.

drivers/char/mem.c +0 -5

@@ -533,15 +533,12 @@
 #define full_lseek      null_lseek
 #define write_zero	write_null
 #define read_full       read_zero
-#define open_mem	open_port
-#define open_kmem	open_mem
 
 static struct file_operations mem_fops = {
 	llseek:		memory_lseek,
 	read:		read_mem,
 	write:		write_mem,
 	mmap:		mmap_mem,
-	open:		open_mem,
 };
 
 static struct file_operations kmem_fops = {

@@ -549,7 +546,6 @@
 	read:		read_kmem,
 	write:		write_kmem,
 	mmap:		mmap_kmem,
-	open:		open_kmem,
 };
 
 static struct file_operations null_fops = {
-

Quoted 1 line

To unsubscribe from this list: send the line

"unsubscribe linux-kernel" in

Quoted 2 lines

the body of a message to [email protected] More majordomo info at

http://vger.kernel.org/majordomo-info.html

Quoted 2 lines

Please read the FAQ at http://www.tux.org/lkml/

Chris Wright

2002-10-17 12:33 UTC #3 lore raw

In reply to Olaf Dietsche

Quoted 13 lines · * Olaf Dietsche (olaf.dietsche#[email protected]) wrote:

Quoted 11 lines · Olaf Dietsche
In drivers/char/mem.c there's open_port(), which is used as open_mem() and open_kmem() as well. I don't see the benefit of this, since /dev/mem and /dev/kmem are already protected by filesystem permissions.
mem.c, line 526:
static int open_port(struct inode * inode, struct file * filp)
{
	return capable(CAP_SYS_RAWIO) ? 0 : -EPERM;
}

Quoted 6 lines

Quoted 2 lines
If anyone knows, why this is done this way, please let me know. Otherwise, I suggest the patch below.
I haven't got a convincing answer against this patch, so far. The patch applies to 2.5.43 as well.
Linus, please apply.

No way. This is clearly a bad idea. CAP_SYS_RAWIO should be treated very seriously, look at what it enables. CAP_DAC_OVERRIDE is substantially less powerful, and if you remove this check, it would be the only capability protecting this.

-chris

Signature · 1 line

Linux Security Modules     http://lsm.immunix.org     http://lsm.bkbits.net

Olaf Dietsche

author

2002-10-17 14:08 UTC #4 lore raw

Quoted 5 lines · Chris Wright

Quoted 4 lines · * Olaf Dietsche (olaf.dietsche#[email protected]) wrote:
I haven't got a convincing answer against this patch, so far. The patch applies to 2.5.43 as well.
Linus, please apply.

Hehe, "please apply" is watched a lot more closely, it seems. Good to know ;-)

Quoted 4 lines

No way. This is clearly a bad idea. CAP_SYS_RAWIO should be treated very seriously, look at what it enables. CAP_DAC_OVERRIDE is substantially less powerful, and if you remove this check, it would be the only capability protecting this.

$ grep -r CAP_SYS_RAWIO v2.5.43 | wc
55

Well, since CAP_SYS_RAWIO is such a dangerous beast and /dev/kmem can't live without, then something like CAP_SYS_KMEM would be more appropriate.

Here's a new untested patch against 2.5.43. Comments?

Regards, Olaf.

drivers/char/mem.c +1 -1

diff -urN a/drivers/char/mem.c b/drivers/char/mem.c
@@ -525,7 +525,7 @@
 
 static int open_port(struct inode * inode, struct file * filp)
 {
-	return capable(CAP_SYS_RAWIO) ? 0 : -EPERM;
+	return capable(CAP_SYS_KMEM) ? 0 : -EPERM;
 }
 
 #define mmap_kmem	mmap_mem

include/linux/capability.h +4 -0

diff -urN a/include/linux/capability.h b/include/linux/capability.h
@@ -283,6 +283,10 @@
 
 #define CAP_LEASE            28
 
+/* Allow access to system memory */
+
+#define CAP_SYS_KMEM         29
+
 #ifdef __KERNEL__
 /* 
  * Bounding set

A message that is not in the archive (sent off-list or not yet received): [email protected]

Olaf Dietsche

author

2002-10-17 16:55 UTC #6 lore raw

Quoted 5 lines · Oliver Neukum

Quoted 5 lines
diff -urN a/drivers/char/mem.c b/drivers/char/mem.c
drivers/char/mem.c +0 -0
@@ -525,7 +525,7 @@
 

Quoted 8 lines

drivers/char/mem.c line 528
Quoted 4 lines
drivers/char/mem.c +1 -1
 static int open_port(struct inode * inode, struct file * filp)
 {
-	return capable(CAP_SYS_RAWIO) ? 0 : -EPERM;
+	return capable(CAP_SYS_KMEM) ? 0 : -EPERM;
return capable(CAP_SYS_KMEM) && capable(CAP_SYS_RAWIO) ? 0 : _EPERM;
Unless you check for RAWIO you can gain RAWIO by illegitimate means.

It's embarrassing, but it took until now for me to realize, that this tries to protect CAP_SYS_RAWIO and not /dev/kmem. Well, thanks for being patient with me.

Quoted 1 line

Now whether one place justifies a whole capability is another question.

This is unnecessary then.

Regards, Olaf.