[PATCH v3 0/4] Fix system crash for accessing unmapped IO port regions

[PATCH v3 0/4] Fix system crash for accessing unmapped IO port regions

[John Garry]

It was reported some time ago that arm64 systems will crash if a driver
attempts to access IO port addresses when the PCI IO port region has not
been mapped [1].

More recently, a similar crash is where the system PCI host probe fails,
and the IPMI driver crashes the system while attempting to do some IO port
accesses [2].

This patchset attempts to keep the kernel alive in such situations by 2
complementary methods:
1. Rejecting IO port resource requests until PCI IO port regions have been
mapped (in a pci_remap_iospace() call).
2. Rejecting logic PIO access to PCI IO regions until, again, PCI IO port
regions have been mapped

About 1:
Currently the PCI IO port region is initialized to the full range,
{0, IO_SPACE_LIMIT}. As such, any IO port region requests would not fail
because of PCI IO port regions not being mapped.

Patch 1/4 looks to remedy this issue by ensuring IO port requests are
made to direct children of ioport_resource (PCI host IO port regions),
similar to Arnd's solution, mentioned in [1]:

"I see that ioport_resource gets initialized to the {0, IO_SPACE_LIMIT}
range. If we could change it so that pci_remap_iospace() hooks up
to ioport_resource and extends it whenever something gets mapped
there up to IO_SPACE_LIMIT, we can change the default range to
{0,0}, which would fail for any request_region call before the
first pci_remap_iospace."

I didn't use this solution, as logical PIO space is sparse in
{0, IO_SPACE_LIMIT}, so we cannot simply grow the region.

*As discussed with Bjorn in v2 series, we doubt that this approach is
sound, as legacy ISA devices do not necessarily reply on PCI.*

However I will keep the patch in the series as a reference and as a topic
of debate.

It's also an RFC as the implementation solution is not idea.

About 2:
Some drivers - like f71805f hwmon driver - do not call
request_{muxed_}region() prior to accessing IO port regions, as they
should do.

So patches 2-3/4 adds a safeguard against this, in that unwarranted PIO IO
accesses will be discarded in the low-level accessors.

About the issue of f71805f driver not requesting the IO port region -
many drivers do this, and need to be fixed up separately.

1. https://lore.kernel.org/linux-pci/56F209A9.4040304@huawei.com
2. https://lore.kernel.org/linux-arm-kernel/e6995b4a-184a-d8d4-f4d4-9ce75d8f47c0@huawei.com/

Differences to v2 patchset:
https://lkml.org/lkml/2019/3/20/788
- Add a patch to use logical PIO accessors for !CONFIG_INDIRECT_PIO
- Some tidy-up according to Andy's review

Differences to v1 patchset:
https://lkml.org/lkml/2019/3/14/630
- Drop f71805f fix - it can be done in a separate patchset
- Change implementation in resource.c patch to check if parent of region
  is ioport_resource
- Add patch to fix some logic_pio.c prints

John Garry (4):
  resource: Request IO port regions from children of ioport_resource
  lib: logic_pio: Use logical PIO low-level accessors for
    !CONFIG_INDIRECT_PIO
  lib: logic_pio: Reject accesses to unregistered CPU MMIO regions
  lib: logic_pio: Fix up some prints

 include/linux/ioport.h    |  11 ++-
 include/linux/logic_pio.h |   7 +-
 kernel/resource.c         |  28 +++++++
 lib/logic_pio.c           | 157 +++++++++++++++++++++++++++++---------
 4 files changed, 159 insertions(+), 44 deletions(-)

-- 
2.17.1

[PATCH v3 0/4] Fix system crash for accessing unmapped IO port regions

[John Garry]

It was reported some time ago that arm64 systems will crash if a driver
attempts to access IO port addresses when the PCI IO port region has not
been mapped [1].

More recently, a similar crash is where the system PCI host probe fails,
and the IPMI driver crashes the system while attempting to do some IO port
accesses [2].

This patchset attempts to keep the kernel alive in such situations by 2
complementary methods:
1. Rejecting IO port resource requests until PCI IO port regions have been
mapped (in a pci_remap_iospace() call).
2. Rejecting logic PIO access to PCI IO regions until, again, PCI IO port
regions have been mapped

About 1:
Currently the PCI IO port region is initialized to the full range,
{0, IO_SPACE_LIMIT}. As such, any IO port region requests would not fail
because of PCI IO port regions not being mapped.

Patch 1/4 looks to remedy this issue by ensuring IO port requests are
made to direct children of ioport_resource (PCI host IO port regions),
similar to Arnd's solution, mentioned in [1]:

"I see that ioport_resource gets initialized to the {0, IO_SPACE_LIMIT}
range. If we could change it so that pci_remap_iospace() hooks up
to ioport_resource and extends it whenever something gets mapped
there up to IO_SPACE_LIMIT, we can change the default range to
{0,0}, which would fail for any request_region call before the
first pci_remap_iospace."

I didn't use this solution, as logical PIO space is sparse in
{0, IO_SPACE_LIMIT}, so we cannot simply grow the region.

*As discussed with Bjorn in v2 series, we doubt that this approach is
sound, as legacy ISA devices do not necessarily reply on PCI.*

However I will keep the patch in the series as a reference and as a topic
of debate.

It's also an RFC as the implementation solution is not idea.

About 2:
Some drivers - like f71805f hwmon driver - do not call
request_{muxed_}region() prior to accessing IO port regions, as they
should do.

So patches 2-3/4 adds a safeguard against this, in that unwarranted PIO IO
accesses will be discarded in the low-level accessors.

About the issue of f71805f driver not requesting the IO port region -
many drivers do this, and need to be fixed up separately.

1. https://lore.kernel.org/linux-pci/56F209A9.4040304@huawei.com
2. https://lore.kernel.org/linux-arm-kernel/e6995b4a-184a-d8d4-f4d4-9ce75d8f47c0@huawei.com/

Differences to v2 patchset:
https://lkml.org/lkml/2019/3/20/788
- Add a patch to use logical PIO accessors for !CONFIG_INDIRECT_PIO
- Some tidy-up according to Andy's review

Differences to v1 patchset:
https://lkml.org/lkml/2019/3/14/630
- Drop f71805f fix - it can be done in a separate patchset
- Change implementation in resource.c patch to check if parent of region
  is ioport_resource
- Add patch to fix some logic_pio.c prints

John Garry (4):
  resource: Request IO port regions from children of ioport_resource
  lib: logic_pio: Use logical PIO low-level accessors for
    !CONFIG_INDIRECT_PIO
  lib: logic_pio: Reject accesses to unregistered CPU MMIO regions
  lib: logic_pio: Fix up some prints

 include/linux/ioport.h    |  11 ++-
 include/linux/logic_pio.h |   7 +-
 kernel/resource.c         |  28 +++++++
 lib/logic_pio.c           | 157 +++++++++++++++++++++++++++++---------
 4 files changed, 159 insertions(+), 44 deletions(-)

-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[RFC PATCH v3 1/4] resource: Request IO port regions from children of ioport_resource

[John Garry]

Currently request_region() requests an IO port region directly from the
top resource, ioport_resource.

There is an issue here, in that drivers may successfully request an IO
port region even if the IO port region has not even been mapped in
(in pci_remap_iospace()).

This may lead to crashes when the system has no PCI host, or, has a host
but it has failed enumeration, while drivers still attempt to access PCI
IO ports, as below:

root@(none)$root@(none)$ insmod f71882fg.ko
 Unable to handle kernel paging request at virtual address ffff7dfffee0002e
 Mem abort info:
   ESR = 0x96000046
   Exception class = DABT (current EL), IL = 32 bits
   SET = 0, FnV = 0
   EA = 0, S1PTW = 0
 Data abort info:
   ISV = 0, ISS = 0x00000046
   CM = 0, WnR = 1
 swapper pgtable: 4k pages, 48-bit VAs, pgdp = (____ptrval____)
 [ffff7dfffee0002e] pgd=000000000141c003, pud=000000000141d003, pmd=0000000000000000
 Internal error: Oops: 96000046 [#1] PREEMPT SMP
 Modules linked in: f71882fg(+)
 CPU: 8 PID: 2732 Comm: insmod Not tainted 5.1.0-rc1-00002-gab1a0e9200b8-dirty #102
 Hardware name: Huawei Taishan 2280 /D05, BIOS Hisilicon D05 IT21 Nemo 2.0 RC0 04/18/2018
 pstate: 80000005 (Nzcv daif -PAN -UAO)
 pc : logic_outb+0x54/0xb8
 lr : f71882fg_find+0x64/0x390 [f71882fg]
 sp : ffff000013393aa0
 x29: ffff000013393aa0 x28: ffff000008b98b10
 x27: ffff000013393df0 x26: 0000000000000100
 x25: ffff801f8c872d30 x24: ffff000011420000
 x23: ffff801fb49d2940 x22: ffff000011291000
 x21: 000000000000002e x20: 0000000000000087
 x19: ffff000013393b44 x18: ffffffffffffffff
 x17: 0000000000000000 x16: 0000000000000000
 x15: ffff00001127d6c8 x14: ffff801f8cfd691c
 x13: 0000000000000000 x12: 0000000000000000
 x11: 0000000000000003 x10: 0000801feace2000
 x9 : 0000000000000000 x8 : ffff841fa654f280
 x7 : 0000000000000000 x6 : 0000000000ffc0e3
 x5 : ffff000011291360 x4 : ffff801fb4949f00
 x3 : 0000000000ffbffe x2 : 76e767a63713d500
 x1 : ffff7dfffee0002e x0 : ffff7dfffee00000
 Process insmod (pid: 2732, stack limit = 0x(____ptrval____))
 Call trace:
  logic_outb+0x54/0xb8
  f71882fg_find+0x64/0x390 [f71882fg]
  f71882fg_init+0x38/0xc70 [f71882fg]
  do_one_initcall+0x5c/0x198
  do_init_module+0x54/0x1b0
  load_module+0x1dc4/0x2158
  __se_sys_init_module+0x14c/0x1e8
  __arm64_sys_init_module+0x18/0x20
  el0_svc_common+0x5c/0x100
  el0_svc_handler+0x2c/0x80
  el0_svc+0x8/0xc
 Code: d2bfdc00 f2cfbfe0 f2ffffe0 8b000021 (39000034)
 ---[ end trace fd4f35b610829a48 ]---
Segmentation fault
root@(none)$

Note that the f71882fg driver correctly calls request_muxed_region().

This issue was originally reported in [1].

This patch changes the functionality of request{muxed_}_region() to
request a region from a direct child descendent of the top
ioport_resource.

In this, if the IO port region has not been mapped for a particular IO
region, the PCI IO resource would also not have been inserted, and so a
suitable child region will not exist. As such,
request_{muxed_}region() calls will fail.

A side note: there are many drivers in the kernel which fail to even call
request_{muxed_}region() prior to IO port accesses, and they also need to
be fixed (to call request_{muxed_}region(), as appropriate) separately.

[1] https://lore.kernel.org/linux-pci/56F209A9.4040304@huawei.com

Signed-off-by: John Garry <john.garry@huawei.com>
---
 include/linux/ioport.h | 11 ++++++++---
 kernel/resource.c      | 28 ++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index da0ebaec25f0..76288b8783ff 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -217,15 +217,20 @@ static inline bool resource_contains(struct resource *r1, struct resource *r2)
 
 
 /* Convenience shorthand with allocation */
-#define request_region(start,n,name)		__request_region(&ioport_resource, (start), (n), (name), 0)
-#define request_muxed_region(start,n,name)	__request_region(&ioport_resource, (start), (n), (name), IORESOURCE_MUXED)
+#define request_region(start, n, name)		__request_region_from_children(&ioport_resource, (start), (n), (name), 0)
+#define request_muxed_region(start, n, name)	__request_region_from_children(&ioport_resource, (start), (n), (name), IORESOURCE_MUXED)
 #define __request_mem_region(start,n,name, excl) __request_region(&iomem_resource, (start), (n), (name), excl)
 #define request_mem_region(start,n,name) __request_region(&iomem_resource, (start), (n), (name), 0)
 #define request_mem_region_exclusive(start,n,name) \
 	__request_region(&iomem_resource, (start), (n), (name), IORESOURCE_EXCLUSIVE)
 #define rename_region(region, newname) do { (region)->name = (newname); } while (0)
 
-extern struct resource * __request_region(struct resource *,
+extern struct resource *__request_region(struct resource *,
+					resource_size_t start,
+					resource_size_t n,
+					const char *name, int flags);
+
+extern struct resource *__request_region_from_children(struct resource *,
 					resource_size_t start,
 					resource_size_t n,
 					const char *name, int flags);
diff --git a/kernel/resource.c b/kernel/resource.c
index 92190f62ebc5..87ed200eda8b 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -1097,6 +1097,34 @@ resource_size_t resource_alignment(struct resource *res)
 
 static DECLARE_WAIT_QUEUE_HEAD(muxed_resource_wait);
 
+/**
+ * __request_region_from_children - create a new busy region from a child
+ * @parent: parent resource descriptor
+ * @start: resource start address
+ * @n: resource region size
+ * @name: reserving caller's ID string
+ * @flags: IO resource flags
+ */
+struct resource *__request_region_from_children(struct resource *parent,
+						resource_size_t start,
+						resource_size_t n,
+						const char *name, int flags)
+{
+	struct resource *res = __request_region(parent, start, n, name, flags);
+
+	if (res && res->parent == parent) {
+		/*
+		 * This is a direct descendent of the parent, which is
+		 * what we didn't want.
+		 */
+		__release_region(parent, start, n);
+		res = NULL;
+	}
+
+	return res;
+}
+EXPORT_SYMBOL(__request_region_from_children);
+
 /**
  * __request_region - create a new busy resource region
  * @parent: parent resource descriptor
-- 
2.17.1

[RFC PATCH v3 1/4] resource: Request IO port regions from children of ioport_resource

[John Garry]

Currently request_region() requests an IO port region directly from the
top resource, ioport_resource.

There is an issue here, in that drivers may successfully request an IO
port region even if the IO port region has not even been mapped in
(in pci_remap_iospace()).

This may lead to crashes when the system has no PCI host, or, has a host
but it has failed enumeration, while drivers still attempt to access PCI
IO ports, as below:

root@(none)$root@(none)$ insmod f71882fg.ko
 Unable to handle kernel paging request at virtual address ffff7dfffee0002e
 Mem abort info:
   ESR = 0x96000046
   Exception class = DABT (current EL), IL = 32 bits
   SET = 0, FnV = 0
   EA = 0, S1PTW = 0
 Data abort info:
   ISV = 0, ISS = 0x00000046
   CM = 0, WnR = 1
 swapper pgtable: 4k pages, 48-bit VAs, pgdp = (____ptrval____)
 [ffff7dfffee0002e] pgd=000000000141c003, pud=000000000141d003, pmd=0000000000000000
 Internal error: Oops: 96000046 [#1] PREEMPT SMP
 Modules linked in: f71882fg(+)
 CPU: 8 PID: 2732 Comm: insmod Not tainted 5.1.0-rc1-00002-gab1a0e9200b8-dirty #102
 Hardware name: Huawei Taishan 2280 /D05, BIOS Hisilicon D05 IT21 Nemo 2.0 RC0 04/18/2018
 pstate: 80000005 (Nzcv daif -PAN -UAO)
 pc : logic_outb+0x54/0xb8
 lr : f71882fg_find+0x64/0x390 [f71882fg]
 sp : ffff000013393aa0
 x29: ffff000013393aa0 x28: ffff000008b98b10
 x27: ffff000013393df0 x26: 0000000000000100
 x25: ffff801f8c872d30 x24: ffff000011420000
 x23: ffff801fb49d2940 x22: ffff000011291000
 x21: 000000000000002e x20: 0000000000000087
 x19: ffff000013393b44 x18: ffffffffffffffff
 x17: 0000000000000000 x16: 0000000000000000
 x15: ffff00001127d6c8 x14: ffff801f8cfd691c
 x13: 0000000000000000 x12: 0000000000000000
 x11: 0000000000000003 x10: 0000801feace2000
 x9 : 0000000000000000 x8 : ffff841fa654f280
 x7 : 0000000000000000 x6 : 0000000000ffc0e3
 x5 : ffff000011291360 x4 : ffff801fb4949f00
 x3 : 0000000000ffbffe x2 : 76e767a63713d500
 x1 : ffff7dfffee0002e x0 : ffff7dfffee00000
 Process insmod (pid: 2732, stack limit = 0x(____ptrval____))
 Call trace:
  logic_outb+0x54/0xb8
  f71882fg_find+0x64/0x390 [f71882fg]
  f71882fg_init+0x38/0xc70 [f71882fg]
  do_one_initcall+0x5c/0x198
  do_init_module+0x54/0x1b0
  load_module+0x1dc4/0x2158
  __se_sys_init_module+0x14c/0x1e8
  __arm64_sys_init_module+0x18/0x20
  el0_svc_common+0x5c/0x100
  el0_svc_handler+0x2c/0x80
  el0_svc+0x8/0xc
 Code: d2bfdc00 f2cfbfe0 f2ffffe0 8b000021 (39000034)
 ---[ end trace fd4f35b610829a48 ]---
Segmentation fault
root@(none)$

Note that the f71882fg driver correctly calls request_muxed_region().

This issue was originally reported in [1].

This patch changes the functionality of request{muxed_}_region() to
request a region from a direct child descendent of the top
ioport_resource.

In this, if the IO port region has not been mapped for a particular IO
region, the PCI IO resource would also not have been inserted, and so a
suitable child region will not exist. As such,
request_{muxed_}region() calls will fail.

A side note: there are many drivers in the kernel which fail to even call
request_{muxed_}region() prior to IO port accesses, and they also need to
be fixed (to call request_{muxed_}region(), as appropriate) separately.

[1] https://lore.kernel.org/linux-pci/56F209A9.4040304@huawei.com

Signed-off-by: John Garry <john.garry@huawei.com>
---
 include/linux/ioport.h | 11 ++++++++---
 kernel/resource.c      | 28 ++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 3 deletions(-)

diff --git a/include/linux/ioport.h b/include/linux/ioport.h
index da0ebaec25f0..76288b8783ff 100644
--- a/include/linux/ioport.h
+++ b/include/linux/ioport.h
@@ -217,15 +217,20 @@ static inline bool resource_contains(struct resource *r1, struct resource *r2)
 
 
 /* Convenience shorthand with allocation */
-#define request_region(start,n,name)		__request_region(&ioport_resource, (start), (n), (name), 0)
-#define request_muxed_region(start,n,name)	__request_region(&ioport_resource, (start), (n), (name), IORESOURCE_MUXED)
+#define request_region(start, n, name)		__request_region_from_children(&ioport_resource, (start), (n), (name), 0)
+#define request_muxed_region(start, n, name)	__request_region_from_children(&ioport_resource, (start), (n), (name), IORESOURCE_MUXED)
 #define __request_mem_region(start,n,name, excl) __request_region(&iomem_resource, (start), (n), (name), excl)
 #define request_mem_region(start,n,name) __request_region(&iomem_resource, (start), (n), (name), 0)
 #define request_mem_region_exclusive(start,n,name) \
 	__request_region(&iomem_resource, (start), (n), (name), IORESOURCE_EXCLUSIVE)
 #define rename_region(region, newname) do { (region)->name = (newname); } while (0)
 
-extern struct resource * __request_region(struct resource *,
+extern struct resource *__request_region(struct resource *,
+					resource_size_t start,
+					resource_size_t n,
+					const char *name, int flags);
+
+extern struct resource *__request_region_from_children(struct resource *,
 					resource_size_t start,
 					resource_size_t n,
 					const char *name, int flags);
diff --git a/kernel/resource.c b/kernel/resource.c
index 92190f62ebc5..87ed200eda8b 100644
--- a/kernel/resource.c
+++ b/kernel/resource.c
@@ -1097,6 +1097,34 @@ resource_size_t resource_alignment(struct resource *res)
 
 static DECLARE_WAIT_QUEUE_HEAD(muxed_resource_wait);
 
+/**
+ * __request_region_from_children - create a new busy region from a child
+ * @parent: parent resource descriptor
+ * @start: resource start address
+ * @n: resource region size
+ * @name: reserving caller's ID string
+ * @flags: IO resource flags
+ */
+struct resource *__request_region_from_children(struct resource *parent,
+						resource_size_t start,
+						resource_size_t n,
+						const char *name, int flags)
+{
+	struct resource *res = __request_region(parent, start, n, name, flags);
+
+	if (res && res->parent == parent) {
+		/*
+		 * This is a direct descendent of the parent, which is
+		 * what we didn't want.
+		 */
+		__release_region(parent, start, n);
+		res = NULL;
+	}
+
+	return res;
+}
+EXPORT_SYMBOL(__request_region_from_children);
+
 /**
  * __request_region - create a new busy resource region
  * @parent: parent resource descriptor
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 2/4] lib: logic_pio: Use logical PIO low-level accessors for !CONFIG_INDIRECT_PIO

[John Garry]

Currently we only use logical PIO low-level accessors for when
CONFIG_INDIRECT_PIO is enabled.

Otherwise we just use inb/out et all directly.

It is useful to now use logical PIO accessors for all cases, so we can add
legality checks to accesses. Such a check would be for ensuring that the
PCI IO port has been IO remapped prior to the access.

Using the logical PIO accesses will add a little processing overhead, but
that's ok as IO port accesses are relatively slow anyway.

Some changes are also made to stop spilling so many lines under
CONFIG_INDIRECT_PIO.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 include/linux/logic_pio.h |  7 +++--
 lib/logic_pio.c           | 61 ++++++++++++++++++++++++++++++---------
 2 files changed, 52 insertions(+), 16 deletions(-)

diff --git a/include/linux/logic_pio.h b/include/linux/logic_pio.h
index cbd9d8495690..06d22b2ec99f 100644
--- a/include/linux/logic_pio.h
+++ b/include/linux/logic_pio.h
@@ -37,7 +37,7 @@ struct logic_pio_host_ops {
 		     size_t dwidth, unsigned int count);
 };
 
-#ifdef CONFIG_INDIRECT_PIO
+#if defined(PCI_IOBASE)
 u8 logic_inb(unsigned long addr);
 void logic_outb(u8 value, unsigned long addr);
 void logic_outw(u16 value, unsigned long addr);
@@ -102,6 +102,7 @@ void logic_outsl(unsigned long addr, const void *buffer, unsigned int count);
 #define outsl logic_outsl
 #endif
 
+#if defined(CONFIG_INDIRECT_PIO)
 /*
  * We reserve 0x4000 bytes for Indirect IO as so far this library is only
  * used by the HiSilicon LPC Host. If needed, we can reserve a wider IO
@@ -109,10 +110,10 @@ void logic_outsl(unsigned long addr, const void *buffer, unsigned int count);
  */
 #define PIO_INDIRECT_SIZE 0x4000
 #define MMIO_UPPER_LIMIT (IO_SPACE_LIMIT - PIO_INDIRECT_SIZE)
-#else
+#else /* CONFIG_INDIRECT_PIO */
 #define MMIO_UPPER_LIMIT IO_SPACE_LIMIT
 #endif /* CONFIG_INDIRECT_PIO */
-
+#endif /* PCI_IOBASE */
 struct logic_pio_hwaddr *find_io_range_by_fwnode(struct fwnode_handle *fwnode);
 unsigned long logic_pio_trans_hwaddr(struct fwnode_handle *fwnode,
 			resource_size_t hw_addr, resource_size_t size);
diff --git a/lib/logic_pio.c b/lib/logic_pio.c
index feea48fd1a0d..431cd8d99236 100644
--- a/lib/logic_pio.c
+++ b/lib/logic_pio.c
@@ -191,7 +191,8 @@ unsigned long logic_pio_trans_cpuaddr(resource_size_t addr)
 	return ~0UL;
 }
 
-#if defined(CONFIG_INDIRECT_PIO) && defined(PCI_IOBASE)
+#if defined(PCI_IOBASE)
+#if defined(CONFIG_INDIRECT_PIO)
 #define BUILD_LOGIC_IO(bw, type)					\
 type logic_in##bw(unsigned long addr)					\
 {									\
@@ -201,10 +202,10 @@ type logic_in##bw(unsigned long addr)					\
 		ret = read##bw(PCI_IOBASE + addr);			\
 	} else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) { \
 		struct logic_pio_hwaddr *entry = find_io_range(addr);	\
+		size_t sz = sizeof(type);				\
 									\
 		if (entry && entry->ops)				\
-			ret = entry->ops->in(entry->hostdata,		\
-					addr, sizeof(type));		\
+			ret = entry->ops->in(entry->hostdata, addr, sz);\
 		else							\
 			WARN_ON_ONCE(1);				\
 	}								\
@@ -217,48 +218,82 @@ void logic_out##bw(type value, unsigned long addr)			\
 		write##bw(value, PCI_IOBASE + addr);			\
 	} else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {	\
 		struct logic_pio_hwaddr *entry = find_io_range(addr);	\
+		size_t sz = sizeof(type);				\
 									\
 		if (entry && entry->ops)				\
 			entry->ops->out(entry->hostdata,		\
-					addr, value, sizeof(type));	\
+					addr, value, sz);		\
 		else							\
 			WARN_ON_ONCE(1);				\
 	}								\
 }									\
 									\
-void logic_ins##bw(unsigned long addr, void *buffer,		\
-		   unsigned int count)					\
+void logic_ins##bw(unsigned long addr, void *buf, unsigned int cnt)	\
 {									\
 	if (addr < MMIO_UPPER_LIMIT) {					\
-		reads##bw(PCI_IOBASE + addr, buffer, count);		\
+		reads##bw(PCI_IOBASE + addr, buf, cnt);			\
 	} else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {	\
 		struct logic_pio_hwaddr *entry = find_io_range(addr);	\
+		size_t sz = sizeof(type);				\
 									\
 		if (entry && entry->ops)				\
 			entry->ops->ins(entry->hostdata,		\
-				addr, buffer, sizeof(type), count);	\
+				addr, buf, sz, cnt);			\
 		else							\
 			WARN_ON_ONCE(1);				\
 	}								\
 									\
 }									\
 									\
-void logic_outs##bw(unsigned long addr, const void *buffer,		\
-		    unsigned int count)					\
+void logic_outs##bw(unsigned long addr, const void *buf,		\
+		    unsigned int cnt)					\
 {									\
 	if (addr < MMIO_UPPER_LIMIT) {					\
-		writes##bw(PCI_IOBASE + addr, buffer, count);		\
+		writes##bw(PCI_IOBASE + addr, buf, cnt);		\
 	} else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {	\
 		struct logic_pio_hwaddr *entry = find_io_range(addr);	\
+		size_t sz = sizeof(type);				\
 									\
 		if (entry && entry->ops)				\
 			entry->ops->outs(entry->hostdata,		\
-				addr, buffer, sizeof(type), count);	\
+				addr, buf, sz, cnt);			\
 		else							\
 			WARN_ON_ONCE(1);				\
 	}								\
 }
 
+#else /* CONFIG_INDIRECT_PIO */
+
+#define BUILD_LOGIC_IO(bw, type)					\
+type logic_in##bw(unsigned long addr)					\
+{									\
+	type ret = (type)~0;						\
+									\
+	if (addr < MMIO_UPPER_LIMIT)					\
+		ret = read##bw(PCI_IOBASE + addr);			\
+	return ret;							\
+}									\
+									\
+void logic_out##bw(type value, unsigned long addr)			\
+{									\
+	if (addr < MMIO_UPPER_LIMIT)					\
+		write##bw(value, PCI_IOBASE + addr);			\
+}									\
+									\
+void logic_ins##bw(unsigned long addr, void *buf, unsigned int cnt)	\
+{									\
+	if (addr < MMIO_UPPER_LIMIT)					\
+		reads##bw(PCI_IOBASE + addr, buf, cnt);			\
+}									\
+									\
+void logic_outs##bw(unsigned long addr, const void *buf,		\
+		    unsigned int cnt)					\
+{									\
+	if (addr < MMIO_UPPER_LIMIT)					\
+		writes##bw(PCI_IOBASE + addr, buf, cnt);		\
+}
+#endif /* CONFIG_INDIRECT_PIO */
+
 BUILD_LOGIC_IO(b, u8)
 EXPORT_SYMBOL(logic_inb);
 EXPORT_SYMBOL(logic_insb);
@@ -277,4 +312,4 @@ EXPORT_SYMBOL(logic_insl);
 EXPORT_SYMBOL(logic_outl);
 EXPORT_SYMBOL(logic_outsl);
 
-#endif /* CONFIG_INDIRECT_PIO && PCI_IOBASE */
+#endif /* PCI_IOBASE */
-- 
2.17.1

[PATCH v3 2/4] lib: logic_pio: Use logical PIO low-level accessors for !CONFIG_INDIRECT_PIO

[John Garry]

Currently we only use logical PIO low-level accessors for when
CONFIG_INDIRECT_PIO is enabled.

Otherwise we just use inb/out et all directly.

It is useful to now use logical PIO accessors for all cases, so we can add
legality checks to accesses. Such a check would be for ensuring that the
PCI IO port has been IO remapped prior to the access.

Using the logical PIO accesses will add a little processing overhead, but
that's ok as IO port accesses are relatively slow anyway.

Some changes are also made to stop spilling so many lines under
CONFIG_INDIRECT_PIO.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 include/linux/logic_pio.h |  7 +++--
 lib/logic_pio.c           | 61 ++++++++++++++++++++++++++++++---------
 2 files changed, 52 insertions(+), 16 deletions(-)

diff --git a/include/linux/logic_pio.h b/include/linux/logic_pio.h
index cbd9d8495690..06d22b2ec99f 100644
--- a/include/linux/logic_pio.h
+++ b/include/linux/logic_pio.h
@@ -37,7 +37,7 @@ struct logic_pio_host_ops {
 		     size_t dwidth, unsigned int count);
 };
 
-#ifdef CONFIG_INDIRECT_PIO
+#if defined(PCI_IOBASE)
 u8 logic_inb(unsigned long addr);
 void logic_outb(u8 value, unsigned long addr);
 void logic_outw(u16 value, unsigned long addr);
@@ -102,6 +102,7 @@ void logic_outsl(unsigned long addr, const void *buffer, unsigned int count);
 #define outsl logic_outsl
 #endif
 
+#if defined(CONFIG_INDIRECT_PIO)
 /*
  * We reserve 0x4000 bytes for Indirect IO as so far this library is only
  * used by the HiSilicon LPC Host. If needed, we can reserve a wider IO
@@ -109,10 +110,10 @@ void logic_outsl(unsigned long addr, const void *buffer, unsigned int count);
  */
 #define PIO_INDIRECT_SIZE 0x4000
 #define MMIO_UPPER_LIMIT (IO_SPACE_LIMIT - PIO_INDIRECT_SIZE)
-#else
+#else /* CONFIG_INDIRECT_PIO */
 #define MMIO_UPPER_LIMIT IO_SPACE_LIMIT
 #endif /* CONFIG_INDIRECT_PIO */
-
+#endif /* PCI_IOBASE */
 struct logic_pio_hwaddr *find_io_range_by_fwnode(struct fwnode_handle *fwnode);
 unsigned long logic_pio_trans_hwaddr(struct fwnode_handle *fwnode,
 			resource_size_t hw_addr, resource_size_t size);
diff --git a/lib/logic_pio.c b/lib/logic_pio.c
index feea48fd1a0d..431cd8d99236 100644
--- a/lib/logic_pio.c
+++ b/lib/logic_pio.c
@@ -191,7 +191,8 @@ unsigned long logic_pio_trans_cpuaddr(resource_size_t addr)
 	return ~0UL;
 }
 
-#if defined(CONFIG_INDIRECT_PIO) && defined(PCI_IOBASE)
+#if defined(PCI_IOBASE)
+#if defined(CONFIG_INDIRECT_PIO)
 #define BUILD_LOGIC_IO(bw, type)					\
 type logic_in##bw(unsigned long addr)					\
 {									\
@@ -201,10 +202,10 @@ type logic_in##bw(unsigned long addr)					\
 		ret = read##bw(PCI_IOBASE + addr);			\
 	} else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) { \
 		struct logic_pio_hwaddr *entry = find_io_range(addr);	\
+		size_t sz = sizeof(type);				\
 									\
 		if (entry && entry->ops)				\
-			ret = entry->ops->in(entry->hostdata,		\
-					addr, sizeof(type));		\
+			ret = entry->ops->in(entry->hostdata, addr, sz);\
 		else							\
 			WARN_ON_ONCE(1);				\
 	}								\
@@ -217,48 +218,82 @@ void logic_out##bw(type value, unsigned long addr)			\
 		write##bw(value, PCI_IOBASE + addr);			\
 	} else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {	\
 		struct logic_pio_hwaddr *entry = find_io_range(addr);	\
+		size_t sz = sizeof(type);				\
 									\
 		if (entry && entry->ops)				\
 			entry->ops->out(entry->hostdata,		\
-					addr, value, sizeof(type));	\
+					addr, value, sz);		\
 		else							\
 			WARN_ON_ONCE(1);				\
 	}								\
 }									\
 									\
-void logic_ins##bw(unsigned long addr, void *buffer,		\
-		   unsigned int count)					\
+void logic_ins##bw(unsigned long addr, void *buf, unsigned int cnt)	\
 {									\
 	if (addr < MMIO_UPPER_LIMIT) {					\
-		reads##bw(PCI_IOBASE + addr, buffer, count);		\
+		reads##bw(PCI_IOBASE + addr, buf, cnt);			\
 	} else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {	\
 		struct logic_pio_hwaddr *entry = find_io_range(addr);	\
+		size_t sz = sizeof(type);				\
 									\
 		if (entry && entry->ops)				\
 			entry->ops->ins(entry->hostdata,		\
-				addr, buffer, sizeof(type), count);	\
+				addr, buf, sz, cnt);			\
 		else							\
 			WARN_ON_ONCE(1);				\
 	}								\
 									\
 }									\
 									\
-void logic_outs##bw(unsigned long addr, const void *buffer,		\
-		    unsigned int count)					\
+void logic_outs##bw(unsigned long addr, const void *buf,		\
+		    unsigned int cnt)					\
 {									\
 	if (addr < MMIO_UPPER_LIMIT) {					\
-		writes##bw(PCI_IOBASE + addr, buffer, count);		\
+		writes##bw(PCI_IOBASE + addr, buf, cnt);		\
 	} else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {	\
 		struct logic_pio_hwaddr *entry = find_io_range(addr);	\
+		size_t sz = sizeof(type);				\
 									\
 		if (entry && entry->ops)				\
 			entry->ops->outs(entry->hostdata,		\
-				addr, buffer, sizeof(type), count);	\
+				addr, buf, sz, cnt);			\
 		else							\
 			WARN_ON_ONCE(1);				\
 	}								\
 }
 
+#else /* CONFIG_INDIRECT_PIO */
+
+#define BUILD_LOGIC_IO(bw, type)					\
+type logic_in##bw(unsigned long addr)					\
+{									\
+	type ret = (type)~0;						\
+									\
+	if (addr < MMIO_UPPER_LIMIT)					\
+		ret = read##bw(PCI_IOBASE + addr);			\
+	return ret;							\
+}									\
+									\
+void logic_out##bw(type value, unsigned long addr)			\
+{									\
+	if (addr < MMIO_UPPER_LIMIT)					\
+		write##bw(value, PCI_IOBASE + addr);			\
+}									\
+									\
+void logic_ins##bw(unsigned long addr, void *buf, unsigned int cnt)	\
+{									\
+	if (addr < MMIO_UPPER_LIMIT)					\
+		reads##bw(PCI_IOBASE + addr, buf, cnt);			\
+}									\
+									\
+void logic_outs##bw(unsigned long addr, const void *buf,		\
+		    unsigned int cnt)					\
+{									\
+	if (addr < MMIO_UPPER_LIMIT)					\
+		writes##bw(PCI_IOBASE + addr, buf, cnt);		\
+}
+#endif /* CONFIG_INDIRECT_PIO */
+
 BUILD_LOGIC_IO(b, u8)
 EXPORT_SYMBOL(logic_inb);
 EXPORT_SYMBOL(logic_insb);
@@ -277,4 +312,4 @@ EXPORT_SYMBOL(logic_insl);
 EXPORT_SYMBOL(logic_outl);
 EXPORT_SYMBOL(logic_outsl);
 
-#endif /* CONFIG_INDIRECT_PIO && PCI_IOBASE */
+#endif /* PCI_IOBASE */
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 3/4] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions

[John Garry]

Currently when accessing logical indirect PIO addresses in
logic_{in, out}{,s}, we first ensure that the region is registered.

However, no such check exists for CPU MMIO regions. The CPU MMIO regions
would be registered by the PCI host - when PCI_IOBASE is defined - in
pci_register_io_range().

We have seen scenarios when systems which don't have a PCI host or, they
do, and the PCI host probe fails, that certain devices attempts to still
attempt to access PCI IO ports; examples are in [1] and [2].

And even though we should protect against this by ensuring the driver
calls request_{muxed_}region(), some don't do this:

root@(none)$ insmod hwmon/f71805f.ko
 Unable to handle kernel paging request at virtual address ffff7dfffee0002e
 Mem abort info:
   ESR = 0x96000046
   Exception class = DABT (current EL), IL = 32 bits
   SET = 0, FnV = 0
   EA = 0, S1PTW = 0
 Data abort info:
   ISV = 0, ISS = 0x00000046
   CM = 0, WnR = 1
 swapper pgtable: 4k pages, 48-bit VAs, pgdp = (____ptrval____)
 [ffff7dfffee0002e] pgd=000000000141c003, pud=000000000141d003, pmd=0000000000000000
 Internal error: Oops: 96000046 [#1] PREEMPT SMP
 Modules linked in: f71805f(+)
 CPU: 20 PID: 2736 Comm: insmod Not tainted 5.1.0-rc1-00003-g6f1bfec2a620-dirty #99
 Hardware name: Huawei Taishan 2280 /D05, BIOS Hisilicon D05 IT21 Nemo 2.0 RC0 04/18/2018
 pstate: 80000005 (Nzcv daif -PAN -UAO)
 pc : logic_outb+0x54/0xb8
 lr : f71805f_find+0x2c/0x1b8 [f71805f]
 sp : ffff000025fbba90
 x29: ffff000025fbba90 x28: ffff000008b944d0
 x27: ffff000025fbbdf0 x26: 0000000000000100
 x25: ffff801f8c270580 x24: ffff000011420000
 x23: ffff000025fbbb3e x22: ffff000025fbbb40
 x21: ffff000008b991b8 x20: 0000000000000087
 x19: 000000000000002e x18: ffffffffffffffff
 x17: 0000000000000000 x16: 0000000000000000
 x15: ffff00001127d6c8 x14: 0000000000000000
 x13: 0000000000000000 x12: 0000000000000000
 x11: 0000000000010820 x10: 0000841fdac40000
 x9 : 0000000000000001 x8 : 0000000040000000
 x7 : 0000000000210d00 x6 : 0000000000000000
 x5 : ffff801fb6a46040 x4 : ffff841febeaeda0
 x3 : 0000000000ffbffe x2 : ffff000025fbbb40
 x1 : ffff7dfffee0002e x0 : ffff7dfffee00000
 Process insmod (pid: 2736, stack limit = 0x(____ptrval____))
 Call trace:
  logic_outb+0x54/0xb8
  f71805f_find+0x2c/0x1b8 [f71805f]
  f71805f_init+0x38/0xe48 [f71805f]
  do_one_initcall+0x5c/0x198
  do_init_module+0x54/0x1b0
  load_module+0x1dc4/0x2158
  __se_sys_init_module+0x14c/0x1e8
  __arm64_sys_init_module+0x18/0x20
  el0_svc_common+0x5c/0x100
  el0_svc_handler+0x2c/0x80
  el0_svc+0x8/0xc
 Code: d2bfdc00 f2cfbfe0 f2ffffe0 8b000021 (39000034)
 ---[ end trace 10ea80bde051bbfc ]---
root@(none)$

Note that the f71805f driver does not call request_{muxed_}region(), as it
should.

This patch adds a check to ensure that the CPU MMIO region is registered
prior to accessing the PCI IO ports.

[1] https://lore.kernel.org/linux-pci/56F209A9.4040304@huawei.com
[2] https://lore.kernel.org/linux-arm-kernel/e6995b4a-184a-d8d4-f4d4-9ce75d8f47c0@huawei.com/

This patch includes some other tidy-up.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 lib/logic_pio.c | 103 +++++++++++++++++++++++++++++++++++-------------
 1 file changed, 75 insertions(+), 28 deletions(-)

diff --git a/lib/logic_pio.c b/lib/logic_pio.c
index 431cd8d99236..3d8d986e9dcb 100644
--- a/lib/logic_pio.c
+++ b/lib/logic_pio.c
@@ -193,95 +193,135 @@ unsigned long logic_pio_trans_cpuaddr(resource_size_t addr)
 
 #if defined(PCI_IOBASE)
 #if defined(CONFIG_INDIRECT_PIO)
+#define INVALID_RANGE(range)						\
+	(!(range) || ((range)->flags == LOGIC_PIO_INDIRECT && !(range)->ops))
+
 #define BUILD_LOGIC_IO(bw, type)					\
 type logic_in##bw(unsigned long addr)					\
 {									\
 	type ret = (type)~0;						\
+	struct logic_pio_hwaddr *range = find_io_range(addr);		\
+									\
+	if (INVALID_RANGE(range)) {					\
+		WARN_ON_ONCE(1);					\
+		return ret;						\
+	}								\
 									\
 	if (addr < MMIO_UPPER_LIMIT) {					\
 		ret = read##bw(PCI_IOBASE + addr);			\
 	} else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) { \
-		struct logic_pio_hwaddr *entry = find_io_range(addr);	\
 		size_t sz = sizeof(type);				\
+		void *hostdata = range->hostdata;			\
 									\
-		if (entry && entry->ops)				\
-			ret = entry->ops->in(entry->hostdata, addr, sz);\
-		else							\
-			WARN_ON_ONCE(1);				\
+		if (range->ops->in)					\
+			ret = range->ops->in(hostdata, addr, sz);	\
 	}								\
 	return ret;							\
 }									\
 									\
-void logic_out##bw(type value, unsigned long addr)			\
+void logic_out##bw(type val, unsigned long addr)			\
 {									\
+	struct logic_pio_hwaddr *range = find_io_range(addr);		\
+									\
+	if (INVALID_RANGE(range)) {					\
+		WARN_ON_ONCE(1);					\
+		return;							\
+	}								\
+									\
 	if (addr < MMIO_UPPER_LIMIT) {					\
-		write##bw(value, PCI_IOBASE + addr);			\
+		write##bw(val, PCI_IOBASE + addr);			\
 	} else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {	\
-		struct logic_pio_hwaddr *entry = find_io_range(addr);	\
 		size_t sz = sizeof(type);				\
+		void *hostdata = range->hostdata;			\
 									\
-		if (entry && entry->ops)				\
-			entry->ops->out(entry->hostdata,		\
-					addr, value, sz);		\
-		else							\
-			WARN_ON_ONCE(1);				\
+		if (range->ops->out)					\
+			range->ops->out(hostdata, addr, val, sz);	\
 	}								\
 }									\
 									\
 void logic_ins##bw(unsigned long addr, void *buf, unsigned int cnt)	\
 {									\
+	struct logic_pio_hwaddr *range = find_io_range(addr);		\
+									\
+	if (INVALID_RANGE(range)) {					\
+		WARN_ON_ONCE(1);					\
+		return;							\
+	}								\
+									\
 	if (addr < MMIO_UPPER_LIMIT) {					\
 		reads##bw(PCI_IOBASE + addr, buf, cnt);			\
 	} else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {	\
-		struct logic_pio_hwaddr *entry = find_io_range(addr);	\
 		size_t sz = sizeof(type);				\
+		void *hostdata = range->hostdata;			\
 									\
-		if (entry && entry->ops)				\
-			entry->ops->ins(entry->hostdata,		\
-				addr, buf, sz, cnt);			\
-		else							\
-			WARN_ON_ONCE(1);				\
+		if (range->ops->ins)					\
+			range->ops->ins(hostdata, addr, buf, sz, cnt);	\
 	}								\
-									\
 }									\
 									\
 void logic_outs##bw(unsigned long addr, const void *buf,		\
 		    unsigned int cnt)					\
 {									\
+	struct logic_pio_hwaddr *range = find_io_range(addr);		\
+									\
+	if (INVALID_RANGE(range)) {					\
+		WARN_ON_ONCE(1);					\
+		return;							\
+	}								\
+									\
 	if (addr < MMIO_UPPER_LIMIT) {					\
 		writes##bw(PCI_IOBASE + addr, buf, cnt);		\
 	} else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {	\
-		struct logic_pio_hwaddr *entry = find_io_range(addr);	\
 		size_t sz = sizeof(type);				\
+		void *hostdata = range->hostdata;			\
 									\
-		if (entry && entry->ops)				\
-			entry->ops->outs(entry->hostdata,		\
-				addr, buf, sz, cnt);			\
-		else							\
-			WARN_ON_ONCE(1);				\
+		if (range->ops->outs)					\
+			range->ops->outs(hostdata, addr, buf, sz, cnt);	\
 	}								\
 }
 
 #else /* CONFIG_INDIRECT_PIO */
 
+#define INVALID_RANGE(range) (!(range))
+
 #define BUILD_LOGIC_IO(bw, type)					\
 type logic_in##bw(unsigned long addr)					\
 {									\
 	type ret = (type)~0;						\
+	struct logic_pio_hwaddr *range = find_io_range(addr);		\
+									\
+	if (INVALID_RANGE(range)) {					\
+		WARN_ON_ONCE(1);					\
+		return ret;						\
+	}								\
 									\
 	if (addr < MMIO_UPPER_LIMIT)					\
 		ret = read##bw(PCI_IOBASE + addr);			\
 	return ret;							\
 }									\
 									\
-void logic_out##bw(type value, unsigned long addr)			\
+void logic_out##bw(type val, unsigned long addr)			\
 {									\
+	struct logic_pio_hwaddr *range = find_io_range(addr);		\
+									\
+	if (INVALID_RANGE(range)) {					\
+		WARN_ON_ONCE(1);					\
+		return;							\
+	}								\
+									\
 	if (addr < MMIO_UPPER_LIMIT)					\
-		write##bw(value, PCI_IOBASE + addr);			\
+		write##bw(val, PCI_IOBASE + addr);			\
 }									\
 									\
 void logic_ins##bw(unsigned long addr, void *buf, unsigned int cnt)	\
 {									\
+	struct logic_pio_hwaddr *range = find_io_range(addr);		\
+									\
+	if (INVALID_RANGE(range)) {					\
+		WARN_ON_ONCE(1);					\
+		return;							\
+	}								\
+									\
 	if (addr < MMIO_UPPER_LIMIT)					\
 		reads##bw(PCI_IOBASE + addr, buf, cnt);			\
 }									\
@@ -289,6 +329,13 @@ void logic_ins##bw(unsigned long addr, void *buf, unsigned int cnt)	\
 void logic_outs##bw(unsigned long addr, const void *buf,		\
 		    unsigned int cnt)					\
 {									\
+	struct logic_pio_hwaddr *range = find_io_range(addr);		\
+									\
+	if (INVALID_RANGE(range)) {					\
+		WARN_ON_ONCE(1);					\
+		return;							\
+	}								\
+									\
 	if (addr < MMIO_UPPER_LIMIT)					\
 		writes##bw(PCI_IOBASE + addr, buf, cnt);		\
 }
-- 
2.17.1

[PATCH v3 3/4] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions

[John Garry]

Currently when accessing logical indirect PIO addresses in
logic_{in, out}{,s}, we first ensure that the region is registered.

However, no such check exists for CPU MMIO regions. The CPU MMIO regions
would be registered by the PCI host - when PCI_IOBASE is defined - in
pci_register_io_range().

We have seen scenarios when systems which don't have a PCI host or, they
do, and the PCI host probe fails, that certain devices attempts to still
attempt to access PCI IO ports; examples are in [1] and [2].

And even though we should protect against this by ensuring the driver
calls request_{muxed_}region(), some don't do this:

root@(none)$ insmod hwmon/f71805f.ko
 Unable to handle kernel paging request at virtual address ffff7dfffee0002e
 Mem abort info:
   ESR = 0x96000046
   Exception class = DABT (current EL), IL = 32 bits
   SET = 0, FnV = 0
   EA = 0, S1PTW = 0
 Data abort info:
   ISV = 0, ISS = 0x00000046
   CM = 0, WnR = 1
 swapper pgtable: 4k pages, 48-bit VAs, pgdp = (____ptrval____)
 [ffff7dfffee0002e] pgd=000000000141c003, pud=000000000141d003, pmd=0000000000000000
 Internal error: Oops: 96000046 [#1] PREEMPT SMP
 Modules linked in: f71805f(+)
 CPU: 20 PID: 2736 Comm: insmod Not tainted 5.1.0-rc1-00003-g6f1bfec2a620-dirty #99
 Hardware name: Huawei Taishan 2280 /D05, BIOS Hisilicon D05 IT21 Nemo 2.0 RC0 04/18/2018
 pstate: 80000005 (Nzcv daif -PAN -UAO)
 pc : logic_outb+0x54/0xb8
 lr : f71805f_find+0x2c/0x1b8 [f71805f]
 sp : ffff000025fbba90
 x29: ffff000025fbba90 x28: ffff000008b944d0
 x27: ffff000025fbbdf0 x26: 0000000000000100
 x25: ffff801f8c270580 x24: ffff000011420000
 x23: ffff000025fbbb3e x22: ffff000025fbbb40
 x21: ffff000008b991b8 x20: 0000000000000087
 x19: 000000000000002e x18: ffffffffffffffff
 x17: 0000000000000000 x16: 0000000000000000
 x15: ffff00001127d6c8 x14: 0000000000000000
 x13: 0000000000000000 x12: 0000000000000000
 x11: 0000000000010820 x10: 0000841fdac40000
 x9 : 0000000000000001 x8 : 0000000040000000
 x7 : 0000000000210d00 x6 : 0000000000000000
 x5 : ffff801fb6a46040 x4 : ffff841febeaeda0
 x3 : 0000000000ffbffe x2 : ffff000025fbbb40
 x1 : ffff7dfffee0002e x0 : ffff7dfffee00000
 Process insmod (pid: 2736, stack limit = 0x(____ptrval____))
 Call trace:
  logic_outb+0x54/0xb8
  f71805f_find+0x2c/0x1b8 [f71805f]
  f71805f_init+0x38/0xe48 [f71805f]
  do_one_initcall+0x5c/0x198
  do_init_module+0x54/0x1b0
  load_module+0x1dc4/0x2158
  __se_sys_init_module+0x14c/0x1e8
  __arm64_sys_init_module+0x18/0x20
  el0_svc_common+0x5c/0x100
  el0_svc_handler+0x2c/0x80
  el0_svc+0x8/0xc
 Code: d2bfdc00 f2cfbfe0 f2ffffe0 8b000021 (39000034)
 ---[ end trace 10ea80bde051bbfc ]---
root@(none)$

Note that the f71805f driver does not call request_{muxed_}region(), as it
should.

This patch adds a check to ensure that the CPU MMIO region is registered
prior to accessing the PCI IO ports.

[1] https://lore.kernel.org/linux-pci/56F209A9.4040304@huawei.com
[2] https://lore.kernel.org/linux-arm-kernel/e6995b4a-184a-d8d4-f4d4-9ce75d8f47c0@huawei.com/

This patch includes some other tidy-up.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 lib/logic_pio.c | 103 +++++++++++++++++++++++++++++++++++-------------
 1 file changed, 75 insertions(+), 28 deletions(-)

diff --git a/lib/logic_pio.c b/lib/logic_pio.c
index 431cd8d99236..3d8d986e9dcb 100644
--- a/lib/logic_pio.c
+++ b/lib/logic_pio.c
@@ -193,95 +193,135 @@ unsigned long logic_pio_trans_cpuaddr(resource_size_t addr)
 
 #if defined(PCI_IOBASE)
 #if defined(CONFIG_INDIRECT_PIO)
+#define INVALID_RANGE(range)						\
+	(!(range) || ((range)->flags == LOGIC_PIO_INDIRECT && !(range)->ops))
+
 #define BUILD_LOGIC_IO(bw, type)					\
 type logic_in##bw(unsigned long addr)					\
 {									\
 	type ret = (type)~0;						\
+	struct logic_pio_hwaddr *range = find_io_range(addr);		\
+									\
+	if (INVALID_RANGE(range)) {					\
+		WARN_ON_ONCE(1);					\
+		return ret;						\
+	}								\
 									\
 	if (addr < MMIO_UPPER_LIMIT) {					\
 		ret = read##bw(PCI_IOBASE + addr);			\
 	} else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) { \
-		struct logic_pio_hwaddr *entry = find_io_range(addr);	\
 		size_t sz = sizeof(type);				\
+		void *hostdata = range->hostdata;			\
 									\
-		if (entry && entry->ops)				\
-			ret = entry->ops->in(entry->hostdata, addr, sz);\
-		else							\
-			WARN_ON_ONCE(1);				\
+		if (range->ops->in)					\
+			ret = range->ops->in(hostdata, addr, sz);	\
 	}								\
 	return ret;							\
 }									\
 									\
-void logic_out##bw(type value, unsigned long addr)			\
+void logic_out##bw(type val, unsigned long addr)			\
 {									\
+	struct logic_pio_hwaddr *range = find_io_range(addr);		\
+									\
+	if (INVALID_RANGE(range)) {					\
+		WARN_ON_ONCE(1);					\
+		return;							\
+	}								\
+									\
 	if (addr < MMIO_UPPER_LIMIT) {					\
-		write##bw(value, PCI_IOBASE + addr);			\
+		write##bw(val, PCI_IOBASE + addr);			\
 	} else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {	\
-		struct logic_pio_hwaddr *entry = find_io_range(addr);	\
 		size_t sz = sizeof(type);				\
+		void *hostdata = range->hostdata;			\
 									\
-		if (entry && entry->ops)				\
-			entry->ops->out(entry->hostdata,		\
-					addr, value, sz);		\
-		else							\
-			WARN_ON_ONCE(1);				\
+		if (range->ops->out)					\
+			range->ops->out(hostdata, addr, val, sz);	\
 	}								\
 }									\
 									\
 void logic_ins##bw(unsigned long addr, void *buf, unsigned int cnt)	\
 {									\
+	struct logic_pio_hwaddr *range = find_io_range(addr);		\
+									\
+	if (INVALID_RANGE(range)) {					\
+		WARN_ON_ONCE(1);					\
+		return;							\
+	}								\
+									\
 	if (addr < MMIO_UPPER_LIMIT) {					\
 		reads##bw(PCI_IOBASE + addr, buf, cnt);			\
 	} else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {	\
-		struct logic_pio_hwaddr *entry = find_io_range(addr);	\
 		size_t sz = sizeof(type);				\
+		void *hostdata = range->hostdata;			\
 									\
-		if (entry && entry->ops)				\
-			entry->ops->ins(entry->hostdata,		\
-				addr, buf, sz, cnt);			\
-		else							\
-			WARN_ON_ONCE(1);				\
+		if (range->ops->ins)					\
+			range->ops->ins(hostdata, addr, buf, sz, cnt);	\
 	}								\
-									\
 }									\
 									\
 void logic_outs##bw(unsigned long addr, const void *buf,		\
 		    unsigned int cnt)					\
 {									\
+	struct logic_pio_hwaddr *range = find_io_range(addr);		\
+									\
+	if (INVALID_RANGE(range)) {					\
+		WARN_ON_ONCE(1);					\
+		return;							\
+	}								\
+									\
 	if (addr < MMIO_UPPER_LIMIT) {					\
 		writes##bw(PCI_IOBASE + addr, buf, cnt);		\
 	} else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {	\
-		struct logic_pio_hwaddr *entry = find_io_range(addr);	\
 		size_t sz = sizeof(type);				\
+		void *hostdata = range->hostdata;			\
 									\
-		if (entry && entry->ops)				\
-			entry->ops->outs(entry->hostdata,		\
-				addr, buf, sz, cnt);			\
-		else							\
-			WARN_ON_ONCE(1);				\
+		if (range->ops->outs)					\
+			range->ops->outs(hostdata, addr, buf, sz, cnt);	\
 	}								\
 }
 
 #else /* CONFIG_INDIRECT_PIO */
 
+#define INVALID_RANGE(range) (!(range))
+
 #define BUILD_LOGIC_IO(bw, type)					\
 type logic_in##bw(unsigned long addr)					\
 {									\
 	type ret = (type)~0;						\
+	struct logic_pio_hwaddr *range = find_io_range(addr);		\
+									\
+	if (INVALID_RANGE(range)) {					\
+		WARN_ON_ONCE(1);					\
+		return ret;						\
+	}								\
 									\
 	if (addr < MMIO_UPPER_LIMIT)					\
 		ret = read##bw(PCI_IOBASE + addr);			\
 	return ret;							\
 }									\
 									\
-void logic_out##bw(type value, unsigned long addr)			\
+void logic_out##bw(type val, unsigned long addr)			\
 {									\
+	struct logic_pio_hwaddr *range = find_io_range(addr);		\
+									\
+	if (INVALID_RANGE(range)) {					\
+		WARN_ON_ONCE(1);					\
+		return;							\
+	}								\
+									\
 	if (addr < MMIO_UPPER_LIMIT)					\
-		write##bw(value, PCI_IOBASE + addr);			\
+		write##bw(val, PCI_IOBASE + addr);			\
 }									\
 									\
 void logic_ins##bw(unsigned long addr, void *buf, unsigned int cnt)	\
 {									\
+	struct logic_pio_hwaddr *range = find_io_range(addr);		\
+									\
+	if (INVALID_RANGE(range)) {					\
+		WARN_ON_ONCE(1);					\
+		return;							\
+	}								\
+									\
 	if (addr < MMIO_UPPER_LIMIT)					\
 		reads##bw(PCI_IOBASE + addr, buf, cnt);			\
 }									\
@@ -289,6 +329,13 @@ void logic_ins##bw(unsigned long addr, void *buf, unsigned int cnt)	\
 void logic_outs##bw(unsigned long addr, const void *buf,		\
 		    unsigned int cnt)					\
 {									\
+	struct logic_pio_hwaddr *range = find_io_range(addr);		\
+									\
+	if (INVALID_RANGE(range)) {					\
+		WARN_ON_ONCE(1);					\
+		return;							\
+	}								\
+									\
 	if (addr < MMIO_UPPER_LIMIT)					\
 		writes##bw(PCI_IOBASE + addr, buf, cnt);		\
 }
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/4] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions

[Guenter Roeck]

On Fri, Apr 05, 2019 at 12:00:01AM +0800, John Garry wrote:
> Currently when accessing logical indirect PIO addresses in
> logic_{in, out}{,s}, we first ensure that the region is registered.
> 
> However, no such check exists for CPU MMIO regions. The CPU MMIO regions
> would be registered by the PCI host - when PCI_IOBASE is defined - in
> pci_register_io_range().
> 
> We have seen scenarios when systems which don't have a PCI host or, they
> do, and the PCI host probe fails, that certain devices attempts to still
> attempt to access PCI IO ports; examples are in [1] and [2].
> 
> And even though we should protect against this by ensuring the driver
> calls request_{muxed_}region(), some don't do this:
> 
> root@(none)$ insmod hwmon/f71805f.ko
>  Unable to handle kernel paging request at virtual address ffff7dfffee0002e
>  Mem abort info:
>    ESR = 0x96000046
>    Exception class = DABT (current EL), IL = 32 bits
>    SET = 0, FnV = 0
>    EA = 0, S1PTW = 0
>  Data abort info:
>    ISV = 0, ISS = 0x00000046
>    CM = 0, WnR = 1
>  swapper pgtable: 4k pages, 48-bit VAs, pgdp = (____ptrval____)
>  [ffff7dfffee0002e] pgd=000000000141c003, pud=000000000141d003, pmd=0000000000000000
>  Internal error: Oops: 96000046 [#1] PREEMPT SMP
>  Modules linked in: f71805f(+)
>  CPU: 20 PID: 2736 Comm: insmod Not tainted 5.1.0-rc1-00003-g6f1bfec2a620-dirty #99
>  Hardware name: Huawei Taishan 2280 /D05, BIOS Hisilicon D05 IT21 Nemo 2.0 RC0 04/18/2018
>  pstate: 80000005 (Nzcv daif -PAN -UAO)
>  pc : logic_outb+0x54/0xb8
>  lr : f71805f_find+0x2c/0x1b8 [f71805f]
>  sp : ffff000025fbba90
>  x29: ffff000025fbba90 x28: ffff000008b944d0
>  x27: ffff000025fbbdf0 x26: 0000000000000100
>  x25: ffff801f8c270580 x24: ffff000011420000
>  x23: ffff000025fbbb3e x22: ffff000025fbbb40
>  x21: ffff000008b991b8 x20: 0000000000000087
>  x19: 000000000000002e x18: ffffffffffffffff
>  x17: 0000000000000000 x16: 0000000000000000
>  x15: ffff00001127d6c8 x14: 0000000000000000
>  x13: 0000000000000000 x12: 0000000000000000
>  x11: 0000000000010820 x10: 0000841fdac40000
>  x9 : 0000000000000001 x8 : 0000000040000000
>  x7 : 0000000000210d00 x6 : 0000000000000000
>  x5 : ffff801fb6a46040 x4 : ffff841febeaeda0
>  x3 : 0000000000ffbffe x2 : ffff000025fbbb40
>  x1 : ffff7dfffee0002e x0 : ffff7dfffee00000
>  Process insmod (pid: 2736, stack limit = 0x(____ptrval____))
>  Call trace:
>   logic_outb+0x54/0xb8
>   f71805f_find+0x2c/0x1b8 [f71805f]
>   f71805f_init+0x38/0xe48 [f71805f]
>   do_one_initcall+0x5c/0x198
>   do_init_module+0x54/0x1b0
>   load_module+0x1dc4/0x2158
>   __se_sys_init_module+0x14c/0x1e8
>   __arm64_sys_init_module+0x18/0x20
>   el0_svc_common+0x5c/0x100
>   el0_svc_handler+0x2c/0x80
>   el0_svc+0x8/0xc
>  Code: d2bfdc00 f2cfbfe0 f2ffffe0 8b000021 (39000034)
>  ---[ end trace 10ea80bde051bbfc ]---
> root@(none)$
> 
> Note that the f71805f driver does not call request_{muxed_}region(), as it
> should.
> 
... which is the real problem, one that is not solved by this patch. This may
result in parallel and descructive accesses if there is another device on the
LPC bus, and another driver accessing that device. Personally I'd rather have
request_muxed_region() added to the f71805f driver.

Guenter

> This patch adds a check to ensure that the CPU MMIO region is registered
> prior to accessing the PCI IO ports.
> 
> [1] https://lore.kernel.org/linux-pci/56F209A9.4040304@huawei.com
> [2] https://lore.kernel.org/linux-arm-kernel/e6995b4a-184a-d8d4-f4d4-9ce75d8f47c0@huawei.com/
> 
> This patch includes some other tidy-up.
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>  lib/logic_pio.c | 103 +++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 75 insertions(+), 28 deletions(-)
> 
> diff --git a/lib/logic_pio.c b/lib/logic_pio.c
> index 431cd8d99236..3d8d986e9dcb 100644
> --- a/lib/logic_pio.c
> +++ b/lib/logic_pio.c
> @@ -193,95 +193,135 @@ unsigned long logic_pio_trans_cpuaddr(resource_size_t addr)
>  
>  #if defined(PCI_IOBASE)
>  #if defined(CONFIG_INDIRECT_PIO)
> +#define INVALID_RANGE(range)						\
> +	(!(range) || ((range)->flags == LOGIC_PIO_INDIRECT && !(range)->ops))
> +
>  #define BUILD_LOGIC_IO(bw, type)					\
>  type logic_in##bw(unsigned long addr)					\
>  {									\
>  	type ret = (type)~0;						\
> +	struct logic_pio_hwaddr *range = find_io_range(addr);		\
> +									\
> +	if (INVALID_RANGE(range)) {					\
> +		WARN_ON_ONCE(1);					\
> +		return ret;						\
> +	}								\
>  									\
>  	if (addr < MMIO_UPPER_LIMIT) {					\
>  		ret = read##bw(PCI_IOBASE + addr);			\
>  	} else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) { \
> -		struct logic_pio_hwaddr *entry = find_io_range(addr);	\
>  		size_t sz = sizeof(type);				\
> +		void *hostdata = range->hostdata;			\
>  									\
> -		if (entry && entry->ops)				\
> -			ret = entry->ops->in(entry->hostdata, addr, sz);\
> -		else							\
> -			WARN_ON_ONCE(1);				\
> +		if (range->ops->in)					\
> +			ret = range->ops->in(hostdata, addr, sz);	\
>  	}								\
>  	return ret;							\
>  }									\
>  									\
> -void logic_out##bw(type value, unsigned long addr)			\
> +void logic_out##bw(type val, unsigned long addr)			\
>  {									\
> +	struct logic_pio_hwaddr *range = find_io_range(addr);		\
> +									\
> +	if (INVALID_RANGE(range)) {					\
> +		WARN_ON_ONCE(1);					\
> +		return;							\
> +	}								\
> +									\
>  	if (addr < MMIO_UPPER_LIMIT) {					\
> -		write##bw(value, PCI_IOBASE + addr);			\
> +		write##bw(val, PCI_IOBASE + addr);			\
>  	} else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {	\
> -		struct logic_pio_hwaddr *entry = find_io_range(addr);	\
>  		size_t sz = sizeof(type);				\
> +		void *hostdata = range->hostdata;			\
>  									\
> -		if (entry && entry->ops)				\
> -			entry->ops->out(entry->hostdata,		\
> -					addr, value, sz);		\
> -		else							\
> -			WARN_ON_ONCE(1);				\
> +		if (range->ops->out)					\
> +			range->ops->out(hostdata, addr, val, sz);	\
>  	}								\
>  }									\
>  									\
>  void logic_ins##bw(unsigned long addr, void *buf, unsigned int cnt)	\
>  {									\
> +	struct logic_pio_hwaddr *range = find_io_range(addr);		\
> +									\
> +	if (INVALID_RANGE(range)) {					\
> +		WARN_ON_ONCE(1);					\
> +		return;							\
> +	}								\
> +									\
>  	if (addr < MMIO_UPPER_LIMIT) {					\
>  		reads##bw(PCI_IOBASE + addr, buf, cnt);			\
>  	} else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {	\
> -		struct logic_pio_hwaddr *entry = find_io_range(addr);	\
>  		size_t sz = sizeof(type);				\
> +		void *hostdata = range->hostdata;			\
>  									\
> -		if (entry && entry->ops)				\
> -			entry->ops->ins(entry->hostdata,		\
> -				addr, buf, sz, cnt);			\
> -		else							\
> -			WARN_ON_ONCE(1);				\
> +		if (range->ops->ins)					\
> +			range->ops->ins(hostdata, addr, buf, sz, cnt);	\
>  	}								\
> -									\
>  }									\
>  									\
>  void logic_outs##bw(unsigned long addr, const void *buf,		\
>  		    unsigned int cnt)					\
>  {									\
> +	struct logic_pio_hwaddr *range = find_io_range(addr);		\
> +									\
> +	if (INVALID_RANGE(range)) {					\
> +		WARN_ON_ONCE(1);					\
> +		return;							\
> +	}								\
> +									\
>  	if (addr < MMIO_UPPER_LIMIT) {					\
>  		writes##bw(PCI_IOBASE + addr, buf, cnt);		\
>  	} else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {	\
> -		struct logic_pio_hwaddr *entry = find_io_range(addr);	\
>  		size_t sz = sizeof(type);				\
> +		void *hostdata = range->hostdata;			\
>  									\
> -		if (entry && entry->ops)				\
> -			entry->ops->outs(entry->hostdata,		\
> -				addr, buf, sz, cnt);			\
> -		else							\
> -			WARN_ON_ONCE(1);				\
> +		if (range->ops->outs)					\
> +			range->ops->outs(hostdata, addr, buf, sz, cnt);	\
>  	}								\
>  }
>  
>  #else /* CONFIG_INDIRECT_PIO */
>  
> +#define INVALID_RANGE(range) (!(range))
> +
>  #define BUILD_LOGIC_IO(bw, type)					\
>  type logic_in##bw(unsigned long addr)					\
>  {									\
>  	type ret = (type)~0;						\
> +	struct logic_pio_hwaddr *range = find_io_range(addr);		\
> +									\
> +	if (INVALID_RANGE(range)) {					\
> +		WARN_ON_ONCE(1);					\
> +		return ret;						\
> +	}								\
>  									\
>  	if (addr < MMIO_UPPER_LIMIT)					\
>  		ret = read##bw(PCI_IOBASE + addr);			\
>  	return ret;							\
>  }									\
>  									\
> -void logic_out##bw(type value, unsigned long addr)			\
> +void logic_out##bw(type val, unsigned long addr)			\
>  {									\
> +	struct logic_pio_hwaddr *range = find_io_range(addr);		\
> +									\
> +	if (INVALID_RANGE(range)) {					\
> +		WARN_ON_ONCE(1);					\
> +		return;							\
> +	}								\
> +									\
>  	if (addr < MMIO_UPPER_LIMIT)					\
> -		write##bw(value, PCI_IOBASE + addr);			\
> +		write##bw(val, PCI_IOBASE + addr);			\
>  }									\
>  									\
>  void logic_ins##bw(unsigned long addr, void *buf, unsigned int cnt)	\
>  {									\
> +	struct logic_pio_hwaddr *range = find_io_range(addr);		\
> +									\
> +	if (INVALID_RANGE(range)) {					\
> +		WARN_ON_ONCE(1);					\
> +		return;							\
> +	}								\
> +									\
>  	if (addr < MMIO_UPPER_LIMIT)					\
>  		reads##bw(PCI_IOBASE + addr, buf, cnt);			\
>  }									\
> @@ -289,6 +329,13 @@ void logic_ins##bw(unsigned long addr, void *buf, unsigned int cnt)	\
>  void logic_outs##bw(unsigned long addr, const void *buf,		\
>  		    unsigned int cnt)					\
>  {									\
> +	struct logic_pio_hwaddr *range = find_io_range(addr);		\
> +									\
> +	if (INVALID_RANGE(range)) {					\
> +		WARN_ON_ONCE(1);					\
> +		return;							\
> +	}								\
> +									\
>  	if (addr < MMIO_UPPER_LIMIT)					\
>  		writes##bw(PCI_IOBASE + addr, buf, cnt);		\
>  }
> -- 
> 2.17.1
> 

Re: [PATCH v3 3/4] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions

[Guenter Roeck]

On Fri, Apr 05, 2019 at 12:00:01AM +0800, John Garry wrote:
> Currently when accessing logical indirect PIO addresses in
> logic_{in, out}{,s}, we first ensure that the region is registered.
> 
> However, no such check exists for CPU MMIO regions. The CPU MMIO regions
> would be registered by the PCI host - when PCI_IOBASE is defined - in
> pci_register_io_range().
> 
> We have seen scenarios when systems which don't have a PCI host or, they
> do, and the PCI host probe fails, that certain devices attempts to still
> attempt to access PCI IO ports; examples are in [1] and [2].
> 
> And even though we should protect against this by ensuring the driver
> calls request_{muxed_}region(), some don't do this:
> 
> root@(none)$ insmod hwmon/f71805f.ko
>  Unable to handle kernel paging request at virtual address ffff7dfffee0002e
>  Mem abort info:
>    ESR = 0x96000046
>    Exception class = DABT (current EL), IL = 32 bits
>    SET = 0, FnV = 0
>    EA = 0, S1PTW = 0
>  Data abort info:
>    ISV = 0, ISS = 0x00000046
>    CM = 0, WnR = 1
>  swapper pgtable: 4k pages, 48-bit VAs, pgdp = (____ptrval____)
>  [ffff7dfffee0002e] pgd=000000000141c003, pud=000000000141d003, pmd=0000000000000000
>  Internal error: Oops: 96000046 [#1] PREEMPT SMP
>  Modules linked in: f71805f(+)
>  CPU: 20 PID: 2736 Comm: insmod Not tainted 5.1.0-rc1-00003-g6f1bfec2a620-dirty #99
>  Hardware name: Huawei Taishan 2280 /D05, BIOS Hisilicon D05 IT21 Nemo 2.0 RC0 04/18/2018
>  pstate: 80000005 (Nzcv daif -PAN -UAO)
>  pc : logic_outb+0x54/0xb8
>  lr : f71805f_find+0x2c/0x1b8 [f71805f]
>  sp : ffff000025fbba90
>  x29: ffff000025fbba90 x28: ffff000008b944d0
>  x27: ffff000025fbbdf0 x26: 0000000000000100
>  x25: ffff801f8c270580 x24: ffff000011420000
>  x23: ffff000025fbbb3e x22: ffff000025fbbb40
>  x21: ffff000008b991b8 x20: 0000000000000087
>  x19: 000000000000002e x18: ffffffffffffffff
>  x17: 0000000000000000 x16: 0000000000000000
>  x15: ffff00001127d6c8 x14: 0000000000000000
>  x13: 0000000000000000 x12: 0000000000000000
>  x11: 0000000000010820 x10: 0000841fdac40000
>  x9 : 0000000000000001 x8 : 0000000040000000
>  x7 : 0000000000210d00 x6 : 0000000000000000
>  x5 : ffff801fb6a46040 x4 : ffff841febeaeda0
>  x3 : 0000000000ffbffe x2 : ffff000025fbbb40
>  x1 : ffff7dfffee0002e x0 : ffff7dfffee00000
>  Process insmod (pid: 2736, stack limit = 0x(____ptrval____))
>  Call trace:
>   logic_outb+0x54/0xb8
>   f71805f_find+0x2c/0x1b8 [f71805f]
>   f71805f_init+0x38/0xe48 [f71805f]
>   do_one_initcall+0x5c/0x198
>   do_init_module+0x54/0x1b0
>   load_module+0x1dc4/0x2158
>   __se_sys_init_module+0x14c/0x1e8
>   __arm64_sys_init_module+0x18/0x20
>   el0_svc_common+0x5c/0x100
>   el0_svc_handler+0x2c/0x80
>   el0_svc+0x8/0xc
>  Code: d2bfdc00 f2cfbfe0 f2ffffe0 8b000021 (39000034)
>  ---[ end trace 10ea80bde051bbfc ]---
> root@(none)$
> 
> Note that the f71805f driver does not call request_{muxed_}region(), as it
> should.
> 
... which is the real problem, one that is not solved by this patch. This may
result in parallel and descructive accesses if there is another device on the
LPC bus, and another driver accessing that device. Personally I'd rather have
request_muxed_region() added to the f71805f driver.

Guenter

> This patch adds a check to ensure that the CPU MMIO region is registered
> prior to accessing the PCI IO ports.
> 
> [1] https://lore.kernel.org/linux-pci/56F209A9.4040304@huawei.com
> [2] https://lore.kernel.org/linux-arm-kernel/e6995b4a-184a-d8d4-f4d4-9ce75d8f47c0@huawei.com/
> 
> This patch includes some other tidy-up.
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>  lib/logic_pio.c | 103 +++++++++++++++++++++++++++++++++++-------------
>  1 file changed, 75 insertions(+), 28 deletions(-)
> 
> diff --git a/lib/logic_pio.c b/lib/logic_pio.c
> index 431cd8d99236..3d8d986e9dcb 100644
> --- a/lib/logic_pio.c
> +++ b/lib/logic_pio.c
> @@ -193,95 +193,135 @@ unsigned long logic_pio_trans_cpuaddr(resource_size_t addr)
>  
>  #if defined(PCI_IOBASE)
>  #if defined(CONFIG_INDIRECT_PIO)
> +#define INVALID_RANGE(range)						\
> +	(!(range) || ((range)->flags == LOGIC_PIO_INDIRECT && !(range)->ops))
> +
>  #define BUILD_LOGIC_IO(bw, type)					\
>  type logic_in##bw(unsigned long addr)					\
>  {									\
>  	type ret = (type)~0;						\
> +	struct logic_pio_hwaddr *range = find_io_range(addr);		\
> +									\
> +	if (INVALID_RANGE(range)) {					\
> +		WARN_ON_ONCE(1);					\
> +		return ret;						\
> +	}								\
>  									\
>  	if (addr < MMIO_UPPER_LIMIT) {					\
>  		ret = read##bw(PCI_IOBASE + addr);			\
>  	} else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) { \
> -		struct logic_pio_hwaddr *entry = find_io_range(addr);	\
>  		size_t sz = sizeof(type);				\
> +		void *hostdata = range->hostdata;			\
>  									\
> -		if (entry && entry->ops)				\
> -			ret = entry->ops->in(entry->hostdata, addr, sz);\
> -		else							\
> -			WARN_ON_ONCE(1);				\
> +		if (range->ops->in)					\
> +			ret = range->ops->in(hostdata, addr, sz);	\
>  	}								\
>  	return ret;							\
>  }									\
>  									\
> -void logic_out##bw(type value, unsigned long addr)			\
> +void logic_out##bw(type val, unsigned long addr)			\
>  {									\
> +	struct logic_pio_hwaddr *range = find_io_range(addr);		\
> +									\
> +	if (INVALID_RANGE(range)) {					\
> +		WARN_ON_ONCE(1);					\
> +		return;							\
> +	}								\
> +									\
>  	if (addr < MMIO_UPPER_LIMIT) {					\
> -		write##bw(value, PCI_IOBASE + addr);			\
> +		write##bw(val, PCI_IOBASE + addr);			\
>  	} else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {	\
> -		struct logic_pio_hwaddr *entry = find_io_range(addr);	\
>  		size_t sz = sizeof(type);				\
> +		void *hostdata = range->hostdata;			\
>  									\
> -		if (entry && entry->ops)				\
> -			entry->ops->out(entry->hostdata,		\
> -					addr, value, sz);		\
> -		else							\
> -			WARN_ON_ONCE(1);				\
> +		if (range->ops->out)					\
> +			range->ops->out(hostdata, addr, val, sz);	\
>  	}								\
>  }									\
>  									\
>  void logic_ins##bw(unsigned long addr, void *buf, unsigned int cnt)	\
>  {									\
> +	struct logic_pio_hwaddr *range = find_io_range(addr);		\
> +									\
> +	if (INVALID_RANGE(range)) {					\
> +		WARN_ON_ONCE(1);					\
> +		return;							\
> +	}								\
> +									\
>  	if (addr < MMIO_UPPER_LIMIT) {					\
>  		reads##bw(PCI_IOBASE + addr, buf, cnt);			\
>  	} else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {	\
> -		struct logic_pio_hwaddr *entry = find_io_range(addr);	\
>  		size_t sz = sizeof(type);				\
> +		void *hostdata = range->hostdata;			\
>  									\
> -		if (entry && entry->ops)				\
> -			entry->ops->ins(entry->hostdata,		\
> -				addr, buf, sz, cnt);			\
> -		else							\
> -			WARN_ON_ONCE(1);				\
> +		if (range->ops->ins)					\
> +			range->ops->ins(hostdata, addr, buf, sz, cnt);	\
>  	}								\
> -									\
>  }									\
>  									\
>  void logic_outs##bw(unsigned long addr, const void *buf,		\
>  		    unsigned int cnt)					\
>  {									\
> +	struct logic_pio_hwaddr *range = find_io_range(addr);		\
> +									\
> +	if (INVALID_RANGE(range)) {					\
> +		WARN_ON_ONCE(1);					\
> +		return;							\
> +	}								\
> +									\
>  	if (addr < MMIO_UPPER_LIMIT) {					\
>  		writes##bw(PCI_IOBASE + addr, buf, cnt);		\
>  	} else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {	\
> -		struct logic_pio_hwaddr *entry = find_io_range(addr);	\
>  		size_t sz = sizeof(type);				\
> +		void *hostdata = range->hostdata;			\
>  									\
> -		if (entry && entry->ops)				\
> -			entry->ops->outs(entry->hostdata,		\
> -				addr, buf, sz, cnt);			\
> -		else							\
> -			WARN_ON_ONCE(1);				\
> +		if (range->ops->outs)					\
> +			range->ops->outs(hostdata, addr, buf, sz, cnt);	\
>  	}								\
>  }
>  
>  #else /* CONFIG_INDIRECT_PIO */
>  
> +#define INVALID_RANGE(range) (!(range))
> +
>  #define BUILD_LOGIC_IO(bw, type)					\
>  type logic_in##bw(unsigned long addr)					\
>  {									\
>  	type ret = (type)~0;						\
> +	struct logic_pio_hwaddr *range = find_io_range(addr);		\
> +									\
> +	if (INVALID_RANGE(range)) {					\
> +		WARN_ON_ONCE(1);					\
> +		return ret;						\
> +	}								\
>  									\
>  	if (addr < MMIO_UPPER_LIMIT)					\
>  		ret = read##bw(PCI_IOBASE + addr);			\
>  	return ret;							\
>  }									\
>  									\
> -void logic_out##bw(type value, unsigned long addr)			\
> +void logic_out##bw(type val, unsigned long addr)			\
>  {									\
> +	struct logic_pio_hwaddr *range = find_io_range(addr);		\
> +									\
> +	if (INVALID_RANGE(range)) {					\
> +		WARN_ON_ONCE(1);					\
> +		return;							\
> +	}								\
> +									\
>  	if (addr < MMIO_UPPER_LIMIT)					\
> -		write##bw(value, PCI_IOBASE + addr);			\
> +		write##bw(val, PCI_IOBASE + addr);			\
>  }									\
>  									\
>  void logic_ins##bw(unsigned long addr, void *buf, unsigned int cnt)	\
>  {									\
> +	struct logic_pio_hwaddr *range = find_io_range(addr);		\
> +									\
> +	if (INVALID_RANGE(range)) {					\
> +		WARN_ON_ONCE(1);					\
> +		return;							\
> +	}								\
> +									\
>  	if (addr < MMIO_UPPER_LIMIT)					\
>  		reads##bw(PCI_IOBASE + addr, buf, cnt);			\
>  }									\
> @@ -289,6 +329,13 @@ void logic_ins##bw(unsigned long addr, void *buf, unsigned int cnt)	\
>  void logic_outs##bw(unsigned long addr, const void *buf,		\
>  		    unsigned int cnt)					\
>  {									\
> +	struct logic_pio_hwaddr *range = find_io_range(addr);		\
> +									\
> +	if (INVALID_RANGE(range)) {					\
> +		WARN_ON_ONCE(1);					\
> +		return;							\
> +	}								\
> +									\
>  	if (addr < MMIO_UPPER_LIMIT)					\
>  		writes##bw(PCI_IOBASE + addr, buf, cnt);		\
>  }
> -- 
> 2.17.1
> 

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/4] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions

[John Garry]

On 04/04/2019 17:41, Guenter Roeck wrote:
> On Fri, Apr 05, 2019 at 12:00:01AM +0800, John Garry wrote:
>> Currently when accessing logical indirect PIO addresses in
>> logic_{in, out}{,s}, we first ensure that the region is registered.
>>
>> However, no such check exists for CPU MMIO regions. The CPU MMIO regions
>> would be registered by the PCI host - when PCI_IOBASE is defined - in
>> pci_register_io_range().
>>
>> We have seen scenarios when systems which don't have a PCI host or, they
>> do, and the PCI host probe fails, that certain devices attempts to still
>> attempt to access PCI IO ports; examples are in [1] and [2].
>>
>> And even though we should protect against this by ensuring the driver
>> calls request_{muxed_}region(), some don't do this:
>>
>> root@(none)$ insmod hwmon/f71805f.ko
>>  Unable to handle kernel paging request at virtual address ffff7dfffee0002e
>>  Mem abort info:
>>    ESR = 0x96000046
>>    Exception class = DABT (current EL), IL = 32 bits
>>    SET = 0, FnV = 0
>>    EA = 0, S1PTW = 0
>>  Data abort info:
>>    ISV = 0, ISS = 0x00000046
>>    CM = 0, WnR = 1
>>  swapper pgtable: 4k pages, 48-bit VAs, pgdp = (____ptrval____)
>>  [ffff7dfffee0002e] pgd=000000000141c003, pud=000000000141d003, pmd=0000000000000000
>>  Internal error: Oops: 96000046 [#1] PREEMPT SMP
>>  Modules linked in: f71805f(+)
>>  CPU: 20 PID: 2736 Comm: insmod Not tainted 5.1.0-rc1-00003-g6f1bfec2a620-dirty #99
>>  Hardware name: Huawei Taishan 2280 /D05, BIOS Hisilicon D05 IT21 Nemo 2.0 RC0 04/18/2018
>>  pstate: 80000005 (Nzcv daif -PAN -UAO)
>>  pc : logic_outb+0x54/0xb8
>>  lr : f71805f_find+0x2c/0x1b8 [f71805f]
>>  sp : ffff000025fbba90
>>  x29: ffff000025fbba90 x28: ffff000008b944d0
>>  x27: ffff000025fbbdf0 x26: 0000000000000100
>>  x25: ffff801f8c270580 x24: ffff000011420000
>>  x23: ffff000025fbbb3e x22: ffff000025fbbb40
>>  x21: ffff000008b991b8 x20: 0000000000000087
>>  x19: 000000000000002e x18: ffffffffffffffff
>>  x17: 0000000000000000 x16: 0000000000000000
>>  x15: ffff00001127d6c8 x14: 0000000000000000
>>  x13: 0000000000000000 x12: 0000000000000000
>>  x11: 0000000000010820 x10: 0000841fdac40000
>>  x9 : 0000000000000001 x8 : 0000000040000000
>>  x7 : 0000000000210d00 x6 : 0000000000000000
>>  x5 : ffff801fb6a46040 x4 : ffff841febeaeda0
>>  x3 : 0000000000ffbffe x2 : ffff000025fbbb40
>>  x1 : ffff7dfffee0002e x0 : ffff7dfffee00000
>>  Process insmod (pid: 2736, stack limit = 0x(____ptrval____))
>>  Call trace:
>>   logic_outb+0x54/0xb8
>>   f71805f_find+0x2c/0x1b8 [f71805f]
>>   f71805f_init+0x38/0xe48 [f71805f]
>>   do_one_initcall+0x5c/0x198
>>   do_init_module+0x54/0x1b0
>>   load_module+0x1dc4/0x2158
>>   __se_sys_init_module+0x14c/0x1e8
>>   __arm64_sys_init_module+0x18/0x20
>>   el0_svc_common+0x5c/0x100
>>   el0_svc_handler+0x2c/0x80
>>   el0_svc+0x8/0xc
>>  Code: d2bfdc00 f2cfbfe0 f2ffffe0 8b000021 (39000034)
>>  ---[ end trace 10ea80bde051bbfc ]---
>> root@(none)$
>>
>> Note that the f71805f driver does not call request_{muxed_}region(), as it
>> should.
>>

Hi Guenter,

> ... which is the real problem, one that is not solved by this patch. This may
> result in parallel and descructive accesses if there is another device on the
> LPC bus, and another driver accessing that device. Personally I'd rather have
> request_muxed_region() added to the f71805f driver.

Right, we should and will still fix f71805f. If you recall, I did have 
the f71805f fix in the v1 series, but you committed that it was 
orthogonal, so I decided to take it out of this work for now.

And even if we fix up f71805f and other known drivers which don't call 
request_muxed_region(), we still need to police against these rogue 
accesses, which is what this patch attempts to do.

Thanks,
John

>
> Guenter
>
>> This patch adds a check to ensure that the CPU MMIO region is registered
>> prior to accessing the PCI IO ports.
>>
>> [1] https://lore.kernel.org/linux-pci/56F209A9.4040304@huawei.com
>> [2] https://lore.kernel.org/linux-arm-kernel/e6995b4a-184a-d8d4-f4d4-9ce75d8f47c0@huawei.com/
>>
>> This patch includes some other tidy-up.
>>
>> Signed-off-by: John Garry <john.garry@huawei.com>
>> ---
>>  lib/logic_pio.c | 103 +++++++++++++++++++++++++++++++++++-------------
>>  1 file changed, 75 insertions(+), 28 deletions(-)
>>
>> diff --git a/lib/logic_pio.c b/lib/logic_pio.c
>> index 431cd8d99236..3d8d986e9dcb 100644
>> --- a/lib/logic_pio.c
>> +++ b/lib/logic_pio.c
>> @@ -193,95 +193,135 @@ unsigned long logic_pio_trans_cpuaddr(resource_size_t addr)
>>
>>  #if defined(PCI_IOBASE)
>>  #if defined(CONFIG_INDIRECT_PIO)
>> +#define INVALID_RANGE(range)						\
>> +	(!(range) || ((range)->flags == LOGIC_PIO_INDIRECT && !(range)->ops))
>> +
>>  #define BUILD_LOGIC_IO(bw, type)					\
>>  type logic_in##bw(unsigned long addr)					\
>>  {									\
>>  	type ret = (type)~0;						\
>> +	struct logic_pio_hwaddr *range = find_io_range(addr);		\
>> +									\
>> +	if (INVALID_RANGE(range)) {					\
>> +		WARN_ON_ONCE(1);					\
>> +		return ret;						\
>> +	}								\
>>  									\
>>  	if (addr < MMIO_UPPER_LIMIT) {					\
>>  		ret = read##bw(PCI_IOBASE + addr);			\
>>  	} else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) { \
>> -		struct logic_pio_hwaddr *entry = find_io_range(addr);	\
>>  		size_t sz = sizeof(type);				\
>> +		void *hostdata = range->hostdata;			\
>>  									\
>> -		if (entry && entry->ops)				\
>> -			ret = entry->ops->in(entry->hostdata, addr, sz);\
>> -		else							\
>> -			WARN_ON_ONCE(1);				\
>> +		if (range->ops->in)					\
>> +			ret = range->ops->in(hostdata, addr, sz);	\
>>  	}								\
>>  	return ret;							\
>>  }									\
>>  									\
>> -void logic_out##bw(type value, unsigned long addr)			\
>> +void logic_out##bw(type val, unsigned long addr)			\
>>  {									\
>> +	struct logic_pio_hwaddr *range = find_io_range(addr);		\
>> +									\
>> +	if (INVALID_RANGE(range)) {					\
>> +		WARN_ON_ONCE(1);					\
>> +		return;							\
>> +	}								\
>> +									\
>>  	if (addr < MMIO_UPPER_LIMIT) {					\
>> -		write##bw(value, PCI_IOBASE + addr);			\
>> +		write##bw(val, PCI_IOBASE + addr);			\
>>  	} else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {	\
>> -		struct logic_pio_hwaddr *entry = find_io_range(addr);	\
>>  		size_t sz = sizeof(type);				\
>> +		void *hostdata = range->hostdata;			\
>>  									\
>> -		if (entry && entry->ops)				\
>> -			entry->ops->out(entry->hostdata,		\
>> -					addr, value, sz);		\
>> -		else							\
>> -			WARN_ON_ONCE(1);				\
>> +		if (range->ops->out)					\
>> +			range->ops->out(hostdata, addr, val, sz);	\
>>  	}								\
>>  }									\
>>  									\
>>  void logic_ins##bw(unsigned long addr, void *buf, unsigned int cnt)	\
>>  {									\
>> +	struct logic_pio_hwaddr *range = find_io_range(addr);		\
>> +									\
>> +	if (INVALID_RANGE(range)) {					\
>> +		WARN_ON_ONCE(1);					\
>> +		return;							\
>> +	}								\
>> +									\
>>  	if (addr < MMIO_UPPER_LIMIT) {					\
>>  		reads##bw(PCI_IOBASE + addr, buf, cnt);			\
>>  	} else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {	\
>> -		struct logic_pio_hwaddr *entry = find_io_range(addr);	\
>>  		size_t sz = sizeof(type);				\
>> +		void *hostdata = range->hostdata;			\
>>  									\
>> -		if (entry && entry->ops)				\
>> -			entry->ops->ins(entry->hostdata,		\
>> -				addr, buf, sz, cnt);			\
>> -		else							\
>> -			WARN_ON_ONCE(1);				\
>> +		if (range->ops->ins)					\
>> +			range->ops->ins(hostdata, addr, buf, sz, cnt);	\
>>  	}								\
>> -									\
>>  }									\
>>  									\
>>  void logic_outs##bw(unsigned long addr, const void *buf,		\
>>  		    unsigned int cnt)					\
>>  {									\
>> +	struct logic_pio_hwaddr *range = find_io_range(addr);		\
>> +									\
>> +	if (INVALID_RANGE(range)) {					\
>> +		WARN_ON_ONCE(1);					\
>> +		return;							\
>> +	}								\
>> +									\
>>  	if (addr < MMIO_UPPER_LIMIT) {					\
>>  		writes##bw(PCI_IOBASE + addr, buf, cnt);		\
>>  	} else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {	\
>> -		struct logic_pio_hwaddr *entry = find_io_range(addr);	\
>>  		size_t sz = sizeof(type);				\
>> +		void *hostdata = range->hostdata;			\
>>  									\
>> -		if (entry && entry->ops)				\
>> -			entry->ops->outs(entry->hostdata,		\
>> -				addr, buf, sz, cnt);			\
>> -		else							\
>> -			WARN_ON_ONCE(1);				\
>> +		if (range->ops->outs)					\
>> +			range->ops->outs(hostdata, addr, buf, sz, cnt);	\
>>  	}								\
>>  }
>>
>>  #else /* CONFIG_INDIRECT_PIO */
>>
>> +#define INVALID_RANGE(range) (!(range))
>> +
>>  #define BUILD_LOGIC_IO(bw, type)					\
>>  type logic_in##bw(unsigned long addr)					\
>>  {									\
>>  	type ret = (type)~0;						\
>> +	struct logic_pio_hwaddr *range = find_io_range(addr);		\
>> +									\
>> +	if (INVALID_RANGE(range)) {					\
>> +		WARN_ON_ONCE(1);					\
>> +		return ret;						\
>> +	}								\
>>  									\
>>  	if (addr < MMIO_UPPER_LIMIT)					\
>>  		ret = read##bw(PCI_IOBASE + addr);			\
>>  	return ret;							\
>>  }									\
>>  									\
>> -void logic_out##bw(type value, unsigned long addr)			\
>> +void logic_out##bw(type val, unsigned long addr)			\
>>  {									\
>> +	struct logic_pio_hwaddr *range = find_io_range(addr);		\
>> +									\
>> +	if (INVALID_RANGE(range)) {					\
>> +		WARN_ON_ONCE(1);					\
>> +		return;							\
>> +	}								\
>> +									\
>>  	if (addr < MMIO_UPPER_LIMIT)					\
>> -		write##bw(value, PCI_IOBASE + addr);			\
>> +		write##bw(val, PCI_IOBASE + addr);			\
>>  }									\
>>  									\
>>  void logic_ins##bw(unsigned long addr, void *buf, unsigned int cnt)	\
>>  {									\
>> +	struct logic_pio_hwaddr *range = find_io_range(addr);		\
>> +									\
>> +	if (INVALID_RANGE(range)) {					\
>> +		WARN_ON_ONCE(1);					\
>> +		return;							\
>> +	}								\
>> +									\
>>  	if (addr < MMIO_UPPER_LIMIT)					\
>>  		reads##bw(PCI_IOBASE + addr, buf, cnt);			\
>>  }									\
>> @@ -289,6 +329,13 @@ void logic_ins##bw(unsigned long addr, void *buf, unsigned int cnt)	\
>>  void logic_outs##bw(unsigned long addr, const void *buf,		\
>>  		    unsigned int cnt)					\
>>  {									\
>> +	struct logic_pio_hwaddr *range = find_io_range(addr);		\
>> +									\
>> +	if (INVALID_RANGE(range)) {					\
>> +		WARN_ON_ONCE(1);					\
>> +		return;							\
>> +	}								\
>> +									\
>>  	if (addr < MMIO_UPPER_LIMIT)					\
>>  		writes##bw(PCI_IOBASE + addr, buf, cnt);		\
>>  }
>> --
>> 2.17.1
>>
>
> .
>

Re: [PATCH v3 3/4] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions

[John Garry]

On 04/04/2019 17:41, Guenter Roeck wrote:
> On Fri, Apr 05, 2019 at 12:00:01AM +0800, John Garry wrote:
>> Currently when accessing logical indirect PIO addresses in
>> logic_{in, out}{,s}, we first ensure that the region is registered.
>>
>> However, no such check exists for CPU MMIO regions. The CPU MMIO regions
>> would be registered by the PCI host - when PCI_IOBASE is defined - in
>> pci_register_io_range().
>>
>> We have seen scenarios when systems which don't have a PCI host or, they
>> do, and the PCI host probe fails, that certain devices attempts to still
>> attempt to access PCI IO ports; examples are in [1] and [2].
>>
>> And even though we should protect against this by ensuring the driver
>> calls request_{muxed_}region(), some don't do this:
>>
>> root@(none)$ insmod hwmon/f71805f.ko
>>  Unable to handle kernel paging request at virtual address ffff7dfffee0002e
>>  Mem abort info:
>>    ESR = 0x96000046
>>    Exception class = DABT (current EL), IL = 32 bits
>>    SET = 0, FnV = 0
>>    EA = 0, S1PTW = 0
>>  Data abort info:
>>    ISV = 0, ISS = 0x00000046
>>    CM = 0, WnR = 1
>>  swapper pgtable: 4k pages, 48-bit VAs, pgdp = (____ptrval____)
>>  [ffff7dfffee0002e] pgd=000000000141c003, pud=000000000141d003, pmd=0000000000000000
>>  Internal error: Oops: 96000046 [#1] PREEMPT SMP
>>  Modules linked in: f71805f(+)
>>  CPU: 20 PID: 2736 Comm: insmod Not tainted 5.1.0-rc1-00003-g6f1bfec2a620-dirty #99
>>  Hardware name: Huawei Taishan 2280 /D05, BIOS Hisilicon D05 IT21 Nemo 2.0 RC0 04/18/2018
>>  pstate: 80000005 (Nzcv daif -PAN -UAO)
>>  pc : logic_outb+0x54/0xb8
>>  lr : f71805f_find+0x2c/0x1b8 [f71805f]
>>  sp : ffff000025fbba90
>>  x29: ffff000025fbba90 x28: ffff000008b944d0
>>  x27: ffff000025fbbdf0 x26: 0000000000000100
>>  x25: ffff801f8c270580 x24: ffff000011420000
>>  x23: ffff000025fbbb3e x22: ffff000025fbbb40
>>  x21: ffff000008b991b8 x20: 0000000000000087
>>  x19: 000000000000002e x18: ffffffffffffffff
>>  x17: 0000000000000000 x16: 0000000000000000
>>  x15: ffff00001127d6c8 x14: 0000000000000000
>>  x13: 0000000000000000 x12: 0000000000000000
>>  x11: 0000000000010820 x10: 0000841fdac40000
>>  x9 : 0000000000000001 x8 : 0000000040000000
>>  x7 : 0000000000210d00 x6 : 0000000000000000
>>  x5 : ffff801fb6a46040 x4 : ffff841febeaeda0
>>  x3 : 0000000000ffbffe x2 : ffff000025fbbb40
>>  x1 : ffff7dfffee0002e x0 : ffff7dfffee00000
>>  Process insmod (pid: 2736, stack limit = 0x(____ptrval____))
>>  Call trace:
>>   logic_outb+0x54/0xb8
>>   f71805f_find+0x2c/0x1b8 [f71805f]
>>   f71805f_init+0x38/0xe48 [f71805f]
>>   do_one_initcall+0x5c/0x198
>>   do_init_module+0x54/0x1b0
>>   load_module+0x1dc4/0x2158
>>   __se_sys_init_module+0x14c/0x1e8
>>   __arm64_sys_init_module+0x18/0x20
>>   el0_svc_common+0x5c/0x100
>>   el0_svc_handler+0x2c/0x80
>>   el0_svc+0x8/0xc
>>  Code: d2bfdc00 f2cfbfe0 f2ffffe0 8b000021 (39000034)
>>  ---[ end trace 10ea80bde051bbfc ]---
>> root@(none)$
>>
>> Note that the f71805f driver does not call request_{muxed_}region(), as it
>> should.
>>

Hi Guenter,

> ... which is the real problem, one that is not solved by this patch. This may
> result in parallel and descructive accesses if there is another device on the
> LPC bus, and another driver accessing that device. Personally I'd rather have
> request_muxed_region() added to the f71805f driver.

Right, we should and will still fix f71805f. If you recall, I did have 
the f71805f fix in the v1 series, but you committed that it was 
orthogonal, so I decided to take it out of this work for now.

And even if we fix up f71805f and other known drivers which don't call 
request_muxed_region(), we still need to police against these rogue 
accesses, which is what this patch attempts to do.

Thanks,
John

>
> Guenter
>
>> This patch adds a check to ensure that the CPU MMIO region is registered
>> prior to accessing the PCI IO ports.
>>
>> [1] https://lore.kernel.org/linux-pci/56F209A9.4040304@huawei.com
>> [2] https://lore.kernel.org/linux-arm-kernel/e6995b4a-184a-d8d4-f4d4-9ce75d8f47c0@huawei.com/
>>
>> This patch includes some other tidy-up.
>>
>> Signed-off-by: John Garry <john.garry@huawei.com>
>> ---
>>  lib/logic_pio.c | 103 +++++++++++++++++++++++++++++++++++-------------
>>  1 file changed, 75 insertions(+), 28 deletions(-)
>>
>> diff --git a/lib/logic_pio.c b/lib/logic_pio.c
>> index 431cd8d99236..3d8d986e9dcb 100644
>> --- a/lib/logic_pio.c
>> +++ b/lib/logic_pio.c
>> @@ -193,95 +193,135 @@ unsigned long logic_pio_trans_cpuaddr(resource_size_t addr)
>>
>>  #if defined(PCI_IOBASE)
>>  #if defined(CONFIG_INDIRECT_PIO)
>> +#define INVALID_RANGE(range)						\
>> +	(!(range) || ((range)->flags == LOGIC_PIO_INDIRECT && !(range)->ops))
>> +
>>  #define BUILD_LOGIC_IO(bw, type)					\
>>  type logic_in##bw(unsigned long addr)					\
>>  {									\
>>  	type ret = (type)~0;						\
>> +	struct logic_pio_hwaddr *range = find_io_range(addr);		\
>> +									\
>> +	if (INVALID_RANGE(range)) {					\
>> +		WARN_ON_ONCE(1);					\
>> +		return ret;						\
>> +	}								\
>>  									\
>>  	if (addr < MMIO_UPPER_LIMIT) {					\
>>  		ret = read##bw(PCI_IOBASE + addr);			\
>>  	} else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) { \
>> -		struct logic_pio_hwaddr *entry = find_io_range(addr);	\
>>  		size_t sz = sizeof(type);				\
>> +		void *hostdata = range->hostdata;			\
>>  									\
>> -		if (entry && entry->ops)				\
>> -			ret = entry->ops->in(entry->hostdata, addr, sz);\
>> -		else							\
>> -			WARN_ON_ONCE(1);				\
>> +		if (range->ops->in)					\
>> +			ret = range->ops->in(hostdata, addr, sz);	\
>>  	}								\
>>  	return ret;							\
>>  }									\
>>  									\
>> -void logic_out##bw(type value, unsigned long addr)			\
>> +void logic_out##bw(type val, unsigned long addr)			\
>>  {									\
>> +	struct logic_pio_hwaddr *range = find_io_range(addr);		\
>> +									\
>> +	if (INVALID_RANGE(range)) {					\
>> +		WARN_ON_ONCE(1);					\
>> +		return;							\
>> +	}								\
>> +									\
>>  	if (addr < MMIO_UPPER_LIMIT) {					\
>> -		write##bw(value, PCI_IOBASE + addr);			\
>> +		write##bw(val, PCI_IOBASE + addr);			\
>>  	} else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {	\
>> -		struct logic_pio_hwaddr *entry = find_io_range(addr);	\
>>  		size_t sz = sizeof(type);				\
>> +		void *hostdata = range->hostdata;			\
>>  									\
>> -		if (entry && entry->ops)				\
>> -			entry->ops->out(entry->hostdata,		\
>> -					addr, value, sz);		\
>> -		else							\
>> -			WARN_ON_ONCE(1);				\
>> +		if (range->ops->out)					\
>> +			range->ops->out(hostdata, addr, val, sz);	\
>>  	}								\
>>  }									\
>>  									\
>>  void logic_ins##bw(unsigned long addr, void *buf, unsigned int cnt)	\
>>  {									\
>> +	struct logic_pio_hwaddr *range = find_io_range(addr);		\
>> +									\
>> +	if (INVALID_RANGE(range)) {					\
>> +		WARN_ON_ONCE(1);					\
>> +		return;							\
>> +	}								\
>> +									\
>>  	if (addr < MMIO_UPPER_LIMIT) {					\
>>  		reads##bw(PCI_IOBASE + addr, buf, cnt);			\
>>  	} else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {	\
>> -		struct logic_pio_hwaddr *entry = find_io_range(addr);	\
>>  		size_t sz = sizeof(type);				\
>> +		void *hostdata = range->hostdata;			\
>>  									\
>> -		if (entry && entry->ops)				\
>> -			entry->ops->ins(entry->hostdata,		\
>> -				addr, buf, sz, cnt);			\
>> -		else							\
>> -			WARN_ON_ONCE(1);				\
>> +		if (range->ops->ins)					\
>> +			range->ops->ins(hostdata, addr, buf, sz, cnt);	\
>>  	}								\
>> -									\
>>  }									\
>>  									\
>>  void logic_outs##bw(unsigned long addr, const void *buf,		\
>>  		    unsigned int cnt)					\
>>  {									\
>> +	struct logic_pio_hwaddr *range = find_io_range(addr);		\
>> +									\
>> +	if (INVALID_RANGE(range)) {					\
>> +		WARN_ON_ONCE(1);					\
>> +		return;							\
>> +	}								\
>> +									\
>>  	if (addr < MMIO_UPPER_LIMIT) {					\
>>  		writes##bw(PCI_IOBASE + addr, buf, cnt);		\
>>  	} else if (addr >= MMIO_UPPER_LIMIT && addr < IO_SPACE_LIMIT) {	\
>> -		struct logic_pio_hwaddr *entry = find_io_range(addr);	\
>>  		size_t sz = sizeof(type);				\
>> +		void *hostdata = range->hostdata;			\
>>  									\
>> -		if (entry && entry->ops)				\
>> -			entry->ops->outs(entry->hostdata,		\
>> -				addr, buf, sz, cnt);			\
>> -		else							\
>> -			WARN_ON_ONCE(1);				\
>> +		if (range->ops->outs)					\
>> +			range->ops->outs(hostdata, addr, buf, sz, cnt);	\
>>  	}								\
>>  }
>>
>>  #else /* CONFIG_INDIRECT_PIO */
>>
>> +#define INVALID_RANGE(range) (!(range))
>> +
>>  #define BUILD_LOGIC_IO(bw, type)					\
>>  type logic_in##bw(unsigned long addr)					\
>>  {									\
>>  	type ret = (type)~0;						\
>> +	struct logic_pio_hwaddr *range = find_io_range(addr);		\
>> +									\
>> +	if (INVALID_RANGE(range)) {					\
>> +		WARN_ON_ONCE(1);					\
>> +		return ret;						\
>> +	}								\
>>  									\
>>  	if (addr < MMIO_UPPER_LIMIT)					\
>>  		ret = read##bw(PCI_IOBASE + addr);			\
>>  	return ret;							\
>>  }									\
>>  									\
>> -void logic_out##bw(type value, unsigned long addr)			\
>> +void logic_out##bw(type val, unsigned long addr)			\
>>  {									\
>> +	struct logic_pio_hwaddr *range = find_io_range(addr);		\
>> +									\
>> +	if (INVALID_RANGE(range)) {					\
>> +		WARN_ON_ONCE(1);					\
>> +		return;							\
>> +	}								\
>> +									\
>>  	if (addr < MMIO_UPPER_LIMIT)					\
>> -		write##bw(value, PCI_IOBASE + addr);			\
>> +		write##bw(val, PCI_IOBASE + addr);			\
>>  }									\
>>  									\
>>  void logic_ins##bw(unsigned long addr, void *buf, unsigned int cnt)	\
>>  {									\
>> +	struct logic_pio_hwaddr *range = find_io_range(addr);		\
>> +									\
>> +	if (INVALID_RANGE(range)) {					\
>> +		WARN_ON_ONCE(1);					\
>> +		return;							\
>> +	}								\
>> +									\
>>  	if (addr < MMIO_UPPER_LIMIT)					\
>>  		reads##bw(PCI_IOBASE + addr, buf, cnt);			\
>>  }									\
>> @@ -289,6 +329,13 @@ void logic_ins##bw(unsigned long addr, void *buf, unsigned int cnt)	\
>>  void logic_outs##bw(unsigned long addr, const void *buf,		\
>>  		    unsigned int cnt)					\
>>  {									\
>> +	struct logic_pio_hwaddr *range = find_io_range(addr);		\
>> +									\
>> +	if (INVALID_RANGE(range)) {					\
>> +		WARN_ON_ONCE(1);					\
>> +		return;							\
>> +	}								\
>> +									\
>>  	if (addr < MMIO_UPPER_LIMIT)					\
>>  		writes##bw(PCI_IOBASE + addr, buf, cnt);		\
>>  }
>> --
>> 2.17.1
>>
>
> .
>



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/4] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions

[Guenter Roeck]

On Thu, Apr 04, 2019 at 05:52:35PM +0100, John Garry wrote:
[ ... ]
> >>
> >>Note that the f71805f driver does not call request_{muxed_}region(), as it
> >>should.
> >>
> 
> Hi Guenter,
> 
> >... which is the real problem, one that is not solved by this patch. This may
> >result in parallel and descructive accesses if there is another device on the
> >LPC bus, and another driver accessing that device. Personally I'd rather have
> >request_muxed_region() added to the f71805f driver.
> 
> Right, we should and will still fix f71805f. If you recall, I did have the
> f71805f fix in the v1 series, but you committed that it was orthogonal, so I
> decided to take it out of this work for now.
> 
> And even if we fix up f71805f and other known drivers which don't call
> request_muxed_region(), we still need to police against these rogue
> accesses, which is what this patch attempts to do.
> 
Do we ? I am personally not convinced that LPC accesses _have_ to occur
through PCI on any given system.

I won't object to this series of patches - not my area. I do mind, though,
if one of the drivers I am responsible for is cited as reason or argument
for this series. I would prefer for those drivers to get fixed.

Guenter

Re: [PATCH v3 3/4] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions

[Guenter Roeck]

On Thu, Apr 04, 2019 at 05:52:35PM +0100, John Garry wrote:
[ ... ]
> >>
> >>Note that the f71805f driver does not call request_{muxed_}region(), as it
> >>should.
> >>
> 
> Hi Guenter,
> 
> >... which is the real problem, one that is not solved by this patch. This may
> >result in parallel and descructive accesses if there is another device on the
> >LPC bus, and another driver accessing that device. Personally I'd rather have
> >request_muxed_region() added to the f71805f driver.
> 
> Right, we should and will still fix f71805f. If you recall, I did have the
> f71805f fix in the v1 series, but you committed that it was orthogonal, so I
> decided to take it out of this work for now.
> 
> And even if we fix up f71805f and other known drivers which don't call
> request_muxed_region(), we still need to police against these rogue
> accesses, which is what this patch attempts to do.
> 
Do we ? I am personally not convinced that LPC accesses _have_ to occur
through PCI on any given system.

I won't object to this series of patches - not my area. I do mind, though,
if one of the drivers I am responsible for is cited as reason or argument
for this series. I would prefer for those drivers to get fixed.

Guenter

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/4] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions

[Bjorn Helgaas]

On Thu, Apr 04, 2019 at 10:43:36AM -0700, Guenter Roeck wrote:
> On Thu, Apr 04, 2019 at 05:52:35PM +0100, John Garry wrote:
> > >>Note that the f71805f driver does not call
> > >>request_{muxed_}region(), as it should.
> > 
> > >... which is the real problem, one that is not solved by this
> > >patch. This may result in parallel and descructive accesses if
> > >there is another device on the LPC bus, and another driver
> > >accessing that device. Personally I'd rather have
> > >request_muxed_region() added to the f71805f driver.
> > 
> > Right, we should and will still fix f71805f. If you recall, I did
> > have the f71805f fix in the v1 series, but you committed that it
> > was orthogonal, so I decided to take it out of this work for now.
> > 
> > And even if we fix up f71805f and other known drivers which don't
> > call request_muxed_region(), we still need to police against these
> > rogue accesses, which is what this patch attempts to do.
> > 
> Do we ? I am personally not convinced that LPC accesses _have_ to
> occur through PCI on any given system.

On current systems, I suspect ISA/LPC devices are typically connected
via a PCI-to-ISA/LPC bridge.  But AFAIK there's no actual requirement
for that bridge, and there certainly *were* systems with ISA devices
but no PCI at all.

IMO, if you want to build ISA drivers on your arch, you need to make
sure the inb() probing done by those drivers works like it does on
x86.  If there's no device there, the inb() should return 0xff with no
fuss and no crash.

Bjorn

Re: [PATCH v3 3/4] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions

[Bjorn Helgaas]

On Thu, Apr 04, 2019 at 10:43:36AM -0700, Guenter Roeck wrote:
> On Thu, Apr 04, 2019 at 05:52:35PM +0100, John Garry wrote:
> > >>Note that the f71805f driver does not call
> > >>request_{muxed_}region(), as it should.
> > 
> > >... which is the real problem, one that is not solved by this
> > >patch. This may result in parallel and descructive accesses if
> > >there is another device on the LPC bus, and another driver
> > >accessing that device. Personally I'd rather have
> > >request_muxed_region() added to the f71805f driver.
> > 
> > Right, we should and will still fix f71805f. If you recall, I did
> > have the f71805f fix in the v1 series, but you committed that it
> > was orthogonal, so I decided to take it out of this work for now.
> > 
> > And even if we fix up f71805f and other known drivers which don't
> > call request_muxed_region(), we still need to police against these
> > rogue accesses, which is what this patch attempts to do.
> > 
> Do we ? I am personally not convinced that LPC accesses _have_ to
> occur through PCI on any given system.

On current systems, I suspect ISA/LPC devices are typically connected
via a PCI-to-ISA/LPC bridge.  But AFAIK there's no actual requirement
for that bridge, and there certainly *were* systems with ISA devices
but no PCI at all.

IMO, if you want to build ISA drivers on your arch, you need to make
sure the inb() probing done by those drivers works like it does on
x86.  If there's no device there, the inb() should return 0xff with no
fuss and no crash.

Bjorn

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/4] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions

[John Garry]

On 04/04/2019 19:58, Bjorn Helgaas wrote:
> On Thu, Apr 04, 2019 at 10:43:36AM -0700, Guenter Roeck wrote:
>> On Thu, Apr 04, 2019 at 05:52:35PM +0100, John Garry wrote:
>>>>> Note that the f71805f driver does not call
>>>>> request_{muxed_}region(), as it should.
>>>
>>>> ... which is the real problem, one that is not solved by this
>>>> patch. This may result in parallel and descructive accesses if
>>>> there is another device on the LPC bus, and another driver
>>>> accessing that device. Personally I'd rather have
>>>> request_muxed_region() added to the f71805f driver.
>>>
>>> Right, we should and will still fix f71805f. If you recall, I did
>>> have the f71805f fix in the v1 series, but you committed that it
>>> was orthogonal, so I decided to take it out of this work for now.
>>>
>>> And even if we fix up f71805f and other known drivers which don't
>>> call request_muxed_region(), we still need to police against these
>>> rogue accesses, which is what this patch attempts to do.
>>>
>> Do we ? I am personally not convinced that LPC accesses _have_ to
>> occur through PCI on any given system.
>
> On current systems, I suspect ISA/LPC devices are typically connected
> via a PCI-to-ISA/LPC bridge.  But AFAIK there's no actual requirement
> for that bridge, and there certainly *were* systems with ISA devices
> but no PCI at all.
>
> IMO, if you want to build ISA drivers on your arch, you need to make
> sure the inb() probing done by those drivers works like it does on
> x86.  If there's no device there, the inb() should return 0xff with no
> fuss and no crash.

Right, and this is what I am attempting to do here.

So today a call to request_muxed_region() can still succeed even if no 
IO space mapped.

As such, even well-behaved drivers like f71882fg can still crash the 
system, as noted in RFC patch 1/4 ("resource: Request IO port regions 
from children of ioport_resource").

Thanks,
John

>
> Bjorn
>
> .
>

Re: [PATCH v3 3/4] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions

[John Garry]

On 04/04/2019 19:58, Bjorn Helgaas wrote:
> On Thu, Apr 04, 2019 at 10:43:36AM -0700, Guenter Roeck wrote:
>> On Thu, Apr 04, 2019 at 05:52:35PM +0100, John Garry wrote:
>>>>> Note that the f71805f driver does not call
>>>>> request_{muxed_}region(), as it should.
>>>
>>>> ... which is the real problem, one that is not solved by this
>>>> patch. This may result in parallel and descructive accesses if
>>>> there is another device on the LPC bus, and another driver
>>>> accessing that device. Personally I'd rather have
>>>> request_muxed_region() added to the f71805f driver.
>>>
>>> Right, we should and will still fix f71805f. If you recall, I did
>>> have the f71805f fix in the v1 series, but you committed that it
>>> was orthogonal, so I decided to take it out of this work for now.
>>>
>>> And even if we fix up f71805f and other known drivers which don't
>>> call request_muxed_region(), we still need to police against these
>>> rogue accesses, which is what this patch attempts to do.
>>>
>> Do we ? I am personally not convinced that LPC accesses _have_ to
>> occur through PCI on any given system.
>
> On current systems, I suspect ISA/LPC devices are typically connected
> via a PCI-to-ISA/LPC bridge.  But AFAIK there's no actual requirement
> for that bridge, and there certainly *were* systems with ISA devices
> but no PCI at all.
>
> IMO, if you want to build ISA drivers on your arch, you need to make
> sure the inb() probing done by those drivers works like it does on
> x86.  If there's no device there, the inb() should return 0xff with no
> fuss and no crash.

Right, and this is what I am attempting to do here.

So today a call to request_muxed_region() can still succeed even if no 
IO space mapped.

As such, even well-behaved drivers like f71882fg can still crash the 
system, as noted in RFC patch 1/4 ("resource: Request IO port regions 
from children of ioport_resource").

Thanks,
John

>
> Bjorn
>
> .
>



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/4] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions

[Bjorn Helgaas]

On Fri, Apr 05, 2019 at 09:10:27AM +0100, John Garry wrote:
> On 04/04/2019 19:58, Bjorn Helgaas wrote:
> > On Thu, Apr 04, 2019 at 10:43:36AM -0700, Guenter Roeck wrote:
> > > On Thu, Apr 04, 2019 at 05:52:35PM +0100, John Garry wrote:
> > > > > > Note that the f71805f driver does not call
> > > > > > request_{muxed_}region(), as it should.
> > > > 
> > > > > ... which is the real problem, one that is not solved by this
> > > > > patch. This may result in parallel and descructive accesses if
> > > > > there is another device on the LPC bus, and another driver
> > > > > accessing that device. Personally I'd rather have
> > > > > request_muxed_region() added to the f71805f driver.
> > > > 
> > > > Right, we should and will still fix f71805f. If you recall, I did
> > > > have the f71805f fix in the v1 series, but you committed that it
> > > > was orthogonal, so I decided to take it out of this work for now.
> > > > 
> > > > And even if we fix up f71805f and other known drivers which don't
> > > > call request_muxed_region(), we still need to police against these
> > > > rogue accesses, which is what this patch attempts to do.
> > > > 
> > > Do we ? I am personally not convinced that LPC accesses _have_ to
> > > occur through PCI on any given system.
> > 
> > On current systems, I suspect ISA/LPC devices are typically connected
> > via a PCI-to-ISA/LPC bridge.  But AFAIK there's no actual requirement
> > for that bridge, and there certainly *were* systems with ISA devices
> > but no PCI at all.
> > 
> > IMO, if you want to build ISA drivers on your arch, you need to make
> > sure the inb() probing done by those drivers works like it does on
> > x86.  If there's no device there, the inb() should return 0xff with no
> > fuss and no crash.
> 
> Right, and this is what I am attempting to do here.
> 
> So today a call to request_muxed_region() can still succeed even if no IO
> space mapped.
> 
> As such, even well-behaved drivers like f71882fg can still crash the system,
> as noted in RFC patch 1/4 ("resource: Request IO port regions from children
> of ioport_resource").

Maybe I'm missing something, but on x86, drivers like f71882fg do not
crash the system because inb() *never* causes a crash.

If you want to build that driver for ARM, I think you need to make
sure that inb() on ARM also *never* causes a crash.  I don't think
changing f71882fg and all the similar drivers is the right answer.

Bjorn

Re: [PATCH v3 3/4] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions

[Bjorn Helgaas]

On Fri, Apr 05, 2019 at 09:10:27AM +0100, John Garry wrote:
> On 04/04/2019 19:58, Bjorn Helgaas wrote:
> > On Thu, Apr 04, 2019 at 10:43:36AM -0700, Guenter Roeck wrote:
> > > On Thu, Apr 04, 2019 at 05:52:35PM +0100, John Garry wrote:
> > > > > > Note that the f71805f driver does not call
> > > > > > request_{muxed_}region(), as it should.
> > > > 
> > > > > ... which is the real problem, one that is not solved by this
> > > > > patch. This may result in parallel and descructive accesses if
> > > > > there is another device on the LPC bus, and another driver
> > > > > accessing that device. Personally I'd rather have
> > > > > request_muxed_region() added to the f71805f driver.
> > > > 
> > > > Right, we should and will still fix f71805f. If you recall, I did
> > > > have the f71805f fix in the v1 series, but you committed that it
> > > > was orthogonal, so I decided to take it out of this work for now.
> > > > 
> > > > And even if we fix up f71805f and other known drivers which don't
> > > > call request_muxed_region(), we still need to police against these
> > > > rogue accesses, which is what this patch attempts to do.
> > > > 
> > > Do we ? I am personally not convinced that LPC accesses _have_ to
> > > occur through PCI on any given system.
> > 
> > On current systems, I suspect ISA/LPC devices are typically connected
> > via a PCI-to-ISA/LPC bridge.  But AFAIK there's no actual requirement
> > for that bridge, and there certainly *were* systems with ISA devices
> > but no PCI at all.
> > 
> > IMO, if you want to build ISA drivers on your arch, you need to make
> > sure the inb() probing done by those drivers works like it does on
> > x86.  If there's no device there, the inb() should return 0xff with no
> > fuss and no crash.
> 
> Right, and this is what I am attempting to do here.
> 
> So today a call to request_muxed_region() can still succeed even if no IO
> space mapped.
> 
> As such, even well-behaved drivers like f71882fg can still crash the system,
> as noted in RFC patch 1/4 ("resource: Request IO port regions from children
> of ioport_resource").

Maybe I'm missing something, but on x86, drivers like f71882fg do not
crash the system because inb() *never* causes a crash.

If you want to build that driver for ARM, I think you need to make
sure that inb() on ARM also *never* causes a crash.  I don't think
changing f71882fg and all the similar drivers is the right answer.

Bjorn

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/4] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions

[Guenter Roeck]

On Fri, Apr 05, 2019 at 01:06:15PM -0500, Bjorn Helgaas wrote:
> On Fri, Apr 05, 2019 at 09:10:27AM +0100, John Garry wrote:
> > On 04/04/2019 19:58, Bjorn Helgaas wrote:
> > > On Thu, Apr 04, 2019 at 10:43:36AM -0700, Guenter Roeck wrote:
> > > > On Thu, Apr 04, 2019 at 05:52:35PM +0100, John Garry wrote:
> > > > > > > Note that the f71805f driver does not call
> > > > > > > request_{muxed_}region(), as it should.
> > > > > 
> > > > > > ... which is the real problem, one that is not solved by this
> > > > > > patch. This may result in parallel and descructive accesses if
> > > > > > there is another device on the LPC bus, and another driver
> > > > > > accessing that device. Personally I'd rather have
> > > > > > request_muxed_region() added to the f71805f driver.
> > > > > 
> > > > > Right, we should and will still fix f71805f. If you recall, I did
> > > > > have the f71805f fix in the v1 series, but you committed that it
> > > > > was orthogonal, so I decided to take it out of this work for now.
> > > > > 
> > > > > And even if we fix up f71805f and other known drivers which don't
> > > > > call request_muxed_region(), we still need to police against these
> > > > > rogue accesses, which is what this patch attempts to do.
> > > > > 
> > > > Do we ? I am personally not convinced that LPC accesses _have_ to
> > > > occur through PCI on any given system.
> > > 
> > > On current systems, I suspect ISA/LPC devices are typically connected
> > > via a PCI-to-ISA/LPC bridge.  But AFAIK there's no actual requirement
> > > for that bridge, and there certainly *were* systems with ISA devices
> > > but no PCI at all.
> > > 
> > > IMO, if you want to build ISA drivers on your arch, you need to make
> > > sure the inb() probing done by those drivers works like it does on
> > > x86.  If there's no device there, the inb() should return 0xff with no
> > > fuss and no crash.
> > 
> > Right, and this is what I am attempting to do here.
> > 
> > So today a call to request_muxed_region() can still succeed even if no IO
> > space mapped.
> > 
> > As such, even well-behaved drivers like f71882fg can still crash the system,
> > as noted in RFC patch 1/4 ("resource: Request IO port regions from children
> > of ioport_resource").
> 
> Maybe I'm missing something, but on x86, drivers like f71882fg do not
> crash the system because inb() *never* causes a crash.
> 
> If you want to build that driver for ARM, I think you need to make
> sure that inb() on ARM also *never* causes a crash.  I don't think
> changing f71882fg and all the similar drivers is the right answer.
> 

Agreed. As I had mentioned earlier, the driver changes are orthogonal:
the drivers should request the IO region before accessing it, primarily
to avoid conflicting accesses by multiple drivers in parallel. For
example, the f71882fg driver supports chips which implement hardware
monitoring as well as watchdog functionality, and both the hwmon
and the watchdog driver may try to access the io space.

If and how the system ensures that the IO region exists and/or that 
inb() always succeeds is a different question. I would prefer a less
complex solution than the one suggested here, but that is my personal
opionion.

Guenter

Re: [PATCH v3 3/4] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions

[Guenter Roeck]

On Fri, Apr 05, 2019 at 01:06:15PM -0500, Bjorn Helgaas wrote:
> On Fri, Apr 05, 2019 at 09:10:27AM +0100, John Garry wrote:
> > On 04/04/2019 19:58, Bjorn Helgaas wrote:
> > > On Thu, Apr 04, 2019 at 10:43:36AM -0700, Guenter Roeck wrote:
> > > > On Thu, Apr 04, 2019 at 05:52:35PM +0100, John Garry wrote:
> > > > > > > Note that the f71805f driver does not call
> > > > > > > request_{muxed_}region(), as it should.
> > > > > 
> > > > > > ... which is the real problem, one that is not solved by this
> > > > > > patch. This may result in parallel and descructive accesses if
> > > > > > there is another device on the LPC bus, and another driver
> > > > > > accessing that device. Personally I'd rather have
> > > > > > request_muxed_region() added to the f71805f driver.
> > > > > 
> > > > > Right, we should and will still fix f71805f. If you recall, I did
> > > > > have the f71805f fix in the v1 series, but you committed that it
> > > > > was orthogonal, so I decided to take it out of this work for now.
> > > > > 
> > > > > And even if we fix up f71805f and other known drivers which don't
> > > > > call request_muxed_region(), we still need to police against these
> > > > > rogue accesses, which is what this patch attempts to do.
> > > > > 
> > > > Do we ? I am personally not convinced that LPC accesses _have_ to
> > > > occur through PCI on any given system.
> > > 
> > > On current systems, I suspect ISA/LPC devices are typically connected
> > > via a PCI-to-ISA/LPC bridge.  But AFAIK there's no actual requirement
> > > for that bridge, and there certainly *were* systems with ISA devices
> > > but no PCI at all.
> > > 
> > > IMO, if you want to build ISA drivers on your arch, you need to make
> > > sure the inb() probing done by those drivers works like it does on
> > > x86.  If there's no device there, the inb() should return 0xff with no
> > > fuss and no crash.
> > 
> > Right, and this is what I am attempting to do here.
> > 
> > So today a call to request_muxed_region() can still succeed even if no IO
> > space mapped.
> > 
> > As such, even well-behaved drivers like f71882fg can still crash the system,
> > as noted in RFC patch 1/4 ("resource: Request IO port regions from children
> > of ioport_resource").
> 
> Maybe I'm missing something, but on x86, drivers like f71882fg do not
> crash the system because inb() *never* causes a crash.
> 
> If you want to build that driver for ARM, I think you need to make
> sure that inb() on ARM also *never* causes a crash.  I don't think
> changing f71882fg and all the similar drivers is the right answer.
> 

Agreed. As I had mentioned earlier, the driver changes are orthogonal:
the drivers should request the IO region before accessing it, primarily
to avoid conflicting accesses by multiple drivers in parallel. For
example, the f71882fg driver supports chips which implement hardware
monitoring as well as watchdog functionality, and both the hwmon
and the watchdog driver may try to access the io space.

If and how the system ensures that the IO region exists and/or that 
inb() always succeeds is a different question. I would prefer a less
complex solution than the one suggested here, but that is my personal
opionion.

Guenter

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/4] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions

[John Garry]

On 05/04/2019 19:29, Guenter Roeck wrote:
> On Fri, Apr 05, 2019 at 01:06:15PM -0500, Bjorn Helgaas wrote:
>> On Fri, Apr 05, 2019 at 09:10:27AM +0100, John Garry wrote:
>>> On 04/04/2019 19:58, Bjorn Helgaas wrote:
>>>> On Thu, Apr 04, 2019 at 10:43:36AM -0700, Guenter Roeck wrote:
>>>>> On Thu, Apr 04, 2019 at 05:52:35PM +0100, John Garry wrote:
>>>>>>>> Note that the f71805f driver does not call
>>>>>>>> request_{muxed_}region(), as it should.
>>>>>>
>>>>>>> ... which is the real problem, one that is not solved by this
>>>>>>> patch. This may result in parallel and descructive accesses if
>>>>>>> there is another device on the LPC bus, and another driver
>>>>>>> accessing that device. Personally I'd rather have
>>>>>>> request_muxed_region() added to the f71805f driver.
>>>>>>
>>>>>> Right, we should and will still fix f71805f. If you recall, I did
>>>>>> have the f71805f fix in the v1 series, but you committed that it
>>>>>> was orthogonal, so I decided to take it out of this work for now.
>>>>>>
>>>>>> And even if we fix up f71805f and other known drivers which don't
>>>>>> call request_muxed_region(), we still need to police against these
>>>>>> rogue accesses, which is what this patch attempts to do.
>>>>>>
>>>>> Do we ? I am personally not convinced that LPC accesses _have_ to
>>>>> occur through PCI on any given system.
>>>>
>>>> On current systems, I suspect ISA/LPC devices are typically connected
>>>> via a PCI-to-ISA/LPC bridge.  But AFAIK there's no actual requirement
>>>> for that bridge, and there certainly *were* systems with ISA devices
>>>> but no PCI at all.
>>>>
>>>> IMO, if you want to build ISA drivers on your arch, you need to make
>>>> sure the inb() probing done by those drivers works like it does on
>>>> x86.  If there's no device there, the inb() should return 0xff with no
>>>> fuss and no crash.
>>>
>>> Right, and this is what I am attempting to do here.
>>>
>>> So today a call to request_muxed_region() can still succeed even if no IO
>>> space mapped.
>>>
>>> As such, even well-behaved drivers like f71882fg can still crash the system,
>>> as noted in RFC patch 1/4 ("resource: Request IO port regions from children
>>> of ioport_resource").
>>
>> Maybe I'm missing something, but on x86, drivers like f71882fg do not
>> crash the system because inb() *never* causes a crash.
>>
>> If you want to build that driver for ARM, I think you need to make
>> sure that inb() on ARM also *never* causes a crash.  I don't think
>> changing f71882fg and all the similar drivers is the right answer.
>>
>
> Agreed. As I had mentioned earlier, the driver changes are orthogonal:
> the drivers should request the IO region before accessing it, primarily
> to avoid conflicting accesses by multiple drivers in parallel. For
> example, the f71882fg driver supports chips which implement hardware
> monitoring as well as watchdog functionality, and both the hwmon
> and the watchdog driver may try to access the io space.
>
> If and how the system ensures that the IO region exists and/or that
> inb() always succeeds is a different question. I would prefer a less
> complex solution than the one suggested here, but that is my personal
> opionion.

Hi Guenter,

I have a question about these super-IO accesses:

To me, it's not good that these hwmon, watchdog, gpio, etc drivers make 
unconstrained accesses to 0x2e and 0x4e ports (ignoring the 
request_muxed_region() call).

The issue I see is that on an arm, IO space for some other device may be 
mapped in this region, so it would not be right for these drivers to 
access those same regions.

Is there any other platform check which can be made to ensure that 
accesses these super-IO ports is appropriate?

Thanks,
John


>
> Guenter
>
> .
>

Re: [PATCH v3 3/4] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions

[John Garry]

On 05/04/2019 19:29, Guenter Roeck wrote:
> On Fri, Apr 05, 2019 at 01:06:15PM -0500, Bjorn Helgaas wrote:
>> On Fri, Apr 05, 2019 at 09:10:27AM +0100, John Garry wrote:
>>> On 04/04/2019 19:58, Bjorn Helgaas wrote:
>>>> On Thu, Apr 04, 2019 at 10:43:36AM -0700, Guenter Roeck wrote:
>>>>> On Thu, Apr 04, 2019 at 05:52:35PM +0100, John Garry wrote:
>>>>>>>> Note that the f71805f driver does not call
>>>>>>>> request_{muxed_}region(), as it should.
>>>>>>
>>>>>>> ... which is the real problem, one that is not solved by this
>>>>>>> patch. This may result in parallel and descructive accesses if
>>>>>>> there is another device on the LPC bus, and another driver
>>>>>>> accessing that device. Personally I'd rather have
>>>>>>> request_muxed_region() added to the f71805f driver.
>>>>>>
>>>>>> Right, we should and will still fix f71805f. If you recall, I did
>>>>>> have the f71805f fix in the v1 series, but you committed that it
>>>>>> was orthogonal, so I decided to take it out of this work for now.
>>>>>>
>>>>>> And even if we fix up f71805f and other known drivers which don't
>>>>>> call request_muxed_region(), we still need to police against these
>>>>>> rogue accesses, which is what this patch attempts to do.
>>>>>>
>>>>> Do we ? I am personally not convinced that LPC accesses _have_ to
>>>>> occur through PCI on any given system.
>>>>
>>>> On current systems, I suspect ISA/LPC devices are typically connected
>>>> via a PCI-to-ISA/LPC bridge.  But AFAIK there's no actual requirement
>>>> for that bridge, and there certainly *were* systems with ISA devices
>>>> but no PCI at all.
>>>>
>>>> IMO, if you want to build ISA drivers on your arch, you need to make
>>>> sure the inb() probing done by those drivers works like it does on
>>>> x86.  If there's no device there, the inb() should return 0xff with no
>>>> fuss and no crash.
>>>
>>> Right, and this is what I am attempting to do here.
>>>
>>> So today a call to request_muxed_region() can still succeed even if no IO
>>> space mapped.
>>>
>>> As such, even well-behaved drivers like f71882fg can still crash the system,
>>> as noted in RFC patch 1/4 ("resource: Request IO port regions from children
>>> of ioport_resource").
>>
>> Maybe I'm missing something, but on x86, drivers like f71882fg do not
>> crash the system because inb() *never* causes a crash.
>>
>> If you want to build that driver for ARM, I think you need to make
>> sure that inb() on ARM also *never* causes a crash.  I don't think
>> changing f71882fg and all the similar drivers is the right answer.
>>
>
> Agreed. As I had mentioned earlier, the driver changes are orthogonal:
> the drivers should request the IO region before accessing it, primarily
> to avoid conflicting accesses by multiple drivers in parallel. For
> example, the f71882fg driver supports chips which implement hardware
> monitoring as well as watchdog functionality, and both the hwmon
> and the watchdog driver may try to access the io space.
>
> If and how the system ensures that the IO region exists and/or that
> inb() always succeeds is a different question. I would prefer a less
> complex solution than the one suggested here, but that is my personal
> opionion.

Hi Guenter,

I have a question about these super-IO accesses:

To me, it's not good that these hwmon, watchdog, gpio, etc drivers make 
unconstrained accesses to 0x2e and 0x4e ports (ignoring the 
request_muxed_region() call).

The issue I see is that on an arm, IO space for some other device may be 
mapped in this region, so it would not be right for these drivers to 
access those same regions.

Is there any other platform check which can be made to ensure that 
accesses these super-IO ports is appropriate?

Thanks,
John


>
> Guenter
>
> .
>



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/4] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions

[Guenter Roeck]

On 4/8/19 1:19 AM, John Garry wrote:
> On 05/04/2019 19:29, Guenter Roeck wrote:
>> On Fri, Apr 05, 2019 at 01:06:15PM -0500, Bjorn Helgaas wrote:
>>> On Fri, Apr 05, 2019 at 09:10:27AM +0100, John Garry wrote:
>>>> On 04/04/2019 19:58, Bjorn Helgaas wrote:
>>>>> On Thu, Apr 04, 2019 at 10:43:36AM -0700, Guenter Roeck wrote:
>>>>>> On Thu, Apr 04, 2019 at 05:52:35PM +0100, John Garry wrote:
>>>>>>>>> Note that the f71805f driver does not call
>>>>>>>>> request_{muxed_}region(), as it should.
>>>>>>>
>>>>>>>> ... which is the real problem, one that is not solved by this
>>>>>>>> patch. This may result in parallel and descructive accesses if
>>>>>>>> there is another device on the LPC bus, and another driver
>>>>>>>> accessing that device. Personally I'd rather have
>>>>>>>> request_muxed_region() added to the f71805f driver.
>>>>>>>
>>>>>>> Right, we should and will still fix f71805f. If you recall, I did
>>>>>>> have the f71805f fix in the v1 series, but you committed that it
>>>>>>> was orthogonal, so I decided to take it out of this work for now.
>>>>>>>
>>>>>>> And even if we fix up f71805f and other known drivers which don't
>>>>>>> call request_muxed_region(), we still need to police against these
>>>>>>> rogue accesses, which is what this patch attempts to do.
>>>>>>>
>>>>>> Do we ? I am personally not convinced that LPC accesses _have_ to
>>>>>> occur through PCI on any given system.
>>>>>
>>>>> On current systems, I suspect ISA/LPC devices are typically connected
>>>>> via a PCI-to-ISA/LPC bridge.  But AFAIK there's no actual requirement
>>>>> for that bridge, and there certainly *were* systems with ISA devices
>>>>> but no PCI at all.
>>>>>
>>>>> IMO, if you want to build ISA drivers on your arch, you need to make
>>>>> sure the inb() probing done by those drivers works like it does on
>>>>> x86.  If there's no device there, the inb() should return 0xff with no
>>>>> fuss and no crash.
>>>>
>>>> Right, and this is what I am attempting to do here.
>>>>
>>>> So today a call to request_muxed_region() can still succeed even if no IO
>>>> space mapped.
>>>>
>>>> As such, even well-behaved drivers like f71882fg can still crash the system,
>>>> as noted in RFC patch 1/4 ("resource: Request IO port regions from children
>>>> of ioport_resource").
>>>
>>> Maybe I'm missing something, but on x86, drivers like f71882fg do not
>>> crash the system because inb() *never* causes a crash.
>>>
>>> If you want to build that driver for ARM, I think you need to make
>>> sure that inb() on ARM also *never* causes a crash.  I don't think
>>> changing f71882fg and all the similar drivers is the right answer.
>>>
>>
>> Agreed. As I had mentioned earlier, the driver changes are orthogonal:
>> the drivers should request the IO region before accessing it, primarily
>> to avoid conflicting accesses by multiple drivers in parallel. For
>> example, the f71882fg driver supports chips which implement hardware
>> monitoring as well as watchdog functionality, and both the hwmon
>> and the watchdog driver may try to access the io space.
>>
>> If and how the system ensures that the IO region exists and/or that
>> inb() always succeeds is a different question. I would prefer a less
>> complex solution than the one suggested here, but that is my personal
>> opionion.
> 
> Hi Guenter,
> 
> I have a question about these super-IO accesses:
> 
> To me, it's not good that these hwmon, watchdog, gpio, etc drivers make unconstrained accesses to 0x2e and 0x4e ports (ignoring the request_muxed_region() call).
> 
> The issue I see is that on an arm, IO space for some other device may be mapped in this region, so it would not be right for these drivers to access those same regions.
> 
Yes, but then there _could_ be some arm or arm64 device supporting one of those chips,
so we can not just add something like "depends on !(ARM || ARM64)".

> Is there any other platform check which can be made to ensure that accesses these super-IO ports is appropriate?
> 

Not that I know of. It would make some sense to provide API functions
for Super-IO accesses, but that would be a lot of work, and I guess
it isn't really valuable enough for anyone to pick up and do.

Normally, if you have such a system, the respective drivers should not be
built. After all, this isn't the only instance where drivers unconditionally
access some io region, no matter if the underlying hardware exists or not.
The only real defense against that is to not build those drivers into
a given kernel.

Guenter

Re: [PATCH v3 3/4] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions

[Guenter Roeck]

On 4/8/19 1:19 AM, John Garry wrote:
> On 05/04/2019 19:29, Guenter Roeck wrote:
>> On Fri, Apr 05, 2019 at 01:06:15PM -0500, Bjorn Helgaas wrote:
>>> On Fri, Apr 05, 2019 at 09:10:27AM +0100, John Garry wrote:
>>>> On 04/04/2019 19:58, Bjorn Helgaas wrote:
>>>>> On Thu, Apr 04, 2019 at 10:43:36AM -0700, Guenter Roeck wrote:
>>>>>> On Thu, Apr 04, 2019 at 05:52:35PM +0100, John Garry wrote:
>>>>>>>>> Note that the f71805f driver does not call
>>>>>>>>> request_{muxed_}region(), as it should.
>>>>>>>
>>>>>>>> ... which is the real problem, one that is not solved by this
>>>>>>>> patch. This may result in parallel and descructive accesses if
>>>>>>>> there is another device on the LPC bus, and another driver
>>>>>>>> accessing that device. Personally I'd rather have
>>>>>>>> request_muxed_region() added to the f71805f driver.
>>>>>>>
>>>>>>> Right, we should and will still fix f71805f. If you recall, I did
>>>>>>> have the f71805f fix in the v1 series, but you committed that it
>>>>>>> was orthogonal, so I decided to take it out of this work for now.
>>>>>>>
>>>>>>> And even if we fix up f71805f and other known drivers which don't
>>>>>>> call request_muxed_region(), we still need to police against these
>>>>>>> rogue accesses, which is what this patch attempts to do.
>>>>>>>
>>>>>> Do we ? I am personally not convinced that LPC accesses _have_ to
>>>>>> occur through PCI on any given system.
>>>>>
>>>>> On current systems, I suspect ISA/LPC devices are typically connected
>>>>> via a PCI-to-ISA/LPC bridge.  But AFAIK there's no actual requirement
>>>>> for that bridge, and there certainly *were* systems with ISA devices
>>>>> but no PCI at all.
>>>>>
>>>>> IMO, if you want to build ISA drivers on your arch, you need to make
>>>>> sure the inb() probing done by those drivers works like it does on
>>>>> x86.  If there's no device there, the inb() should return 0xff with no
>>>>> fuss and no crash.
>>>>
>>>> Right, and this is what I am attempting to do here.
>>>>
>>>> So today a call to request_muxed_region() can still succeed even if no IO
>>>> space mapped.
>>>>
>>>> As such, even well-behaved drivers like f71882fg can still crash the system,
>>>> as noted in RFC patch 1/4 ("resource: Request IO port regions from children
>>>> of ioport_resource").
>>>
>>> Maybe I'm missing something, but on x86, drivers like f71882fg do not
>>> crash the system because inb() *never* causes a crash.
>>>
>>> If you want to build that driver for ARM, I think you need to make
>>> sure that inb() on ARM also *never* causes a crash.  I don't think
>>> changing f71882fg and all the similar drivers is the right answer.
>>>
>>
>> Agreed. As I had mentioned earlier, the driver changes are orthogonal:
>> the drivers should request the IO region before accessing it, primarily
>> to avoid conflicting accesses by multiple drivers in parallel. For
>> example, the f71882fg driver supports chips which implement hardware
>> monitoring as well as watchdog functionality, and both the hwmon
>> and the watchdog driver may try to access the io space.
>>
>> If and how the system ensures that the IO region exists and/or that
>> inb() always succeeds is a different question. I would prefer a less
>> complex solution than the one suggested here, but that is my personal
>> opionion.
> 
> Hi Guenter,
> 
> I have a question about these super-IO accesses:
> 
> To me, it's not good that these hwmon, watchdog, gpio, etc drivers make unconstrained accesses to 0x2e and 0x4e ports (ignoring the request_muxed_region() call).
> 
> The issue I see is that on an arm, IO space for some other device may be mapped in this region, so it would not be right for these drivers to access those same regions.
> 
Yes, but then there _could_ be some arm or arm64 device supporting one of those chips,
so we can not just add something like "depends on !(ARM || ARM64)".

> Is there any other platform check which can be made to ensure that accesses these super-IO ports is appropriate?
> 

Not that I know of. It would make some sense to provide API functions
for Super-IO accesses, but that would be a lot of work, and I guess
it isn't really valuable enough for anyone to pick up and do.

Normally, if you have such a system, the respective drivers should not be
built. After all, this isn't the only instance where drivers unconditionally
access some io region, no matter if the underlying hardware exists or not.
The only real defense against that is to not build those drivers into
a given kernel.

Guenter

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/4] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions

[John Garry]

On 08/04/2019 14:47, Guenter Roeck wrote:
>>>>> FC patch 1/4 ("resource: Request IO port regions from children
>>>>> of ioport_resource").
>>>>
>>>> Maybe I'm missing something, but on x86, drivers like f71882fg do not
>>>> crash the system because inb() *never* causes a crash.
>>>>
>>>> If you want to build that driver for ARM, I think you need to make
>>>> sure that inb() on ARM also *never* causes a crash.  I don't think
>>>> changing f71882fg and all the similar drivers is the right answer.
>>>>
>>>
>>> Agreed. As I had mentioned earlier, the driver changes are orthogonal:
>>> the drivers should request the IO region before accessing it, primarily
>>> to avoid conflicting accesses by multiple drivers in parallel. For
>>> example, the f71882fg driver supports chips which implement hardware
>>> monitoring as well as watchdog functionality, and both the hwmon
>>> and the watchdog driver may try to access the io space.
>>>
>>> If and how the system ensures that the IO region exists and/or that
>>> inb() always succeeds is a different question. I would prefer a less
>>> complex solution than the one suggested here, but that is my personal
>>> opionion.
>>
>> Hi Guenter,
>>
>> I have a question about these super-IO accesses:
>>
>> To me, it's not good that these hwmon, watchdog, gpio, etc drivers
>> make unconstrained accesses to 0x2e and 0x4e ports (ignoring the
>> request_muxed_region() call).
>>
>> The issue I see is that on an arm, IO space for some other device may
>> be mapped in this region, so it would not be right for these drivers
>> to access those same regions.
>>
> Yes, but then there _could_ be some arm or arm64 device supporting one
> of those chips,
> so we can not just add something like "depends on !(ARM || ARM64)".

This looks like what has been added for PPC in commmit 746cdfbf01c0.

However, agreed, it's not a good approach.

>
>> Is there any other platform check which can be made to ensure that
>> accesses these super-IO ports is appropriate?
>>
>
> Not that I know of. It would make some sense to provide API functions
> for Super-IO accesses, but that would be a lot of work, and I guess
> it isn't really valuable enough for anyone to pick up and do.
>
> Normally, if you have such a system, the respective drivers should not be
> built. After all, this isn't the only instance where drivers
> unconditionally
> access some io region, no matter if the underlying hardware exists or not.
> The only real defense against that is to not build those drivers into
> a given kernel.

If we're going to support a multi-plaform kernel for a given arch, then 
we can't always avoid it.

It seems that the only solution on the table now is to discard these IO 
port accesses on arm64 when the IO port are not mapped.

Thanks again,
John

>
> Guenter
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
> .

Re: [PATCH v3 3/4] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions

[John Garry]

On 08/04/2019 14:47, Guenter Roeck wrote:
>>>>> FC patch 1/4 ("resource: Request IO port regions from children
>>>>> of ioport_resource").
>>>>
>>>> Maybe I'm missing something, but on x86, drivers like f71882fg do not
>>>> crash the system because inb() *never* causes a crash.
>>>>
>>>> If you want to build that driver for ARM, I think you need to make
>>>> sure that inb() on ARM also *never* causes a crash.  I don't think
>>>> changing f71882fg and all the similar drivers is the right answer.
>>>>
>>>
>>> Agreed. As I had mentioned earlier, the driver changes are orthogonal:
>>> the drivers should request the IO region before accessing it, primarily
>>> to avoid conflicting accesses by multiple drivers in parallel. For
>>> example, the f71882fg driver supports chips which implement hardware
>>> monitoring as well as watchdog functionality, and both the hwmon
>>> and the watchdog driver may try to access the io space.
>>>
>>> If and how the system ensures that the IO region exists and/or that
>>> inb() always succeeds is a different question. I would prefer a less
>>> complex solution than the one suggested here, but that is my personal
>>> opionion.
>>
>> Hi Guenter,
>>
>> I have a question about these super-IO accesses:
>>
>> To me, it's not good that these hwmon, watchdog, gpio, etc drivers
>> make unconstrained accesses to 0x2e and 0x4e ports (ignoring the
>> request_muxed_region() call).
>>
>> The issue I see is that on an arm, IO space for some other device may
>> be mapped in this region, so it would not be right for these drivers
>> to access those same regions.
>>
> Yes, but then there _could_ be some arm or arm64 device supporting one
> of those chips,
> so we can not just add something like "depends on !(ARM || ARM64)".

This looks like what has been added for PPC in commmit 746cdfbf01c0.

However, agreed, it's not a good approach.

>
>> Is there any other platform check which can be made to ensure that
>> accesses these super-IO ports is appropriate?
>>
>
> Not that I know of. It would make some sense to provide API functions
> for Super-IO accesses, but that would be a lot of work, and I guess
> it isn't really valuable enough for anyone to pick up and do.
>
> Normally, if you have such a system, the respective drivers should not be
> built. After all, this isn't the only instance where drivers
> unconditionally
> access some io region, no matter if the underlying hardware exists or not.
> The only real defense against that is to not build those drivers into
> a given kernel.

If we're going to support a multi-plaform kernel for a given arch, then 
we can't always avoid it.

It seems that the only solution on the table now is to discard these IO 
port accesses on arm64 when the IO port are not mapped.

Thanks again,
John

>
> Guenter
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
> .



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/4] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions

[Will Deacon]

On Mon, Apr 08, 2019 at 05:35:51PM +0100, John Garry wrote:
> On 08/04/2019 14:47, Guenter Roeck wrote:
> > > > > > FC patch 1/4 ("resource: Request IO port regions from children
> > > > > > of ioport_resource").
> > > > > 
> > > > > Maybe I'm missing something, but on x86, drivers like f71882fg do not
> > > > > crash the system because inb() *never* causes a crash.
> > > > > 
> > > > > If you want to build that driver for ARM, I think you need to make
> > > > > sure that inb() on ARM also *never* causes a crash.  I don't think
> > > > > changing f71882fg and all the similar drivers is the right answer.
> > > > > 
> > > > 
> > > > Agreed. As I had mentioned earlier, the driver changes are orthogonal:
> > > > the drivers should request the IO region before accessing it, primarily
> > > > to avoid conflicting accesses by multiple drivers in parallel. For
> > > > example, the f71882fg driver supports chips which implement hardware
> > > > monitoring as well as watchdog functionality, and both the hwmon
> > > > and the watchdog driver may try to access the io space.
> > > > 
> > > > If and how the system ensures that the IO region exists and/or that
> > > > inb() always succeeds is a different question. I would prefer a less
> > > > complex solution than the one suggested here, but that is my personal
> > > > opionion.
> > > 
> > > Hi Guenter,
> > > 
> > > I have a question about these super-IO accesses:
> > > 
> > > To me, it's not good that these hwmon, watchdog, gpio, etc drivers
> > > make unconstrained accesses to 0x2e and 0x4e ports (ignoring the
> > > request_muxed_region() call).
> > > 
> > > The issue I see is that on an arm, IO space for some other device may
> > > be mapped in this region, so it would not be right for these drivers
> > > to access those same regions.
> > > 
> > Yes, but then there _could_ be some arm or arm64 device supporting one
> > of those chips,
> > so we can not just add something like "depends on !(ARM || ARM64)".
> 
> This looks like what has been added for PPC in commmit 746cdfbf01c0.
> 
> However, agreed, it's not a good approach.
> 
> > 
> > > Is there any other platform check which can be made to ensure that
> > > accesses these super-IO ports is appropriate?
> > > 
> > 
> > Not that I know of. It would make some sense to provide API functions
> > for Super-IO accesses, but that would be a lot of work, and I guess
> > it isn't really valuable enough for anyone to pick up and do.
> > 
> > Normally, if you have such a system, the respective drivers should not be
> > built. After all, this isn't the only instance where drivers
> > unconditionally
> > access some io region, no matter if the underlying hardware exists or not.
> > The only real defense against that is to not build those drivers into
> > a given kernel.
> 
> If we're going to support a multi-plaform kernel for a given arch, then we
> can't always avoid it.
> 
> It seems that the only solution on the table now is to discard these IO port
> accesses on arm64 when the IO port are not mapped.

Hmm, how are you going to achieve that? I'm not sure we can guarantee a
synchronous abort, so I'd be nervous about anything that tries to handle
the exception after making the unmapped access.

Will

Re: [PATCH v3 3/4] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions

[Will Deacon]

On Mon, Apr 08, 2019 at 05:35:51PM +0100, John Garry wrote:
> On 08/04/2019 14:47, Guenter Roeck wrote:
> > > > > > FC patch 1/4 ("resource: Request IO port regions from children
> > > > > > of ioport_resource").
> > > > > 
> > > > > Maybe I'm missing something, but on x86, drivers like f71882fg do not
> > > > > crash the system because inb() *never* causes a crash.
> > > > > 
> > > > > If you want to build that driver for ARM, I think you need to make
> > > > > sure that inb() on ARM also *never* causes a crash.  I don't think
> > > > > changing f71882fg and all the similar drivers is the right answer.
> > > > > 
> > > > 
> > > > Agreed. As I had mentioned earlier, the driver changes are orthogonal:
> > > > the drivers should request the IO region before accessing it, primarily
> > > > to avoid conflicting accesses by multiple drivers in parallel. For
> > > > example, the f71882fg driver supports chips which implement hardware
> > > > monitoring as well as watchdog functionality, and both the hwmon
> > > > and the watchdog driver may try to access the io space.
> > > > 
> > > > If and how the system ensures that the IO region exists and/or that
> > > > inb() always succeeds is a different question. I would prefer a less
> > > > complex solution than the one suggested here, but that is my personal
> > > > opionion.
> > > 
> > > Hi Guenter,
> > > 
> > > I have a question about these super-IO accesses:
> > > 
> > > To me, it's not good that these hwmon, watchdog, gpio, etc drivers
> > > make unconstrained accesses to 0x2e and 0x4e ports (ignoring the
> > > request_muxed_region() call).
> > > 
> > > The issue I see is that on an arm, IO space for some other device may
> > > be mapped in this region, so it would not be right for these drivers
> > > to access those same regions.
> > > 
> > Yes, but then there _could_ be some arm or arm64 device supporting one
> > of those chips,
> > so we can not just add something like "depends on !(ARM || ARM64)".
> 
> This looks like what has been added for PPC in commmit 746cdfbf01c0.
> 
> However, agreed, it's not a good approach.
> 
> > 
> > > Is there any other platform check which can be made to ensure that
> > > accesses these super-IO ports is appropriate?
> > > 
> > 
> > Not that I know of. It would make some sense to provide API functions
> > for Super-IO accesses, but that would be a lot of work, and I guess
> > it isn't really valuable enough for anyone to pick up and do.
> > 
> > Normally, if you have such a system, the respective drivers should not be
> > built. After all, this isn't the only instance where drivers
> > unconditionally
> > access some io region, no matter if the underlying hardware exists or not.
> > The only real defense against that is to not build those drivers into
> > a given kernel.
> 
> If we're going to support a multi-plaform kernel for a given arch, then we
> can't always avoid it.
> 
> It seems that the only solution on the table now is to discard these IO port
> accesses on arm64 when the IO port are not mapped.

Hmm, how are you going to achieve that? I'm not sure we can guarantee a
synchronous abort, so I'd be nervous about anything that tries to handle
the exception after making the unmapped access.

Will

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/4] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions

[John Garry]

>>>>
>>>> To me, it's not good that these hwmon, watchdog, gpio, etc drivers
>>>> make unconstrained accesses to 0x2e and 0x4e ports (ignoring the
>>>> request_muxed_region() call).
>>>>
>>>> The issue I see is that on an arm, IO space for some other device may
>>>> be mapped in this region, so it would not be right for these drivers
>>>> to access those same regions.
>>>>
>>> Yes, but then there _could_ be some arm or arm64 device supporting one
>>> of those chips,
>>> so we can not just add something like "depends on !(ARM || ARM64)".
>>
>> This looks like what has been added for PPC in commmit 746cdfbf01c0.
>>
>> However, agreed, it's not a good approach.
>>
>>>
>>>> Is there any other platform check which can be made to ensure that
>>>> accesses these super-IO ports is appropriate?
>>>>
>>>
>>> Not that I know of. It would make some sense to provide API functions
>>> for Super-IO accesses, but that would be a lot of work, and I guess
>>> it isn't really valuable enough for anyone to pick up and do.
>>>
>>> Normally, if you have such a system, the respective drivers should not be
>>> built. After all, this isn't the only instance where drivers
>>> unconditionally
>>> access some io region, no matter if the underlying hardware exists or not.
>>> The only real defense against that is to not build those drivers into
>>> a given kernel.
>>
>> If we're going to support a multi-plaform kernel for a given arch, then we
>> can't always avoid it.
>>
>> It seems that the only solution on the table now is to discard these IO port
>> accesses on arm64 when the IO port are not mapped.
>
> Hmm, how are you going to achieve that? I'm not sure we can guarantee a
> synchronous abort, so I'd be nervous about anything that tries to handle
> the exception after making the unmapped access.
>

Patches 2+3 in this series are the attempt to solve it.

The solution is not in handling an abort generated in accessing the 
unmapped region, but rather avoiding any potential abort and just 
discard the access if the region is unmapped. This is done in the IO 
port accessors, by checking if a PCI host has registered a region in 
"logical PIO" space.

For more info on logical PIO space, see commits 031e3601869c ("lib: Add 
generic PIO mapping method") and 5745392e0c2b ("PCI: Apply the new 
generic I/O management on PCI IO hosts").

In these patches, I add a check in memory-mapped IO port region accesses 
to ensure that a PCI host has registered a region, i.e. IO ports are mapped.

With these patches, we now get the following on an arm64 system with no 
PCI host:

root@(none)$ insmod hwmon/f71882fg.ko
[   30.950909] LOGIC PIO: PIO entry token 0x2e invalid
[   30.955795] LOGIC PIO: PIO entry token 0x2e invalid
[   30.960664] LOGIC PIO: PIO entry token 0x2e invalid
[   30.965531] LOGIC PIO: PIO entry token 0x2f invalid
[   30.970398] LOGIC PIO: PIO entry token 0x2e invalid
[   30.975264] LOGIC PIO: PIO entry token 0x2f invalid
[   30.980131] LOGIC PIO: PIO entry token 0x2e invalid
[   30.984997] LOGIC PIO: PIO entry token 0x4e invalid
[   30.989864] LOGIC PIO: PIO entry token 0x4e invalid
[   30.994730] LOGIC PIO: PIO entry token 0x4e invalid
[   30.999596] LOGIC PIO: PIO entry token 0x4f invalid
[   31.004463] LOGIC PIO: PIO entry token 0x4e invalid
[   31.009329] LOGIC PIO: PIO entry token 0x4f invalid
[   31.014195] LOGIC PIO: PIO entry token 0x4e invalid
insmod: can't insert 'hwmon/f71882fg.ko': No such device
root@(none)$

Note: I've removed a WARN_ONCE from the patches for this experiment. 
Maybe I'll permanently remove.

Without the patches:

root@(none)$ insmod hwmon/f71882fg.ko
[   82.001270] Unable to handle kernel paging request at virtual address 
ffff7dfffee0002e
[   82.009194] Mem abort info:
[   82.011980]   ESR = 0x96000046
[   82.015025]   Exception class = DABT (current EL), IL = 32 bits
[   82.020934]   SET = 0, FnV = 0
[   82.023978]   EA = 0, S1PTW = 0
[   82.027108] Data abort info:
[   82.029975]   ISV = 0, ISS = 0x00000046
[   82.033800]   CM = 0, WnR = 1
[   82.036757] swapper pgtable: 4k pages, 48-bit VAs, pgdp = 
(____ptrval____)
[   82.043620] [ffff7dfffee0002e] pgd=000000000141c003, 
pud=000000000141d003, pmd=0000000000000000
[   82.052309] Internal error: Oops: 96000046 [#1] PREEMPT SMP
[   82.057867] Modules linked in: f71882fg(+)
[   82.061953] CPU: 10 PID: 2731 Comm: insmod Not tainted 
5.1.0-rc1-00003-g938306ea0c86-dirty #230
[   82.070637] Hardware name: Huawei Taishan 2280 /D05, BIOS Hisilicon 
D05 IT21 Nemo 2.0 RC0 04/18/2018
[   82.079754] pstate: 80000005 (Nzcv daif -PAN -UAO)
[   82.084538] pc : logic_outb+0x54/0xb8
[   82.088188] lr : f71882fg_find+0x64/0x390 [f71882fg]
[   82.093139] sp : ffff000026a1baa0
[   82.096439] x29: ffff000026a1baa0 x28: ffff000008b98b10
[   82.101738] x27: ffff000026a1bdf0 x26: 0000000000000100
[   82.107037] x25: ffff801fb5391530 x24: ffff000011420000
[   82.112335] x23: ffff801fb68a3700 x22: ffff000011291000
[   82.117634] x21: 000000000000002e x20: 0000000000000087
[   82.122932] x19: ffff000026a1bb44 x18: ffffffffffffffff
[   82.128230] x17: 0000000000000000 x16: 0000000000000000
[   82.133528] x15: ffff00001127d6c8 x14: 0000000000000000
[   82.138826] x13: 0000000000000000 x12: 0000000000000000
[   82.144124] x11: ffff801ffbf92c80 x10: 0000801fead0f000
[   82.149422] x9 : 0000000000000000 x8 : ffff841fa6403680
[   82.154721] x7 : 0000000000000000 x6 : 000000000000003f
[   82.160019] x5 : ffff000011291360 x4 : 0000000000000000
[   82.165318] x3 : 0000000000ffbffe x2 : eed92a7132399200
[   82.170616] x1 : ffff7dfffee0002e x0 : ffff7dfffee00000
[   82.175915] Process insmod (pid: 2731, stack limit = 0x(____ptrval____))
[   82.182601] Call trace:
[   82.185035]  logic_outb+0x54/0xb8
[   82.188338]  f71882fg_find+0x64/0x390 [f71882fg]
[   82.192943]  f71882fg_init+0x38/0xc70 [f71882fg]
[   82.197548]  do_one_initcall+0x5c/0x198
[   82.201372]  do_init_module+0x54/0x1b0
[   82.205107]  load_module+0x1dc4/0x2158
[   82.208843]  __se_sys_init_module+0x14c/0x1e8
[   82.213186]  __arm64_sys_init_module+0x18/0x20
[   82.217618]  el0_svc_common+0x5c/0x100
[   82.221353]  el0_svc_handler+0x2c/0x80
[   82.225089]  el0_svc+0x8/0xc
[   82.227957] Code: d2bfdc00 f2cfbfe0 f2ffffe0 8b000021 (39000034)
[   82.234038] ---[ end trace 57421eeb25dcc011 ]---
Segmentation fault
root@(none)$

@Guenter, sorry for citing a hwmon driver again as triggering a crash...

John


> Will
>
> .
>

Re: [PATCH v3 3/4] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions

[John Garry]

>>>>
>>>> To me, it's not good that these hwmon, watchdog, gpio, etc drivers
>>>> make unconstrained accesses to 0x2e and 0x4e ports (ignoring the
>>>> request_muxed_region() call).
>>>>
>>>> The issue I see is that on an arm, IO space for some other device may
>>>> be mapped in this region, so it would not be right for these drivers
>>>> to access those same regions.
>>>>
>>> Yes, but then there _could_ be some arm or arm64 device supporting one
>>> of those chips,
>>> so we can not just add something like "depends on !(ARM || ARM64)".
>>
>> This looks like what has been added for PPC in commmit 746cdfbf01c0.
>>
>> However, agreed, it's not a good approach.
>>
>>>
>>>> Is there any other platform check which can be made to ensure that
>>>> accesses these super-IO ports is appropriate?
>>>>
>>>
>>> Not that I know of. It would make some sense to provide API functions
>>> for Super-IO accesses, but that would be a lot of work, and I guess
>>> it isn't really valuable enough for anyone to pick up and do.
>>>
>>> Normally, if you have such a system, the respective drivers should not be
>>> built. After all, this isn't the only instance where drivers
>>> unconditionally
>>> access some io region, no matter if the underlying hardware exists or not.
>>> The only real defense against that is to not build those drivers into
>>> a given kernel.
>>
>> If we're going to support a multi-plaform kernel for a given arch, then we
>> can't always avoid it.
>>
>> It seems that the only solution on the table now is to discard these IO port
>> accesses on arm64 when the IO port are not mapped.
>
> Hmm, how are you going to achieve that? I'm not sure we can guarantee a
> synchronous abort, so I'd be nervous about anything that tries to handle
> the exception after making the unmapped access.
>

Patches 2+3 in this series are the attempt to solve it.

The solution is not in handling an abort generated in accessing the 
unmapped region, but rather avoiding any potential abort and just 
discard the access if the region is unmapped. This is done in the IO 
port accessors, by checking if a PCI host has registered a region in 
"logical PIO" space.

For more info on logical PIO space, see commits 031e3601869c ("lib: Add 
generic PIO mapping method") and 5745392e0c2b ("PCI: Apply the new 
generic I/O management on PCI IO hosts").

In these patches, I add a check in memory-mapped IO port region accesses 
to ensure that a PCI host has registered a region, i.e. IO ports are mapped.

With these patches, we now get the following on an arm64 system with no 
PCI host:

root@(none)$ insmod hwmon/f71882fg.ko
[   30.950909] LOGIC PIO: PIO entry token 0x2e invalid
[   30.955795] LOGIC PIO: PIO entry token 0x2e invalid
[   30.960664] LOGIC PIO: PIO entry token 0x2e invalid
[   30.965531] LOGIC PIO: PIO entry token 0x2f invalid
[   30.970398] LOGIC PIO: PIO entry token 0x2e invalid
[   30.975264] LOGIC PIO: PIO entry token 0x2f invalid
[   30.980131] LOGIC PIO: PIO entry token 0x2e invalid
[   30.984997] LOGIC PIO: PIO entry token 0x4e invalid
[   30.989864] LOGIC PIO: PIO entry token 0x4e invalid
[   30.994730] LOGIC PIO: PIO entry token 0x4e invalid
[   30.999596] LOGIC PIO: PIO entry token 0x4f invalid
[   31.004463] LOGIC PIO: PIO entry token 0x4e invalid
[   31.009329] LOGIC PIO: PIO entry token 0x4f invalid
[   31.014195] LOGIC PIO: PIO entry token 0x4e invalid
insmod: can't insert 'hwmon/f71882fg.ko': No such device
root@(none)$

Note: I've removed a WARN_ONCE from the patches for this experiment. 
Maybe I'll permanently remove.

Without the patches:

root@(none)$ insmod hwmon/f71882fg.ko
[   82.001270] Unable to handle kernel paging request at virtual address 
ffff7dfffee0002e
[   82.009194] Mem abort info:
[   82.011980]   ESR = 0x96000046
[   82.015025]   Exception class = DABT (current EL), IL = 32 bits
[   82.020934]   SET = 0, FnV = 0
[   82.023978]   EA = 0, S1PTW = 0
[   82.027108] Data abort info:
[   82.029975]   ISV = 0, ISS = 0x00000046
[   82.033800]   CM = 0, WnR = 1
[   82.036757] swapper pgtable: 4k pages, 48-bit VAs, pgdp = 
(____ptrval____)
[   82.043620] [ffff7dfffee0002e] pgd=000000000141c003, 
pud=000000000141d003, pmd=0000000000000000
[   82.052309] Internal error: Oops: 96000046 [#1] PREEMPT SMP
[   82.057867] Modules linked in: f71882fg(+)
[   82.061953] CPU: 10 PID: 2731 Comm: insmod Not tainted 
5.1.0-rc1-00003-g938306ea0c86-dirty #230
[   82.070637] Hardware name: Huawei Taishan 2280 /D05, BIOS Hisilicon 
D05 IT21 Nemo 2.0 RC0 04/18/2018
[   82.079754] pstate: 80000005 (Nzcv daif -PAN -UAO)
[   82.084538] pc : logic_outb+0x54/0xb8
[   82.088188] lr : f71882fg_find+0x64/0x390 [f71882fg]
[   82.093139] sp : ffff000026a1baa0
[   82.096439] x29: ffff000026a1baa0 x28: ffff000008b98b10
[   82.101738] x27: ffff000026a1bdf0 x26: 0000000000000100
[   82.107037] x25: ffff801fb5391530 x24: ffff000011420000
[   82.112335] x23: ffff801fb68a3700 x22: ffff000011291000
[   82.117634] x21: 000000000000002e x20: 0000000000000087
[   82.122932] x19: ffff000026a1bb44 x18: ffffffffffffffff
[   82.128230] x17: 0000000000000000 x16: 0000000000000000
[   82.133528] x15: ffff00001127d6c8 x14: 0000000000000000
[   82.138826] x13: 0000000000000000 x12: 0000000000000000
[   82.144124] x11: ffff801ffbf92c80 x10: 0000801fead0f000
[   82.149422] x9 : 0000000000000000 x8 : ffff841fa6403680
[   82.154721] x7 : 0000000000000000 x6 : 000000000000003f
[   82.160019] x5 : ffff000011291360 x4 : 0000000000000000
[   82.165318] x3 : 0000000000ffbffe x2 : eed92a7132399200
[   82.170616] x1 : ffff7dfffee0002e x0 : ffff7dfffee00000
[   82.175915] Process insmod (pid: 2731, stack limit = 0x(____ptrval____))
[   82.182601] Call trace:
[   82.185035]  logic_outb+0x54/0xb8
[   82.188338]  f71882fg_find+0x64/0x390 [f71882fg]
[   82.192943]  f71882fg_init+0x38/0xc70 [f71882fg]
[   82.197548]  do_one_initcall+0x5c/0x198
[   82.201372]  do_init_module+0x54/0x1b0
[   82.205107]  load_module+0x1dc4/0x2158
[   82.208843]  __se_sys_init_module+0x14c/0x1e8
[   82.213186]  __arm64_sys_init_module+0x18/0x20
[   82.217618]  el0_svc_common+0x5c/0x100
[   82.221353]  el0_svc_handler+0x2c/0x80
[   82.225089]  el0_svc+0x8/0xc
[   82.227957] Code: d2bfdc00 f2cfbfe0 f2ffffe0 8b000021 (39000034)
[   82.234038] ---[ end trace 57421eeb25dcc011 ]---
Segmentation fault
root@(none)$

@Guenter, sorry for citing a hwmon driver again as triggering a crash...

John


> Will
>
> .
>



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH v3 3/4] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions

[John Garry]

On 05/04/2019 19:06, Bjorn Helgaas wrote:
> On Fri, Apr 05, 2019 at 09:10:27AM +0100, John Garry wrote:
>> On 04/04/2019 19:58, Bjorn Helgaas wrote:
>>> On Thu, Apr 04, 2019 at 10:43:36AM -0700, Guenter Roeck wrote:
>>>> On Thu, Apr 04, 2019 at 05:52:35PM +0100, John Garry wrote:
>>>>>>> Note that the f71805f driver does not call
>>>>>>> request_{muxed_}region(), as it should.
>>>>>
>>>>>> ... which is the real problem, one that is not solved by this
>>>>>> patch. This may result in parallel and descructive accesses if
>>>>>> there is another device on the LPC bus, and another driver
>>>>>> accessing that device. Personally I'd rather have
>>>>>> request_muxed_region() added to the f71805f driver.
>>>>>
>>>>> Right, we should and will still fix f71805f. If you recall, I did
>>>>> have the f71805f fix in the v1 series, but you committed that it
>>>>> was orthogonal, so I decided to take it out of this work for now.
>>>>>
>>>>> And even if we fix up f71805f and other known drivers which don't
>>>>> call request_muxed_region(), we still need to police against these
>>>>> rogue accesses, which is what this patch attempts to do.
>>>>>
>>>> Do we ? I am personally not convinced that LPC accesses _have_ to
>>>> occur through PCI on any given system.
>>>
>>> On current systems, I suspect ISA/LPC devices are typically connected
>>> via a PCI-to-ISA/LPC bridge.  But AFAIK there's no actual requirement
>>> for that bridge, and there certainly *were* systems with ISA devices
>>> but no PCI at all.
>>>
>>> IMO, if you want to build ISA drivers on your arch, you need to make
>>> sure the inb() probing done by those drivers works like it does on
>>> x86.  If there's no device there, the inb() should return 0xff with no
>>> fuss and no crash.
>>
>> Right, and this is what I am attempting to do here.
>>
>> So today a call to request_muxed_region() can still succeed even if no IO
>> space mapped.
>>
>> As such, even well-behaved drivers like f71882fg can still crash the system,
>> as noted in RFC patch 1/4 ("resource: Request IO port regions from children
>> of ioport_resource").
>

Correction:

"As such, *on arm* even well-behaved drivers like f71882fg can still 
crash the system, ...

So, yes, x86 - which has native IO ports - would not have this issue.

> Maybe I'm missing something, but on x86, drivers like f71882fg do not
> crash the system because inb() *never* causes a crash.
>
> If you want to build that driver for ARM, I think you need to make
> sure that inb() on ARM also *never* causes a crash.  I don't think
> changing f71882fg and all the similar drivers is the right answer.
>

Right, so this is the intention of this patch.

However it would be still good to find a way to fail a request to claim 
an IO port region if none is accessible or mapped.

Thanks,
John

> Bjorn
>
> .
>

Re: [PATCH v3 3/4] lib: logic_pio: Reject accesses to unregistered CPU MMIO regions

[John Garry]

On 05/04/2019 19:06, Bjorn Helgaas wrote:
> On Fri, Apr 05, 2019 at 09:10:27AM +0100, John Garry wrote:
>> On 04/04/2019 19:58, Bjorn Helgaas wrote:
>>> On Thu, Apr 04, 2019 at 10:43:36AM -0700, Guenter Roeck wrote:
>>>> On Thu, Apr 04, 2019 at 05:52:35PM +0100, John Garry wrote:
>>>>>>> Note that the f71805f driver does not call
>>>>>>> request_{muxed_}region(), as it should.
>>>>>
>>>>>> ... which is the real problem, one that is not solved by this
>>>>>> patch. This may result in parallel and descructive accesses if
>>>>>> there is another device on the LPC bus, and another driver
>>>>>> accessing that device. Personally I'd rather have
>>>>>> request_muxed_region() added to the f71805f driver.
>>>>>
>>>>> Right, we should and will still fix f71805f. If you recall, I did
>>>>> have the f71805f fix in the v1 series, but you committed that it
>>>>> was orthogonal, so I decided to take it out of this work for now.
>>>>>
>>>>> And even if we fix up f71805f and other known drivers which don't
>>>>> call request_muxed_region(), we still need to police against these
>>>>> rogue accesses, which is what this patch attempts to do.
>>>>>
>>>> Do we ? I am personally not convinced that LPC accesses _have_ to
>>>> occur through PCI on any given system.
>>>
>>> On current systems, I suspect ISA/LPC devices are typically connected
>>> via a PCI-to-ISA/LPC bridge.  But AFAIK there's no actual requirement
>>> for that bridge, and there certainly *were* systems with ISA devices
>>> but no PCI at all.
>>>
>>> IMO, if you want to build ISA drivers on your arch, you need to make
>>> sure the inb() probing done by those drivers works like it does on
>>> x86.  If there's no device there, the inb() should return 0xff with no
>>> fuss and no crash.
>>
>> Right, and this is what I am attempting to do here.
>>
>> So today a call to request_muxed_region() can still succeed even if no IO
>> space mapped.
>>
>> As such, even well-behaved drivers like f71882fg can still crash the system,
>> as noted in RFC patch 1/4 ("resource: Request IO port regions from children
>> of ioport_resource").
>

Correction:

"As such, *on arm* even well-behaved drivers like f71882fg can still 
crash the system, ...

So, yes, x86 - which has native IO ports - would not have this issue.

> Maybe I'm missing something, but on x86, drivers like f71882fg do not
> crash the system because inb() *never* causes a crash.
>
> If you want to build that driver for ARM, I think you need to make
> sure that inb() on ARM also *never* causes a crash.  I don't think
> changing f71882fg and all the similar drivers is the right answer.
>

Right, so this is the intention of this patch.

However it would be still good to find a way to fail a request to claim 
an IO port region if none is accessible or mapped.

Thanks,
John

> Bjorn
>
> .
>



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH v3 4/4] lib: logic_pio: Fix up some prints

[John Garry]

A print in find_io_range() is for a hex number, but doesn't prefix "0x".
Add the prefix, as is the norm.

In the case of the print in logic_pio_trans_cpuaddr(), don't cast the
value to unsigned long long, and just print the resource_size_t type
directly.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 lib/logic_pio.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/lib/logic_pio.c b/lib/logic_pio.c
index 3d8d986e9dcb..475d469c1a16 100644
--- a/lib/logic_pio.c
+++ b/lib/logic_pio.c
@@ -126,7 +126,7 @@ static struct logic_pio_hwaddr *find_io_range(unsigned long pio)
 		if (in_range(pio, range->io_start, range->size))
 			return range;
 	}
-	pr_err("PIO entry token %lx invalid\n", pio);
+	pr_err("PIO entry token 0x%lx invalid\n", pio);
 	return NULL;
 }
 
@@ -186,8 +186,7 @@ unsigned long logic_pio_trans_cpuaddr(resource_size_t addr)
 		if (in_range(addr, range->hw_start, range->size))
 			return addr - range->hw_start + range->io_start;
 	}
-	pr_err("addr %llx not registered in io_range_list\n",
-	       (unsigned long long) addr);
+	pr_err("addr %pa not registered in io_range_list\n",  &addr);
 	return ~0UL;
 }
 
-- 
2.17.1

[PATCH v3 4/4] lib: logic_pio: Fix up some prints

[John Garry]

A print in find_io_range() is for a hex number, but doesn't prefix "0x".
Add the prefix, as is the norm.

In the case of the print in logic_pio_trans_cpuaddr(), don't cast the
value to unsigned long long, and just print the resource_size_t type
directly.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 lib/logic_pio.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/lib/logic_pio.c b/lib/logic_pio.c
index 3d8d986e9dcb..475d469c1a16 100644
--- a/lib/logic_pio.c
+++ b/lib/logic_pio.c
@@ -126,7 +126,7 @@ static struct logic_pio_hwaddr *find_io_range(unsigned long pio)
 		if (in_range(pio, range->io_start, range->size))
 			return range;
 	}
-	pr_err("PIO entry token %lx invalid\n", pio);
+	pr_err("PIO entry token 0x%lx invalid\n", pio);
 	return NULL;
 }
 
@@ -186,8 +186,7 @@ unsigned long logic_pio_trans_cpuaddr(resource_size_t addr)
 		if (in_range(addr, range->hw_start, range->size))
 			return addr - range->hw_start + range->io_start;
 	}
-	pr_err("addr %llx not registered in io_range_list\n",
-	       (unsigned long long) addr);
+	pr_err("addr %pa not registered in io_range_list\n",  &addr);
 	return ~0UL;
 }
 
-- 
2.17.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel