| Message ID | 1554393602-152448-4-git-send-email-john.garry@huawei.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix system crash for accessing unmapped IO port regions | expand |
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 >
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 >> > > . >
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
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
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 > > . >
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
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
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 > > . >
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 > > . >
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
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 > > .
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
>>>> >>>> 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 > > . >
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); \ }
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(-) -- 2.17.1