| Message ID | 20220131075235.787432-1-francesco.dolcini@toradex.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | [v1] PCI: imx6: Handle the abort from user-space | expand |
Hello Lorenzo, just a gently ping on this patch. Francesco On Mon, Jan 31, 2022 at 08:52:35AM +0100, Francesco Dolcini wrote: > From: Jason Liu <jason.hui.liu@nxp.com> > > The driver install one hook to handle the external abort, but issue > is that if the abort introduced from user space code, the following > code unsigned long instr = *(unsigned long *)pc; which will created > another data-abort(page domain fault) if CONFIG_CPU_SW_DOMAIN_PAN. > > The patch does not intent to use copy_from_user and then do the hack > due to the security consideration. In fact, we can just return and > report the external abort to user-space. > > Link: https://lore.kernel.org/all/20220128082920.591115-1-francesco.dolcini@toradex.com > Signed-off-by: Jason Liu <jason.hui.liu@nxp.com> > Reviewed-by: Richard Zhu <hongxing.zhu@nxp.com> > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com> > Acked-by: Lucas Stach <l.stach@pengutronix.de>
On Thu, Feb 10, 2022 at 09:00:50AM +0100, Francesco Dolcini wrote: > Hello Lorenzo, > just a gently ping on this patch. > > Francesco > > On Mon, Jan 31, 2022 at 08:52:35AM +0100, Francesco Dolcini wrote: > > From: Jason Liu <jason.hui.liu@nxp.com> > > > > The driver install one hook to handle the external abort, but issue > > is that if the abort introduced from user space code, the following > > code unsigned long instr = *(unsigned long *)pc; which will created > > another data-abort(page domain fault) if CONFIG_CPU_SW_DOMAIN_PAN. > > > > The patch does not intent to use copy_from_user and then do the hack > > due to the security consideration. In fact, we can just return and > > report the external abort to user-space. Apologies for the delay in replying. This commit log should be rewritten - it is not clear. Isn't this an issue for all PCI host controllers that install a fault hook ? Is this referring to accessing config space directly from user space ? Can you explain the triggering conditions a bit better please ? Thanks, Lorenzo > > Link: https://lore.kernel.org/all/20220128082920.591115-1-francesco.dolcini@toradex.com > > Signed-off-by: Jason Liu <jason.hui.liu@nxp.com> > > Reviewed-by: Richard Zhu <hongxing.zhu@nxp.com> > > Signed-off-by: Francesco Dolcini <francesco.dolcini@toradex.com> > > Acked-by: Lucas Stach <l.stach@pengutronix.de>
Stefan: I added you to this email thread, maybe you remember some details on this patch from your previous life. On Wed, Feb 23, 2022 at 01:00:59PM +0000, Lorenzo Pieralisi wrote: > On Thu, Feb 10, 2022 at 09:00:50AM +0100, Francesco Dolcini wrote: > > On Mon, Jan 31, 2022 at 08:52:35AM +0100, Francesco Dolcini wrote: > > > From: Jason Liu <jason.hui.liu@nxp.com> > > > > > > The driver install one hook to handle the external abort, but issue > > > is that if the abort introduced from user space code, the following > > > code unsigned long instr = *(unsigned long *)pc; which will created > > > another data-abort(page domain fault) if CONFIG_CPU_SW_DOMAIN_PAN. > > > > > > The patch does not intent to use copy_from_user and then do the hack > > > due to the security consideration. In fact, we can just return and > > > report the external abort to user-space. > > Isn't this an issue for all PCI host controllers that install a fault > hook ? I would say some, not all. DWC PCI keystone is likely the same, however from a quick investigation other PCI fault handler do not touch any user memory, so likely not affected by the same issue. > Is this referring to accessing config space directly from user space ? Yes, it is. > Can you explain the triggering conditions a bit better please ? Just a short preamble, I'm not 100% sure about the actual details, we have a patch identical (but independent) to this one in our kernel tree since years. I found this one in the NXP i.MX vendor kernel and decided to submit that since it was already reviewed by one of the driver maintainer, I sent it as an RFC [0] and after it was acked-by Lucas to me it was obvious to move forward. My understanding is that instead of returning all ones when we timeout reading from non-existing downstream devices we get an abort [1], if the read was triggered from user space (I guess reading directly the memory, using sysfs does not trigger the issue) we end up with the handler triggering another abort while trying to read the user memory. Maybe this is just considered a non-valid use case? If you NXP guys have more details please speak up, see LF-228. If we move forward probably a `Fixes: [1]` should be added to this patch. Just slightly related, today while playing around with PCIe to answer this email, after a suspend-resume cycle, I was able to trigger this issue, I guess is just the imx6q_pcie_abort_handler() returning 1. ``` [ 1969.800185] pci 0000:01:00.0: can't change power state from unknown to D0 (config space inaccessible) [ 1969.800542] pci 0000:02:02.0: can't change power state from unknown to D0 (config space inaccessible) [ 1969.800557] pci 0000:02:01.0: can't change power state from unknown to D0 (config space inaccessible) [ 1969.800821] pci 0000:02:03.0: can't change power state from unknown to D0 (config space inaccessible) [ 1969.842913] 8<--- cut here --- [ 1969.846398] Unhandled fault: external abort on non-linefetch (0x1808) at 0xe6a00070 [ 1969.854276] [e6a00070] *pgd=12256811, *pte=01f00243, *ppte=01f00013 [ 1969.860770] Internal error: : 1808 [#1] SMP ARM [ 1969.865337] Modules linked in: bnep imx_sdma coda_vpu v4l2_jpeg imx_vdoa dw_hdmi_ahb_audio evbug [ 1969.874229] CPU: 1 PID: 16466 Comm: kworker/u8:8 Not tainted 5.17.0-rc5-00011-g5c1ee569660d #11 [ 1969.882982] Hardware name: Freescale i.MX6 Quad/DualLite (Device Tree) [ 1969.889549] Workqueue: events_unbound async_run_entry_fn [ 1969.894916] PC is at pci_generic_config_write+0x58/0x8c [ 1969.900194] LR is at arm_heavy_mb+0x18/0x4c [ 1969.904415] pc : [<c06c7f90>] lr : [<c0118378>] psr: 60000093 [ 1969.910718] sp : c2147e08 ip : 00000908 fp : ffffe000 [ 1969.915974] r10: 00000000 r9 : 00000000 r8 : 40000013 [ 1969.921229] r7 : 00000010 r6 : 00000810 r5 : 00000000 r4 : e6a00070 [ 1969.927794] r3 : 00000810 r2 : 00000000 r1 : 00000730 r0 : e6a00070 [ 1969.934359] Flags: nZCv IRQs off FIQs on Mode SVC_32 ISA ARM Segment none [ 1969.941628] Control: 10c5387d Table: 1000404a DAC: 00000051 [ 1969.947405] Register r0 information: 0-page vmalloc region starting at 0xe6a00000 allocated at devm_pci_remap_cfgspac e+0x44/0x8c [ 1969.959055] Register r1 information: non-paged memory [ 1969.964144] Register r2 information: NULL pointer [ 1969.968880] Register r3 information: non-paged memory [ 1969.973965] Register r4 information: 0-page vmalloc region starting at 0xe6a00000 allocated at devm_pci_remap_cfgspac e+0x44/0x8c [ 1969.985601] Register r5 information: NULL pointer [ 1969.990338] Register r6 information: non-paged memory [ 1969.995424] Register r7 information: zero-size pointer [ 1970.000596] Register r8 information: non-paged memory [ 1970.005682] Register r9 information: NULL pointer [ 1970.010418] Register r10 information: NULL pointer [ 1970.015243] Register r11 information: non-paged memory [ 1970.020416] Register r12 information: non-paged memory [ 1970.025590] Process kworker/u8:8 (pid: 16466, stack limit = 0x50c451ad) [ 1970.032247] Stack: (0xc2147e08 to 0xc2148000) [ 1970.036640] 7e00: c06e9620 c2704060 00000810 c06e9638 00000810 c0f65f54 [ 1970.044865] 7e20: 00000001 00000000 c06c7c18 00000018 00000070 c06e9620 c2658c00 c06c7c3c [ 1970.053091] 7e40: 00000810 c2808800 c2808800 c2252cc0 00000010 c2808800 00000005 c06d1788 [ 1970.061317] 7e60: ffffe000 c06d0880 ffffffff c1709388 ffff97e4 c2808880 00000001 00000000 [ 1970.069541] 7e80: c2808800 00000005 00000000 c06d5298 c06d5214 c190af20 c2808880 c17093d0 [ 1970.077765] 7ea0: c1f23ad4 c088e0a4 c280895c 00000001 c190a200 c17093d0 c200f005 c2808880 [ 1970.085989] 7ec0: 00000010 00000000 c200f000 00000000 c190a200 c17093d0 c200f005 c088f6e8 [ 1970.094213] 7ee0: c2808880 c1f8b818 c2006e00 c088fcbc c35b7090 c17159f0 c2006e00 c0155084 [ 1970.102438] 7f00: c35b7090 c274cb80 c2006e00 c01481c4 00000001 00000000 c0148118 00000001 [ 1970.110662] 7f20: 00000001 c1709388 c1916f98 c1d81690 00000000 c13b76c8 00000000 c1709388 [ 1970.118886] 7f40: c2006e3c c274cb80 c2006e00 c274cb98 c2006e3c c1705d00 00000088 c2146000 [ 1970.127110] 7f60: c2006e00 c014872c 00000000 c3a2ee40 c2146000 c01486fc c274cb80 c2184080 [ 1970.135333] 7f80: c2645ea4 c2c2e040 00000000 c015189c c3a2ee40 c0151774 00000000 00000000 [ 1970.143557] 7fa0: 00000000 00000000 00000000 c010011c 00000000 00000000 00000000 00000000 [ 1970.151781] 7fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 [ 1970.160006] 7fe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000 [ 1970.168228] pci_generic_config_write from dw_pcie_wr_other_conf+0x18/0x74 [ 1970.175163] dw_pcie_wr_other_conf from pci_bus_write_config_word+0x50/0x70 [ 1970.182177] pci_bus_write_config_word from pci_restore_state.part.0+0xd4/0x474 [ 1970.189547] pci_restore_state.part.0 from pci_pm_resume_noirq+0x84/0x124 [ 1970.196387] pci_pm_resume_noirq from dpm_run_callback+0x84/0x2f0 [ 1970.202532] dpm_run_callback from device_resume_noirq+0xc4/0x244 [ 1970.208670] device_resume_noirq from async_resume_noirq+0x18/0x4c [ 1970.214894] async_resume_noirq from async_run_entry_fn+0x20/0xb4 [ 1970.221031] async_run_entry_fn from process_one_work+0x298/0x7d0 [ 1970.227175] process_one_work from worker_thread+0x30/0x510 [ 1970.232790] worker_thread from kthread+0x128/0x14c [ 1970.237711] kthread from ret_from_fork+0x14/0x38 [ 1970.242453] Exception stack(0xc2147fb0 to 0xc2147ff8) [ 1970.247539] 7fa0: 00000000 00000000 00000000 00000000 [ 1970.255763] 7fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000 [ 1970.263986] 7fe0: 00000000 00000000 00000000 00000000 00000013 00000000 [ 1970.270642] Code: ee075f9a ebe940f5 e59d3010 e6ff3073 (e1c430b0) [ 1970.276775] ---[ end trace 0000000000000000 ]--- ``` Francesco [0] https://lore.kernel.org/all/20220128082920.591115-1-francesco.dolcini@toradex.com/ [1] 415b6185c541 ("PCI: imx6: Fix config read timeout handling")
diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c index 6974bd5aa116..c47a05332a94 100644 --- a/drivers/pci/controller/dwc/pci-imx6.c +++ b/drivers/pci/controller/dwc/pci-imx6.c @@ -22,6 +22,7 @@ #include <linux/of_address.h> #include <linux/pci.h> #include <linux/platform_device.h> +#include <linux/ptrace.h> #include <linux/regmap.h> #include <linux/regulator/consumer.h> #include <linux/resource.h> @@ -297,8 +298,15 @@ static int imx6q_pcie_abort_handler(unsigned long addr, unsigned int fsr, struct pt_regs *regs) { unsigned long pc = instruction_pointer(regs); - unsigned long instr = *(unsigned long *)pc; - int reg = (instr >> 12) & 15; + unsigned long instr; + int reg; + + /* if the abort from user-space, just return and report it */ + if (user_mode(regs)) + return 1; + + instr = *(unsigned long *)pc; + reg = (instr >> 12) & 15; /* * If the instruction being executed was a read,