On Thu, May 14, 2020 at 3:46 PM Christophe Leroy
[off-list ref] wrote:

Le 06/05/2020 à 05:40, Jordan Niethe a écrit :

quoted

Introduce a probe_user_read_inst() function to use in cases where
probe_user_read() is used for getting an instruction. This will be more
useful for prefixed instructions.

Reviewed-by: Alistair Popple <redacted>
Signed-off-by: Jordan Niethe <redacted>
---
v6: - New to series
---
  arch/powerpc/include/asm/inst.h |  3 +++
  arch/powerpc/lib/Makefile       |  2 +-
  arch/powerpc/lib/inst.c         | 18 ++++++++++++++++++
  arch/powerpc/mm/fault.c         |  2 +-
  4 files changed, 23 insertions(+), 2 deletions(-)
  create mode 100644 arch/powerpc/lib/inst.c

diff --git a/arch/powerpc/include/asm/inst.h b/arch/powerpc/include/asm/inst.h
index 552e953bf04f..3e9a58420151 100644
--- a/arch/powerpc/include/asm/inst.h
+++ b/arch/powerpc/include/asm/inst.h

@@ -37,4 +37,7 @@ static inline bool ppc_inst_equal(struct ppc_inst x, struct ppc_inst y)
      return ppc_inst_val(x) == ppc_inst_val(y);
  }

+int probe_user_read_inst(struct ppc_inst *inst,
+                      struct ppc_inst *nip);
+
  #endif /* _ASM_INST_H */

diff --git a/arch/powerpc/lib/Makefile b/arch/powerpc/lib/Makefile
index b8de3be10eb4..546591848219 100644
--- a/arch/powerpc/lib/Makefile
+++ b/arch/powerpc/lib/Makefile

@@ -16,7 +16,7 @@ CFLAGS_code-patching.o += -DDISABLE_BRANCH_PROFILING
  CFLAGS_feature-fixups.o += -DDISABLE_BRANCH_PROFILING
  endif

-obj-y += alloc.o code-patching.o feature-fixups.o pmem.o
+obj-y += alloc.o code-patching.o feature-fixups.o pmem.o inst.o

  ifndef CONFIG_KASAN
  obj-y       +=      string.o memcmp_$(BITS).o

diff --git a/arch/powerpc/lib/inst.c b/arch/powerpc/lib/inst.c
new file mode 100644
index 000000000000..eaf786afad2b
--- /dev/null
+++ b/arch/powerpc/lib/inst.c

@@ -0,0 +1,18 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ *  Copyright 2020, IBM Corporation.
+ */
+
+#include <linux/uaccess.h>
+#include <asm/inst.h>
+
+int probe_user_read_inst(struct ppc_inst *inst,
+                      struct ppc_inst *nip)
+{
+     unsigned int val;
+     int err;
+
+     err = probe_user_read(&val, nip, sizeof(val));
+     *inst = ppc_inst(val);
+     return err;
+}

diff --git a/arch/powerpc/mm/fault.c b/arch/powerpc/mm/fault.c
index 4a50f125ec18..f3a943eae305 100644
--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c

@@ -281,7 +281,7 @@ static bool bad_stack_expansion(struct pt_regs *regs, unsigned long address,
                  access_ok(nip, sizeof(*nip))) {
                      struct ppc_inst inst;

-                     if (!probe_user_read(&inst, nip, sizeof(inst)))
+                     if (!probe_user_read_inst(&inst, (struct ppc_inst __user *)nip))

Shouldn't 'nip' become de 'struct ppc_inst __user *' instead of casting ?

quoted

                              return !store_updates_sp(inst);
                      *must_retry = true;
              }

Yeah it would make more sense to do it like this.

--- a/arch/powerpc/mm/fault.c
+++ b/arch/powerpc/mm/fault.c

@@ -256,7 +256,7 @@ static bool bad_stack_expansion(struct pt_regs

*regs, unsigned long address,
      * expand to 1MB without further checks.
      */
     if (address + 0x100000 < vma->vm_end) {
-        unsigned int __user *nip = (unsigned int __user *)regs->nip;
+        struct ppc_inst __user *nip = (struct ppc_inst __user *)regs->nip;
         /* get user regs even if this fault is in kernel mode */
         struct pt_regs *uregs = current->thread.regs;
         if (uregs == NULL)

@@ -281,7 +281,7 @@ static bool bad_stack_expansion(struct pt_regs

*regs, unsigned long address,
             access_ok(nip, sizeof(*nip))) {
             struct ppc_inst inst;

-            if (!probe_user_read_inst(&inst, (struct ppc_inst __user *)nip))
+            if (!probe_user_read_inst(&inst, nip))
                 return !store_updates_sp(inst);
             *must_retry = true;
         }
-- 
2.17.1

Christophe