Re: [PATCH v3] powerpc/uprobes: Validation for prefixed instruction

Le 04/03/2021 à 11:13, Ravi Bangoria a écrit :

On 3/4/21 1:02 PM, Christophe Leroy wrote:

quoted

Le 04/03/2021 à 06:05, Ravi Bangoria a écrit :

quoted

As per ISA 3.1, prefixed instruction should not cross 64-byte
boundary. So don't allow Uprobe on such prefixed instruction.

There are two ways probed instruction is changed in mapped pages.
First, when Uprobe is activated, it searches for all the relevant
pages and replace instruction in them. In this case, if that probe
is on the 64-byte unaligned prefixed instruction, error out
directly. Second, when Uprobe is already active and user maps a
relevant page via mmap(), instruction is replaced via mmap() code
path. But because Uprobe is invalid, entire mmap() operation can
not be stopped. In this case just print an error and continue.

Signed-off-by: Ravi Bangoria <redacted>
---
v2: https://lore.kernel.org/r/20210204104703.273429-1-ravi.bangoria@linux.ibm.com (local)
v2->v3:
   - Drop restriction for Uprobe on suffix of prefixed instruction.
     It needs lot of code change including generic code but what
     we get in return is not worth it.

  arch/powerpc/kernel/uprobes.c | 8 ++++++++
  1 file changed, 8 insertions(+)

diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c
index e8a63713e655..c400971ebe70 100644
--- a/arch/powerpc/kernel/uprobes.c
+++ b/arch/powerpc/kernel/uprobes.c

@@ -41,6 +41,14 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe,

      if (addr & 0x03)
          return -EINVAL;
+    if (!IS_ENABLED(CONFIG_PPC64) || !cpu_has_feature(CPU_FTR_ARCH_31))

cpu_has_feature(CPU_FTR_ARCH_31) should return 'false' when CONFIG_PPC64 is not enabled, no need 
to double check.

Ok.

I'm going to drop CONFIG_PPC64 check because it's not really
required as I replied to Naveen. So, I'll keep CPU_FTR_ARCH_31
check as is.

quoted

quoted

+        return 0;
+
+    if (ppc_inst_prefixed(auprobe->insn) && (addr & 0x3F) == 0x3C) {

Maybe 3C instead of 4F ? : (addr & 0x3C) == 0x3C

Didn't follow. It's not (addr & 0x3C), it's (addr & 0x3F).

Sorry I meant 3c instead of 3f (And usually we don't use capital letters for that).
The last two bits are supposed to always be 0, so it doesn't really matter, I just thought it would 
look better having the same value both sides of the test, ie (addr & 0x3c) == 0x3c.

quoted

What about

(addr & (SZ_64 - 4)) == SZ_64 - 4 to make it more explicit ?

Yes this is bit better. Though, it should be:

     (addr & (SZ_64 - 1)) == SZ_64 - 4

-1 or -4 should give the same results as instructions are always 32 bits aligned though.

Christophe