[Patch 0/2] add check_legacy_ioport calls to prevent oops

[Patch 0/2] add check_legacy_ioport calls to prevent oops

[Christian Krafft]

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

Hi,

the following two patches prevent kernel from crashing on powerpc.
The surrounding ifdefs will be obsolete, if check_legacy_ioport becomes
generic code.


-- 
Mit freundlichen Gruessen,
kind regards,

Christian Krafft
IBM Systems & Technology Group,
Linux Kernel Development
IT Specialist


Vorsitzender des Aufsichtsrats:	Martin Jetter
Geschaeftsfuehrung:		Herbert Kircher
Sitz der Gesellschaft:		Boeblingen
Registriergericht:		Amtsgericht Stuttgart, HRB 243294

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

Re: [Patch 0/2] powerpc: avoid userspace poking to legacy ioports

[Christian Krafft]

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

sensors_detect crashes kernel on PowerPC, as it pokes directly to memory.
This patch adds a check_legacy_ioports to read_port and write_port.
It will now return ENXIO, instead of oopsing.

Signed-off-by: Christian Krafft <krafft@de.ibm.com>

Index: linux.git/drivers/char/mem.c
===================================================================
--- linux.git.orig/drivers/char/mem.c
+++ linux.git/drivers/char/mem.c
@@ -566,8 +566,13 @@ static ssize_t read_port(struct file * f
 	char __user *tmp = buf;
 
 	if (!access_ok(VERIFY_WRITE, buf, count))
-		return -EFAULT; 
+		return -EFAULT;
+
 	while (count-- > 0 && i < 65536) {
+#ifdef CONFIG_PPC_MERGE
+		if (check_legacy_ioport(i))
+			return -ENXIO;
+#endif
 		if (__put_user(inb(i),tmp) < 0) 
 			return -EFAULT;  
 		i++;
@@ -585,6 +590,7 @@ static ssize_t write_port(struct file * 
 
 	if (!access_ok(VERIFY_READ,buf,count))
 		return -EFAULT;
+
 	while (count-- > 0 && i < 65536) {
 		char c;
 		if (__get_user(c, tmp)) {
@@ -592,6 +598,10 @@ static ssize_t write_port(struct file * 
 				break;
 			return -EFAULT; 
 		}
+#ifdef CONFIG_PPC_MERGE
+		if (check_legacy_ioport(i))
+			return -ENXIO;
+#endif
 		outb(c,i);
 		i++;
 		tmp++;


-- 
Mit freundlichen Gruessen,
kind regards,

Christian Krafft
IBM Systems & Technology Group,
Linux Kernel Development
IT Specialist


Vorsitzender des Aufsichtsrats:	Martin Jetter
Geschaeftsfuehrung:		Herbert Kircher
Sitz der Gesellschaft:		Boeblingen
Registriergericht:		Amtsgericht Stuttgart, HRB 243294

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

Re: [Patch 2/2] powerpc: i2c-isa: add access check to legacy ioports

[Christian Krafft]

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

when probing i2c-pca-isa writes to legacy ioports, which crashes the kernel
if there is no device at that port.
This patch adds a check_legacy_ioport call, so probe failes gracefully
and thus prevents the oops.

Signed-off-by: Christian Krafft <krafft@de.ibm.com>

Index: linux.git/drivers/i2c/busses/i2c-pca-isa.c
===================================================================
--- linux.git.orig/drivers/i2c/busses/i2c-pca-isa.c
+++ linux.git/drivers/i2c/busses/i2c-pca-isa.c
@@ -125,6 +125,13 @@ static int __devinit pca_isa_probe(struc
 
 	dev_info(dev, "i/o base %#08lx. irq %d\n", base, irq);
 
+#ifdef CONFIG_PPC_MERGE
+	if (check_legacy_ioport(base)) {
+		dev_err(dev, "I/O address %#08lx is not available\n", base);
+		goto out;
+	}
+#endif
+
 	if (!request_region(base, IO_SIZE, "i2c-pca-isa")) {
 		dev_err(dev, "I/O address %#08lx is in use\n", base);
 		goto out;


-- 
Mit freundlichen Gruessen,
kind regards,

Christian Krafft
IBM Systems & Technology Group,
Linux Kernel Development
IT Specialist


Vorsitzender des Aufsichtsrats:	Martin Jetter
Geschaeftsfuehrung:		Herbert Kircher
Sitz der Gesellschaft:		Boeblingen
Registriergericht:		Amtsgericht Stuttgart, HRB 243294

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

Re: [Patch 0/2] powerpc: avoid userspace poking to legacy ioports

[Benjamin Herrenschmidt]

On Wed, 2008-02-13 at 18:35 +0100, Christian Krafft wrote:
> sensors_detect crashes kernel on PowerPC, as it pokes directly to memory.
> This patch adds a check_legacy_ioports to read_port and write_port.
> It will now return ENXIO, instead of oopsing.
> 
> Signed-off-by: Christian Krafft <krafft@de.ibm.com>

The problem is that this prevents using /proc/ioports to access PCI
IO space, which might be useful.

I hate that sensors_detect.. or for that matter any other userland code
that pokes random ports like that. It should die.

Ben.

> Index: linux.git/drivers/char/mem.c
> ===================================================================
> --- linux.git.orig/drivers/char/mem.c
> +++ linux.git/drivers/char/mem.c
> @@ -566,8 +566,13 @@ static ssize_t read_port(struct file * f
>  	char __user *tmp = buf;
>  
>  	if (!access_ok(VERIFY_WRITE, buf, count))
> -		return -EFAULT; 
> +		return -EFAULT;
> +
>  	while (count-- > 0 && i < 65536) {
> +#ifdef CONFIG_PPC_MERGE
> +		if (check_legacy_ioport(i))
> +			return -ENXIO;
> +#endif
>  		if (__put_user(inb(i),tmp) < 0) 
>  			return -EFAULT;  
>  		i++;
> @@ -585,6 +590,7 @@ static ssize_t write_port(struct file * 
>  
>  	if (!access_ok(VERIFY_READ,buf,count))
>  		return -EFAULT;
> +
>  	while (count-- > 0 && i < 65536) {
>  		char c;
>  		if (__get_user(c, tmp)) {
> @@ -592,6 +598,10 @@ static ssize_t write_port(struct file * 
>  				break;
>  			return -EFAULT; 
>  		}
> +#ifdef CONFIG_PPC_MERGE
> +		if (check_legacy_ioport(i))
> +			return -ENXIO;
> +#endif
>  		outb(c,i);
>  		i++;
>  		tmp++;
> 
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [Patch 0/2] powerpc: avoid userspace poking to legacy ioports

[Arnd Bergmann]

On Wednesday 13 February 2008, Benjamin Herrenschmidt wrote:
> On Wed, 2008-02-13 at 18:35 +0100, Christian Krafft wrote:
> > sensors_detect crashes kernel on PowerPC, as it pokes directly to memory.
> > This patch adds a check_legacy_ioports to read_port and write_port.
> > It will now return ENXIO, instead of oopsing.
> > 
> > Signed-off-by: Christian Krafft <krafft@de.ibm.com>
> 
> The problem is that this prevents using /proc/ioports to access PCI
> IO space, which might be useful.
> 
> I hate that sensors_detect.. or for that matter any other userland code
> that pokes random ports like that. It should die.

What kind of Oops do you get? Is it because the ioport area is not
ioremapped at all or do you get a machine check? If there is no
mapping, we could possibly change inb and outb do deal with that.

	Arnd <><

Re: [Patch 2/2] powerpc: i2c-isa: add access check to legacy ioports

[Jean Delvare]

Hi Christian,

On Wed, 13 Feb 2008 18:37:01 +0100, Christian Krafft wrote:
> when probing i2c-pca-isa writes to legacy ioports, which crashes the kernel
> if there is no device at that port.
> This patch adds a check_legacy_ioport call, so probe failes gracefully
> and thus prevents the oops.
> 
> Signed-off-by: Christian Krafft <krafft@de.ibm.com>
> 
> Index: linux.git/drivers/i2c/busses/i2c-pca-isa.c
> ===================================================================
> --- linux.git.orig/drivers/i2c/busses/i2c-pca-isa.c
> +++ linux.git/drivers/i2c/busses/i2c-pca-isa.c
> @@ -125,6 +125,13 @@ static int __devinit pca_isa_probe(struc
>  
>  	dev_info(dev, "i/o base %#08lx. irq %d\n", base, irq);
>  
> +#ifdef CONFIG_PPC_MERGE
> +	if (check_legacy_ioport(base)) {
> +		dev_err(dev, "I/O address %#08lx is not available\n", base);
> +		goto out;
> +	}
> +#endif
> +
>  	if (!request_region(base, IO_SIZE, "i2c-pca-isa")) {
>  		dev_err(dev, "I/O address %#08lx is in use\n", base);
>  		goto out;
> 

I can apply this, no problem. That being said, you might alternatively
just exclude this driver from PPC. It's for a rare device with no known
user, the driver is unmaintained and will hopefully be replaced soon by
a platform driver which will not probe random ports at load time.

-- 
Jean Delvare

Re: [Patch 0/2] powerpc: avoid userspace poking to legacy ioports

[Jean Delvare]

Hi Ben,

On Thu, 14 Feb 2008 07:42:54 +1100, Benjamin Herrenschmidt wrote:
> 
> On Wed, 2008-02-13 at 18:35 +0100, Christian Krafft wrote:
> > sensors_detect crashes kernel on PowerPC, as it pokes directly to memory.

For the records, sensors-detect accesses I/O ports, not memory.

> > This patch adds a check_legacy_ioports to read_port and write_port.
> > It will now return ENXIO, instead of oopsing.
> > 
> > Signed-off-by: Christian Krafft <krafft@de.ibm.com>
> 
> The problem is that this prevents using /proc/ioports to access PCI
> IO space, which might be useful.

Maybe Christian's patch can be improved to not do the check on these?
As long as /dev/port exists, it seems reasonable that the kernel should
behave, no matter what I/O ports are accessed from user-space.

> I hate that sensors_detect.. or for that matter any other userland code
> that pokes random ports like that. It should die.

What do you propose as a replacement?

And how is userland code poking at random ports different from kernel
code poking at random ports? We could move sensors-detect inside the
kernel (and I have some plan to do that) but I fail to see how this
would solve this particular problem.

> > Index: linux.git/drivers/char/mem.c
> > ===================================================================
> > --- linux.git.orig/drivers/char/mem.c
> > +++ linux.git/drivers/char/mem.c
> > @@ -566,8 +566,13 @@ static ssize_t read_port(struct file * f
> >  	char __user *tmp = buf;
> >  
> >  	if (!access_ok(VERIFY_WRITE, buf, count))
> > -		return -EFAULT; 
> > +		return -EFAULT;
> > +
> >  	while (count-- > 0 && i < 65536) {
> > +#ifdef CONFIG_PPC_MERGE
> > +		if (check_legacy_ioport(i))
> > +			return -ENXIO;
> > +#endif
> >  		if (__put_user(inb(i),tmp) < 0) 
> >  			return -EFAULT;  
> >  		i++;
> > @@ -585,6 +590,7 @@ static ssize_t write_port(struct file * 
> >  
> >  	if (!access_ok(VERIFY_READ,buf,count))
> >  		return -EFAULT;
> > +
> >  	while (count-- > 0 && i < 65536) {
> >  		char c;
> >  		if (__get_user(c, tmp)) {
> > @@ -592,6 +598,10 @@ static ssize_t write_port(struct file * 
> >  				break;
> >  			return -EFAULT; 
> >  		}
> > +#ifdef CONFIG_PPC_MERGE
> > +		if (check_legacy_ioport(i))
> > +			return -ENXIO;
> > +#endif
> >  		outb(c,i);
> >  		i++;
> >  		tmp++;


-- 
Jean Delvare

Re: [Patch 0/2] powerpc: avoid userspace poking to legacy ioports

[Benjamin Herrenschmidt]

> Maybe Christian's patch can be improved to not do the check on these?
> As long as /dev/port exists, it seems reasonable that the kernel should
> behave, no matter what I/O ports are accessed from user-space.

nonsense.

 /dev/mem exists for example, but you are still not supposed to go
bang all over the place in it.

> > I hate that sensors_detect.. or for that matter any other userland code
> > that pokes random ports like that. It should die.
> 
> What do you propose as a replacement?

Dunno, something less scary, like knowing where your sensors are on a
given machine... honestly, it's just scary the risk you guys are taking
by banging random IO ports.

At the very least, that shouldn't be done on non-x86.

> And how is userland code poking at random ports different from kernel
> code poking at random ports? We could move sensors-detect inside the
> kernel (and I have some plan to do that) but I fail to see how this
> would solve this particular problem.

It wouldn't, but at least I could NAK it or make it CONFIG_X86 :-)

Ben.

Re: [Patch 0/2] powerpc: avoid userspace poking to legacy ioports

[Jean Delvare]

On Tue, 19 Feb 2008 07:42:03 +1100, Benjamin Herrenschmidt wrote:
> 
> > Maybe Christian's patch can be improved to not do the check on these?
> > As long as /dev/port exists, it seems reasonable that the kernel should
> > behave, no matter what I/O ports are accessed from user-space.
> 
> nonsense.
> 
>  /dev/mem exists for example, but you are still not supposed to go
> bang all over the place in it.

You should at least be able to read from it without crashing the
machine. Of course writing is a different story.

> > > I hate that sensors_detect.. or for that matter any other userland code
> > > that pokes random ports like that. It should die.
> > 
> > What do you propose as a replacement?
> 
> Dunno, something less scary, like knowing where your sensors are on a
> given machine...

You mean, having a complete database for the, what, 4000 PC
motherboards out there? And maintaining it day after day? _This_ sounds
much scarier to me than the current situation.

> honestly, it's just scary the risk you guys are taking
> by banging random IO ports.

I don't remember anyone reporting problems with this in the past 3 or 4
years, so it doesn't seem to be a big problem in practice.

> At the very least, that shouldn't be done on non-x86.

I am surprised that anyone would actually run sensors-detect on
non-x86... Non-PC hardware usually doesn't have sensors driven by
"hwmon" drivers anyway, or people know what they have and do not need
detection. But I would be totally fine with updating sensors-detect to
skip some of the probes on non-x86 hardware. There are basically
3 /dev/port probes that are done currently:

* Super-I/O chips at 0x2e/0x2f and 0x4e/0x4f.

* Legacy PC hardware monitoring chips at 0x290-0x297.

* IPMI interface at 0x0ca3 and 0x0cab (read-only).

Please tell me which ones should be skipped on PowerPC.

Christian, can you tell me which of these probes caused trouble for you?

> > And how is userland code poking at random ports different from kernel
> > code poking at random ports? We could move sensors-detect inside the
> > kernel (and I have some plan to do that) but I fail to see how this
> > would solve this particular problem.
> 
> It wouldn't, but at least I could NAK it or make it CONFIG_X86 :-)

The same could be done for user-space (or at the /dev/port level.)

-- 
Jean Delvare

Re: [Patch 0/2] powerpc: avoid userspace poking to legacy ioports

[Arjan van de Ven]

On Mon, 18 Feb 2008 21:58:42 +0100
Jean Delvare <khali@linux-fr.org> wrote:

> On Tue, 19 Feb 2008 07:42:03 +1100, Benjamin Herrenschmidt wrote:
> > 
> > > Maybe Christian's patch can be improved to not do the check on
> > > these? As long as /dev/port exists, it seems reasonable that the
> > > kernel should behave, no matter what I/O ports are accessed from
> > > user-space.
> > 
> > nonsense.
> > 
> >  /dev/mem exists for example, but you are still not supposed to go
> > bang all over the place in it.
> 
> You should at least be able to read from it without crashing the
> machine. Of course writing is a different story.

keep dreaming. This is not how /dev/mem works today, not on x86 and very likely not on ppc either.

Re: [Patch 0/2] powerpc: avoid userspace poking to legacy ioports

[Benjamin Herrenschmidt]

> 
> * Super-I/O chips at 0x2e/0x2f and 0x4e/0x4f.
> 
> * Legacy PC hardware monitoring chips at 0x290-0x297.
> 
> * IPMI interface at 0x0ca3 and 0x0cab (read-only).
> 
> Please tell me which ones should be skipped on PowerPC.

Skip the whole thing. I consider that on a powerpc linux port, the
platform is responsible for telling drivers where things are (via the
device tree generally)
 
> Christian, can you tell me which of these probes caused trouble for you?
> 
> > > And how is userland code poking at random ports different from kernel
> > > code poking at random ports? We could move sensors-detect inside the
> > > kernel (and I have some plan to do that) but I fail to see how this
> > > would solve this particular problem.
> > 
> > It wouldn't, but at least I could NAK it or make it CONFIG_X86 :-)
> 
> The same could be done for user-space (or at the /dev/port level.)

Well, there are -other- legit usages of /dev/port...

Ben.