[PATCH] Disable /dev/port interface on powerpc systems

[PATCH] Disable /dev/port interface on powerpc systems

[Haren Myneni]

Some power systems do not have legacy ISA devices. So, /dev/port is not
a valid interface on these systems. User level tools such as kbdrate is
trying to access the device using this interface which is causing the
system crash. 

This patch will fix this issue by not creating this interface on these
powerpc systems. 

Signed-off-by: Haren Myneni <haren@us.ibm.com>


diff -Naurp linux.orig/arch/powerpc/kernel/isa-bridge.c
linux/arch/powerpc/kernel/isa-bridge.c
--- linux.orig/arch/powerpc/kernel/isa-bridge.c	2012-02-11
02:08:08.780005293 -0800
+++ linux/arch/powerpc/kernel/isa-bridge.c	2012-02-11 02:16:25.080003386
-0800
@@ -255,6 +255,14 @@ static struct notifier_block isa_bridge_
 	.notifier_call = isa_bridge_notify
 };
 
+int __init legacy_isa_device_found(void)
+{
+	if (isa_bridge_pcidev)
+		return 1;
+	
+	return 0;
+}
+
 /**
  * isa_bridge_init - register to be notified of ISA bridge
addition/removal
  *
diff -Naurp linux.orig/drivers/char/mem.c linux/drivers/char/mem.c
--- linux.orig/drivers/char/mem.c	2012-02-11 02:08:42.710002440 -0800
+++ linux/drivers/char/mem.c	2012-03-20 20:40:43.650000003 -0700
@@ -35,6 +35,8 @@
 # include <linux/efi.h>
 #endif
 
+#define DEVPORT_MINOR	4
+
 static inline unsigned long size_inside_page(unsigned long start,
 					     unsigned long size)
 {
@@ -910,6 +912,11 @@ static char *mem_devnode(struct device *
 
 static struct class *mem_class;
 
+int __init __weak legacy_isa_device_found(void)
+{
+	return 1;
+}
+
 static int __init chr_dev_init(void)
 {
 	int minor;
@@ -930,6 +937,13 @@ static int __init chr_dev_init(void)
 	for (minor = 1; minor < ARRAY_SIZE(devlist); minor++) {
 		if (!devlist[minor].name)
 			continue;
+
+		/*
+		 * Create /dev/port? 
+		 */
+		if ((minor == DEVPORT_MINOR) && !legacy_isa_device_found())
+			continue;
+
 		device_create(mem_class, NULL, MKDEV(MEM_MAJOR, minor),
 			      NULL, devlist[minor].name);
 	}
diff -Naurp linux.orig/include/linux/isa.h linux/include/linux/isa.h
--- linux.orig/include/linux/isa.h	2012-02-11 02:07:52.030019810 -0800
+++ linux/include/linux/isa.h	2012-02-11 02:16:49.810002296 -0800
@@ -36,4 +36,5 @@ static inline void isa_unregister_driver
 }
 #endif
 
+extern int __init legacy_isa_device_found(void);
 #endif /* __LINUX_ISA_H */

Re: [PATCH] Disable /dev/port interface on powerpc systems

[Benjamin Herrenschmidt]

On Tue, 2012-03-20 at 22:37 -0700, Haren Myneni wrote:
> Some power systems do not have legacy ISA devices. So, /dev/port is not
> a valid interface on these systems. User level tools such as kbdrate is
> trying to access the device using this interface which is causing the
> system crash. 
> 
> This patch will fix this issue by not creating this interface on these
> powerpc systems. 

Doesn't fix 32-bit... not a big deal for now I suppose... But I'd rather
you change the patch a tiny bit to change legacy_isa_device_found() to
arch_has_dev_port() instead. There may be other reason than legacy ISA
to enable dev/port or not so let's make the arch hook more generic.

Another approach which might be even better is to filter per-access,
ie for each read/write to /dev/port, have a hook to check if that
specific port is valid, but that may be overkill for what is
essentially a legacy interface that should have died a long time ago :)

Cheers,
Ben.

> Signed-off-by: Haren Myneni <haren@us.ibm.com>
> 
> 
> diff -Naurp linux.orig/arch/powerpc/kernel/isa-bridge.c
> linux/arch/powerpc/kernel/isa-bridge.c
> --- linux.orig/arch/powerpc/kernel/isa-bridge.c	2012-02-11
> 02:08:08.780005293 -0800
> +++ linux/arch/powerpc/kernel/isa-bridge.c	2012-02-11 02:16:25.080003386
> -0800
> @@ -255,6 +255,14 @@ static struct notifier_block isa_bridge_
>  	.notifier_call = isa_bridge_notify
>  };
>  
> +int __init legacy_isa_device_found(void)
> +{
> +	if (isa_bridge_pcidev)
> +		return 1;
> +	
> +	return 0;
> +}
> +
>  /**
>   * isa_bridge_init - register to be notified of ISA bridge
> addition/removal
>   *
> diff -Naurp linux.orig/drivers/char/mem.c linux/drivers/char/mem.c
> --- linux.orig/drivers/char/mem.c	2012-02-11 02:08:42.710002440 -0800
> +++ linux/drivers/char/mem.c	2012-03-20 20:40:43.650000003 -0700
> @@ -35,6 +35,8 @@
>  # include <linux/efi.h>
>  #endif
>  
> +#define DEVPORT_MINOR	4
> +
>  static inline unsigned long size_inside_page(unsigned long start,
>  					     unsigned long size)
>  {
> @@ -910,6 +912,11 @@ static char *mem_devnode(struct device *
>  
>  static struct class *mem_class;
>  
> +int __init __weak legacy_isa_device_found(void)
> +{
> +	return 1;
> +}
> +
>  static int __init chr_dev_init(void)
>  {
>  	int minor;
> @@ -930,6 +937,13 @@ static int __init chr_dev_init(void)
>  	for (minor = 1; minor < ARRAY_SIZE(devlist); minor++) {
>  		if (!devlist[minor].name)
>  			continue;
> +
> +		/*
> +		 * Create /dev/port? 
> +		 */
> +		if ((minor == DEVPORT_MINOR) && !legacy_isa_device_found())
> +			continue;
> +
>  		device_create(mem_class, NULL, MKDEV(MEM_MAJOR, minor),
>  			      NULL, devlist[minor].name);
>  	}
> diff -Naurp linux.orig/include/linux/isa.h linux/include/linux/isa.h
> --- linux.orig/include/linux/isa.h	2012-02-11 02:07:52.030019810 -0800
> +++ linux/include/linux/isa.h	2012-02-11 02:16:49.810002296 -0800
> @@ -36,4 +36,5 @@ static inline void isa_unregister_driver
>  }
>  #endif
>  
> +extern int __init legacy_isa_device_found(void);
>  #endif /* __LINUX_ISA_H */
> 

Re: [PATCH] Disable /dev/port interface on powerpc systems

[Haren Myneni]

On Wed, 2012-03-21 at 18:23 +1100, Benjamin Herrenschmidt wrote:
> On Tue, 2012-03-20 at 22:37 -0700, Haren Myneni wrote:
> > Some power systems do not have legacy ISA devices. So, /dev/port is not
> > a valid interface on these systems. User level tools such as kbdrate is
> > trying to access the device using this interface which is causing the
> > system crash. 
> > 
> > This patch will fix this issue by not creating this interface on these
> > powerpc systems. 
> 
> Doesn't fix 32-bit... not a big deal for now I suppose... But I'd rather
> you change the patch a tiny bit to change legacy_isa_device_found() to
> arch_has_dev_port() instead. There may be other reason than legacy ISA
> to enable dev/port or not so let's make the arch hook more generic.

Thanks for your suggestions. I will make a change and re-post the patch.

> 
> Another approach which might be even better is to filter per-access,
> ie for each read/write to /dev/port, have a hook to check if that
> specific port is valid, but that may be overkill for what is
> essentially a legacy interface that should have died a long time ago :)

Yes, can be changed open or read/write calls and return -ENODEV,
but /dev/port is no use on these systems.

I am not sure whether any power systems have legacy ISA devices. If we
do not have, CONFIG_DEVPORT can be disabled in Kconfig.

--- 
src/drivers/char/Kconfig.orig	2012-03-21 11:05:54.610010864 -0700
+++ src/drivers/char/Kconfig	2012-03-21 11:02:29.750009886 -0700
@@ -596,6 +596,7 @@ config DEVPORT
 	bool
 	depends on !M68K
 	depends on ISA || PCI
+	depends on !PPC64
 	default y


Thanks
Haren

> 
> Cheers,
> Ben.
> 
> > Signed-off-by: Haren Myneni <haren@us.ibm.com>
> > 
> > 
> > diff -Naurp linux.orig/arch/powerpc/kernel/isa-bridge.c
> > linux/arch/powerpc/kernel/isa-bridge.c
> > --- linux.orig/arch/powerpc/kernel/isa-bridge.c	2012-02-11
> > 02:08:08.780005293 -0800
> > +++ linux/arch/powerpc/kernel/isa-bridge.c	2012-02-11 02:16:25.080003386
> > -0800
> > @@ -255,6 +255,14 @@ static struct notifier_block isa_bridge_
> >  	.notifier_call = isa_bridge_notify
> >  };
> >  
> > +int __init legacy_isa_device_found(void)
> > +{
> > +	if (isa_bridge_pcidev)
> > +		return 1;
> > +	
> > +	return 0;
> > +}
> > +
> >  /**
> >   * isa_bridge_init - register to be notified of ISA bridge
> > addition/removal
> >   *
> > diff -Naurp linux.orig/drivers/char/mem.c linux/drivers/char/mem.c
> > --- linux.orig/drivers/char/mem.c	2012-02-11 02:08:42.710002440 -0800
> > +++ linux/drivers/char/mem.c	2012-03-20 20:40:43.650000003 -0700
> > @@ -35,6 +35,8 @@
> >  # include <linux/efi.h>
> >  #endif
> >  
> > +#define DEVPORT_MINOR	4
> > +
> >  static inline unsigned long size_inside_page(unsigned long start,
> >  					     unsigned long size)
> >  {
> > @@ -910,6 +912,11 @@ static char *mem_devnode(struct device *
> >  
> >  static struct class *mem_class;
> >  
> > +int __init __weak legacy_isa_device_found(void)
> > +{
> > +	return 1;
> > +}
> > +
> >  static int __init chr_dev_init(void)
> >  {
> >  	int minor;
> > @@ -930,6 +937,13 @@ static int __init chr_dev_init(void)
> >  	for (minor = 1; minor < ARRAY_SIZE(devlist); minor++) {
> >  		if (!devlist[minor].name)
> >  			continue;
> > +
> > +		/*
> > +		 * Create /dev/port? 
> > +		 */
> > +		if ((minor == DEVPORT_MINOR) && !legacy_isa_device_found())
> > +			continue;
> > +
> >  		device_create(mem_class, NULL, MKDEV(MEM_MAJOR, minor),
> >  			      NULL, devlist[minor].name);
> >  	}
> > diff -Naurp linux.orig/include/linux/isa.h linux/include/linux/isa.h
> > --- linux.orig/include/linux/isa.h	2012-02-11 02:07:52.030019810 -0800
> > +++ linux/include/linux/isa.h	2012-02-11 02:16:49.810002296 -0800
> > @@ -36,4 +36,5 @@ static inline void isa_unregister_driver
> >  }
> >  #endif
> >  
> > +extern int __init legacy_isa_device_found(void);
> >  #endif /* __LINUX_ISA_H */
> > 
> 
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@lists.ozlabs.org
> https://lists.ozlabs.org/listinfo/linuxppc-dev
> 

Re: [PATCH] Disable /dev/port interface on powerpc systems

[Benjamin Herrenschmidt]

On Wed, 2012-03-21 at 11:58 -0700, Haren Myneni wrote:
> 
> Yes, can be changed open or read/write calls and return -ENODEV,
> but /dev/port is no use on these systems.
> 
> I am not sure whether any power systems have legacy ISA devices. If we
> do not have, CONFIG_DEVPORT can be disabled in Kconfig. 

Dome do, older ones mostly.

Cheers,
Ben.

[PATCH] Disable /dev/port interface on powerpc systems

[Haren Myneni]

Ben, Here it is the updated patch based on your suggestions. Please let
me know if it has any issues.

Thanks
Haren


Some power systems do not have legacy ISA devices. So, /dev/port is not
a valid interface on these systems. User level tools such as kbdrate is
trying to access the device using this interface which is causing the
system crash. 

This patch will fix this issue by not creating this interface on these
powerpc systems. 

Signed-off-by: Haren Myneni <haren@us.ibm.com>

diff -Naurp linux.orig/arch/powerpc/include/asm/io.h
linux/arch/powerpc/include/asm/io.h
--- linux.orig/arch/powerpc/include/asm/io.h	2012-03-24
00:51:21.619999958 -0700
+++ linux/arch/powerpc/include/asm/io.h	2012-03-24 00:52:50.450000543
-0700
@@ -20,6 +20,14 @@ extern int check_legacy_ioport(unsigned 
 #define _PNPWRP		0xa79
 #define PNPBIOS_BASE	0xf000
 
+#ifdef CONFIG_PPC64
+extern struct pci_dev *isa_bridge_pcidev;
+/*
+ * has legacy ISA devices ?
+ */
+#define arch_has_dev_port()	(isa_bridge_pcidev != NULL)
+#endif
+
 #include <linux/device.h>
 #include <linux/io.h>
 
diff -Naurp linux.orig/drivers/char/mem.c linux/drivers/char/mem.c
--- linux.orig/drivers/char/mem.c	2012-03-24 00:50:21.570000660 -0700
+++ linux/drivers/char/mem.c	2012-03-24 00:52:32.080000802 -0700
@@ -35,6 +35,8 @@
 # include <linux/efi.h>
 #endif
 
+#define DEVPORT_MINOR	4
+
 static inline unsigned long size_inside_page(unsigned long start,
 					     unsigned long size)
 {
@@ -930,6 +932,13 @@ static int __init chr_dev_init(void)
 	for (minor = 1; minor < ARRAY_SIZE(devlist); minor++) {
 		if (!devlist[minor].name)
 			continue;
+
+		/*
+		 * Create /dev/port? 
+		 */
+		if ((minor == DEVPORT_MINOR) && !arch_has_dev_port())
+			continue;
+
 		device_create(mem_class, NULL, MKDEV(MEM_MAJOR, minor),
 			      NULL, devlist[minor].name);
 	}
diff -Naurp linux.orig/include/linux/io.h linux/include/linux/io.h
--- linux.orig/include/linux/io.h	2012-03-24 00:54:18.060000449 -0700
+++ linux/include/linux/io.h	2012-03-24 01:12:43.280000840 -0700
@@ -67,4 +67,13 @@ int check_signature(const volatile void 
 			const unsigned char *signature, int length);
 void devm_ioremap_release(struct device *dev, void *res);
 
+/*
+ * Some systems do not have legacy ISA devices.
+ * /dev/port is not a valid interface on these systems.
+ * So for those archs, <asm/io.h> should define the following symbol.
+ */
+#ifndef arch_has_dev_port
+#define arch_has_dev_port()     (1)
+#endif
+
 #endif /* _LINUX_IO_H */

Re: [PATCH] Disable /dev/port interface on powerpc systems

[Benjamin Herrenschmidt]

On Wed, 2012-03-21 at 18:23 +1100, Benjamin Herrenschmidt wrote:
> On Tue, 2012-03-20 at 22:37 -0700, Haren Myneni wrote:
> > Some power systems do not have legacy ISA devices. So, /dev/port is not
> > a valid interface on these systems. User level tools such as kbdrate is
> > trying to access the device using this interface which is causing the
> > system crash. 
> > 
> > This patch will fix this issue by not creating this interface on these
> > powerpc systems. 
> 
> Doesn't fix 32-bit... not a big deal for now I suppose... But I'd rather
> you change the patch a tiny bit to change legacy_isa_device_found() to
> arch_has_dev_port() instead. There may be other reason than legacy ISA
> to enable dev/port or not so let's make the arch hook more generic.
> 
> Another approach which might be even better is to filter per-access,
> ie for each read/write to /dev/port, have a hook to check if that
> specific port is valid, but that may be overkill for what is
> essentially a legacy interface that should have died a long time ago :)

The more I think about this the less I like it ...

At the end of the day, the bug is in kbdrate (or the distro, whatever)

It's just completely broken to have a bit of userspace running as root
randomly whacking IO ports or MMIO registers based on the assumption
that there must be some kind of ISA kbd controller there... It's going
to break on more than just powerpc (ARM anyone ?).

So kbdrate is busted and needs to be either fixed or removed.

Cheers,
Ben.

Re: [PATCH] Disable /dev/port interface on powerpc systems

[Michael Ellerman]

On Sat, 2012-03-24 at 01:23 -0700, Haren Myneni wrote:
> Ben, Here it is the updated patch based on your suggestions. Please let
> me know if it has any issues.
> 

This breaks the i386 defconfig build with:

drivers/char/mem.c:903:3: error: implicit declaration of function
'arch_has_dev_port' [-Werror=implicit-function-declaration]


I think because that file includes <asm/io.h> directly, not via
include/linux/io.h, so your stub is never seen.

cheers

[PATCH] Disable /dev/port interface on powerpc systems

[Haren Myneni]

Michael, Thanks for the comment. Here it is the updated patch which
fixes x86 build.

Thanks
Haren

Some power systems do not have legacy ISA devices. So, /dev/port is not
a valid interface on these systems. User level tools such as kbdrate is
trying to access the device using this interface which is causing the
system crash. 

This patch will fix this issue by not creating this interface on these
powerpc systems. 

Signed-off-by: Haren Myneni <haren@us.ibm.com>

diff -Naurp linux.orig/arch/powerpc/include/asm/io.h linux/arch/powerpc/include/asm/io.h
--- linux.orig/arch/powerpc/include/asm/io.h	2012-06-16 01:15:11.000000000 -0500
+++ linux/arch/powerpc/include/asm/io.h	2012-06-16 01:15:20.000000000 -0500
@@ -20,6 +20,14 @@ extern int check_legacy_ioport(unsigned 
 #define _PNPWRP		0xa79
 #define PNPBIOS_BASE	0xf000
 
+#ifdef CONFIG_PPC64
+extern struct pci_dev *isa_bridge_pcidev;
+/*
+ * has legacy ISA devices ?
+ */
+#define arch_has_dev_port()	(isa_bridge_pcidev != NULL)
+#endif
+
 #include <linux/device.h>
 #include <linux/io.h>
 
diff -Naurp linux.orig/drivers/char/mem.c linux/drivers/char/mem.c
--- linux.orig/drivers/char/mem.c	2012-06-16 01:15:42.000000000 -0500
+++ linux/drivers/char/mem.c	2012-06-16 01:15:53.000000000 -0500
@@ -27,14 +27,16 @@
 #include <linux/splice.h>
 #include <linux/pfn.h>
 #include <linux/export.h>
+#include <linux/io.h>
 
 #include <asm/uaccess.h>
-#include <asm/io.h>
 
 #ifdef CONFIG_IA64
 # include <linux/efi.h>
 #endif
 
+#define DEVPORT_MINOR	4
+
 static inline unsigned long size_inside_page(unsigned long start,
 					     unsigned long size)
 {
@@ -894,6 +896,13 @@ static int __init chr_dev_init(void)
 	for (minor = 1; minor < ARRAY_SIZE(devlist); minor++) {
 		if (!devlist[minor].name)
 			continue;
+
+		/*
+		 * Create /dev/port? 
+		 */
+		if ((minor == DEVPORT_MINOR) && !arch_has_dev_port())
+			continue;
+
 		device_create(mem_class, NULL, MKDEV(MEM_MAJOR, minor),
 			      NULL, devlist[minor].name);
 	}
diff -Naurp linux.orig/include/linux/io.h linux/include/linux/io.h
--- linux.orig/include/linux/io.h	2012-06-16 01:16:13.000000000 -0500
+++ linux/include/linux/io.h	2012-06-16 01:16:24.000000000 -0500
@@ -67,4 +67,13 @@ int check_signature(const volatile void 
 			const unsigned char *signature, int length);
 void devm_ioremap_release(struct device *dev, void *res);
 
+/*
+ * Some systems do not have legacy ISA devices.
+ * /dev/port is not a valid interface on these systems.
+ * So for those archs, <asm/io.h> should define the following symbol.
+ */
+#ifndef arch_has_dev_port
+#define arch_has_dev_port()     (1)
+#endif
+
 #endif /* _LINUX_IO_H */