2018-12-04 13:30:30

by Stefan Agner

[permalink] [raw]
Subject: [PATCH v2 1/2] pci: imx6: avoid dereferencing program counter from user mode

The custom fault handler is currently only meant to handle kernel
mode bus faults. Exit in case the abort happened in user mode.

Signed-off-by: Stefan Agner <[email protected]>
---
drivers/pci/controller/dwc/pci-imx6.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 69f86234f7c0..54a29e441303 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -270,8 +270,14 @@ 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 (user_mode(regs))
+ return 1;
+
+ instr = *(unsigned long *)pc;
+ reg = (instr >> 12) & 15;

/*
* If the instruction being executed was a read,
--
2.19.1



2018-12-04 13:28:45

by Stefan Agner

[permalink] [raw]
Subject: [PATCH v2 2/2] pci: imx6: support kernels built in Thumb-2 mode

Add a fault handler which handles immediate reads in Thumb-2
mode. Install the appropriate handler depending on which mode
the kernel has been built. This avoids an "Unhandled fault:
external abort on non-linefetch (0x1008) at 0xf0a80000"
during boot on a device with a PCIe switch connected.

Link: https://lore.kernel.org/linux-pci/[email protected]/
Signed-off-by: Stefan Agner <[email protected]>
---
Changes since v1:
- Added Thumb-2 32-bit instruction support (tested by inserting .w
instructions in arch/arm/include/asm/io.h)
- Avoid dereferencing if fault happened in user mode

drivers/pci/controller/dwc/pci-imx6.c | 59 ++++++++++++++++++++++++++-
1 file changed, 58 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
index 54a29e441303..a9bbbf176c4a 100644
--- a/drivers/pci/controller/dwc/pci-imx6.c
+++ b/drivers/pci/controller/dwc/pci-imx6.c
@@ -29,6 +29,7 @@
#include <linux/reset.h>
#include <linux/pm_domain.h>
#include <linux/pm_runtime.h>
+#include <asm/opcodes.h>

#include "pcie-designware.h"

@@ -305,6 +306,59 @@ static int imx6q_pcie_abort_handler(unsigned long addr,
return 1;
}

+static int imx6q_pcie_abort_handler_thumb2(unsigned long addr,
+ unsigned int fsr, struct pt_regs *regs)
+{
+ unsigned long pc = instruction_pointer(regs);
+ unsigned long instr;
+
+ if (user_mode(regs))
+ return 1;
+
+ instr = __mem_to_opcode_thumb32(*(unsigned long *)pc);
+
+ if (__opcode_is_thumb32(instr)) {
+ /* Load word/byte and halfword immediate offset */
+ if ((instr & 0xff100000UL) == 0xf8100000UL) {
+ int reg = (instr >> 12) & 0xf;
+ unsigned long val;
+
+ if ((instr & 0x00700000UL) == 0x00100000UL)
+ val = 0xff;
+ else if ((instr & 0x00700000UL) == 0x00300000UL)
+ val = 0xffff;
+ else
+ val = 0xffffffffUL;
+
+ regs->uregs[reg] = val;
+ regs->ARM_pc += 4;
+ return 0;
+ }
+ } else {
+ instr = __mem_to_opcode_thumb16(*(unsigned long *)pc);
+
+ /* Load word/byte and halfword immediate offset */
+ if (((instr & 0xe800) == 0x6800) ||
+ ((instr & 0xf800) == 0x8800)) {
+ int reg = instr & 0x7;
+ unsigned long val;
+
+ if (instr & 0x1000)
+ val = 0xff;
+ else if (instr & 0x8000)
+ val = 0xffff;
+ else
+ val = 0xffffffffUL;
+
+ regs->uregs[reg] = val;
+ regs->ARM_pc += 2;
+ return 0;
+ }
+ }
+
+ return 1;
+}
+
static int imx6_pcie_attach_pd(struct device *dev)
{
struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
@@ -1075,6 +1129,8 @@ static struct platform_driver imx6_pcie_driver = {

static int __init imx6_pcie_init(void)
{
+ bool thumb2 = IS_ENABLED(CONFIG_THUMB2_KERNEL);
+
/*
* Since probe() can be deferred we need to make sure that
* hook_fault_code is not called after __init memory is freed
@@ -1082,7 +1138,8 @@ static int __init imx6_pcie_init(void)
* we can install the handler here without risking it
* accessing some uninitialized driver state.
*/
- hook_fault_code(8, imx6q_pcie_abort_handler, SIGBUS, 0,
+ hook_fault_code(8, thumb2 ? imx6q_pcie_abort_handler_thumb2 :
+ imx6q_pcie_abort_handler, SIGBUS, 0,
"external abort on non-linefetch");

return platform_driver_register(&imx6_pcie_driver);
--
2.19.1


2019-02-08 12:15:33

by Lorenzo Pieralisi

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] pci: imx6: avoid dereferencing program counter from user mode

On Tue, Dec 04, 2018 at 02:27:32PM +0100, Stefan Agner wrote:
> The custom fault handler is currently only meant to handle kernel
> mode bus faults. Exit in case the abort happened in user mode.
>
> Signed-off-by: Stefan Agner <[email protected]>
> ---
> drivers/pci/controller/dwc/pci-imx6.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)

If this series is still aimed at mainline I need Lucas' ACK to
merge it.

Lorenzo

> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 69f86234f7c0..54a29e441303 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -270,8 +270,14 @@ 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 (user_mode(regs))
> + return 1;
> +
> + instr = *(unsigned long *)pc;
> + reg = (instr >> 12) & 15;
>
> /*
> * If the instruction being executed was a read,
> --
> 2.19.1
>

2019-02-11 20:29:14

by Lucas Stach

[permalink] [raw]
Subject: Re: [PATCH v2 1/2] pci: imx6: avoid dereferencing program counter from user mode

Am Dienstag, den 04.12.2018, 14:27 +0100 schrieb Stefan Agner:
> The custom fault handler is currently only meant to handle kernel
> mode bus faults. Exit in case the abort happened in user mode.
>
> Signed-off-by: Stefan Agner <[email protected]>

Reviewed-by: Lucas Stach <[email protected]>

> ---
>  drivers/pci/controller/dwc/pci-imx6.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 69f86234f7c0..54a29e441303 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -270,8 +270,14 @@ 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 (user_mode(regs))
> + return 1;
> +
> + instr = *(unsigned long *)pc;
> + reg = (instr >> 12) & 15;
>  
>   /*
>    * If the instruction being executed was a read,

2019-02-11 20:29:28

by Lucas Stach

[permalink] [raw]
Subject: Re: [PATCH v2 2/2] pci: imx6: support kernels built in Thumb-2 mode

Am Dienstag, den 04.12.2018, 14:27 +0100 schrieb Stefan Agner:
> Add a fault handler which handles immediate reads in Thumb-2
> mode. Install the appropriate handler depending on which mode
> the kernel has been built. This avoids an "Unhandled fault:
> external abort on non-linefetch (0x1008) at 0xf0a80000"
> during boot on a device with a PCIe switch connected.
>
> Link: https://lore.kernel.org/linux-pci/[email protected]/
> Signed-off-by: Stefan Agner <[email protected]>

Acked-by: Lucas Stach <[email protected]>

> ---
> Changes since v1:
> - Added Thumb-2 32-bit instruction support (tested by inserting .w
>   instructions in arch/arm/include/asm/io.h)
> - Avoid dereferencing if fault happened in user mode
>
>  drivers/pci/controller/dwc/pci-imx6.c | 59 ++++++++++++++++++++++++++-
>  1 file changed, 58 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/pci/controller/dwc/pci-imx6.c b/drivers/pci/controller/dwc/pci-imx6.c
> index 54a29e441303..a9bbbf176c4a 100644
> --- a/drivers/pci/controller/dwc/pci-imx6.c
> +++ b/drivers/pci/controller/dwc/pci-imx6.c
> @@ -29,6 +29,7 @@
>  #include <linux/reset.h>
>  #include <linux/pm_domain.h>
>  #include <linux/pm_runtime.h>
> +#include <asm/opcodes.h>
>  
>  #include "pcie-designware.h"
>  
> @@ -305,6 +306,59 @@ static int imx6q_pcie_abort_handler(unsigned long addr,
>   return 1;
>  }
>  
> +static int imx6q_pcie_abort_handler_thumb2(unsigned long addr,
> + unsigned int fsr, struct pt_regs *regs)
> +{
> + unsigned long pc = instruction_pointer(regs);
> + unsigned long instr;
> +
> + if (user_mode(regs))
> + return 1;
> +
> + instr = __mem_to_opcode_thumb32(*(unsigned long *)pc);
> +
> + if (__opcode_is_thumb32(instr)) {
> + /* Load word/byte and halfword immediate offset */
> + if ((instr & 0xff100000UL) == 0xf8100000UL) {
> + int reg = (instr >> 12) & 0xf;
> + unsigned long val;
> +
> + if ((instr & 0x00700000UL) == 0x00100000UL)
> + val = 0xff;
> + else if ((instr & 0x00700000UL) == 0x00300000UL)
> + val = 0xffff;
> + else
> + val = 0xffffffffUL;
> +
> + regs->uregs[reg] = val;
> + regs->ARM_pc += 4;
> + return 0;
> + }
> + } else {
> + instr = __mem_to_opcode_thumb16(*(unsigned long *)pc);
> +
> + /* Load word/byte and halfword immediate offset */
> + if (((instr & 0xe800) == 0x6800) ||
> +     ((instr & 0xf800) == 0x8800)) {
> + int reg = instr & 0x7;
> + unsigned long val;
> +
> + if (instr & 0x1000)
> + val = 0xff;
> + else if (instr & 0x8000)
> + val = 0xffff;
> + else
> + val = 0xffffffffUL;
> +
> + regs->uregs[reg] = val;
> + regs->ARM_pc += 2;
> + return 0;
> + }
> + }
> +
> + return 1;
> +}
> +
>  static int imx6_pcie_attach_pd(struct device *dev)
>  {
>   struct imx6_pcie *imx6_pcie = dev_get_drvdata(dev);
> @@ -1075,6 +1129,8 @@ static struct platform_driver imx6_pcie_driver = {
>  
>  static int __init imx6_pcie_init(void)
>  {
> + bool thumb2 = IS_ENABLED(CONFIG_THUMB2_KERNEL);
> +
>   /*
>    * Since probe() can be deferred we need to make sure that
>    * hook_fault_code is not called after __init memory is freed
> @@ -1082,7 +1138,8 @@ static int __init imx6_pcie_init(void)
>    * we can install the handler here without risking it
>    * accessing some uninitialized driver state.
>    */
> - hook_fault_code(8, imx6q_pcie_abort_handler, SIGBUS, 0,
> + hook_fault_code(8, thumb2 ? imx6q_pcie_abort_handler_thumb2 :
> + imx6q_pcie_abort_handler, SIGBUS, 0,
>   "external abort on non-linefetch");
>  
>   return platform_driver_register(&imx6_pcie_driver);