Re: [PATCH v2] powerpc/uprobes: Validation for prefixed instruction
From: Ravi Bangoria
Date: Mon Feb 08 2021 - 06:26:34 EST
On 2/4/21 9:42 PM, Naveen N. Rao wrote:
On 2021/02/04 06:38PM, Naveen N. Rao wrote:
On 2021/02/04 04:17PM, Ravi Bangoria wrote:
Don't allow Uprobe on 2nd word of a prefixed instruction. As per
ISA 3.1, prefixed instruction should not cross 64-byte boundary.
So don't allow Uprobe on such prefixed instruction as well.
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 we notice
that probe is on the 2nd word of 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 <ravi.bangoria@xxxxxxxxxxxxx>
---
v1: http://lore.kernel.org/r/20210119091234.76317-1-ravi.bangoria@xxxxxxxxxxxxx
v1->v2:
- Instead of introducing new arch hook from verify_opcode(), use
existing hook arch_uprobe_analyze_insn().
- Add explicit check for prefixed instruction crossing 64-byte
boundary. If probe is on such instruction, throw an error.
arch/powerpc/kernel/uprobes.c | 66 ++++++++++++++++++++++++++++++++++-
1 file changed, 65 insertions(+), 1 deletion(-)
diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c
index e8a63713e655..485d19a2a31f 100644
--- a/arch/powerpc/kernel/uprobes.c
+++ b/arch/powerpc/kernel/uprobes.c
@@ -7,6 +7,7 @@
* Adapted from the x86 port by Ananth N Mavinakayanahalli <ananth@xxxxxxxxxx>
*/
#include <linux/kernel.h>
+#include <linux/highmem.h>
#include <linux/sched.h>
#include <linux/ptrace.h>
#include <linux/uprobes.h>
@@ -28,6 +29,69 @@ bool is_trap_insn(uprobe_opcode_t *insn)
return (is_trap(*insn));
}
+#ifdef CONFIG_PPC64
+static int get_instr(struct mm_struct *mm, unsigned long addr, u32 *instr)
+{
+ struct page *page;
+ struct vm_area_struct *vma;
+ void *kaddr;
+ unsigned int gup_flags = FOLL_FORCE | FOLL_SPLIT_PMD;
+
+ if (get_user_pages_remote(mm, addr, 1, gup_flags, &page, &vma, NULL) <= 0)
+ return -EINVAL;
+
+ kaddr = kmap_atomic(page);
+ *instr = *((u32 *)(kaddr + (addr & ~PAGE_MASK)));
+ kunmap_atomic(kaddr);
+ put_page(page);
+ return 0;
+}
+
+static int validate_prefixed_instr(struct mm_struct *mm, unsigned long addr)
+{
+ struct ppc_inst inst;
+ u32 prefix, suffix;
+
+ /*
+ * No need to check if addr is pointing to beginning of the
+ * page. Even if probe is on a suffix of page-unaligned
+ * prefixed instruction, hw will raise exception and kernel
+ * will send SIGBUS.
+ */
+ if (!(addr & ~PAGE_MASK))
+ return 0;
+
+ if (get_instr(mm, addr, &prefix) < 0)
+ return -EINVAL;
+ if (get_instr(mm, addr + 4, &suffix) < 0)
+ return -EINVAL;
+
+ inst = ppc_inst_prefix(prefix, suffix);
+ if (ppc_inst_prefixed(inst) && (addr & 0x3F) == 0x3C) {
+ printk_ratelimited("Cannot register a uprobe on 64 byte "
^^^^^^^^^^^^^^^^^^ pr_info_ratelimited()
It should be sufficient to check the primary opcode to determine if it
is a prefixed instruction. You don't have to read the suffix. I see that
we don't have a helper to do this currently, so you could do:
if (ppc_inst_primary_opcode(ppc_inst(prefix)) == 1)
Seeing the kprobes code, I realized that we have to check for another
scenario (Thanks, Jordan!). If this is the suffix of a prefix
instruction for which a uprobe has already been installed, then the
previous word will be a 'trap' instruction. You need to check if there
is a uprobe at the previous word, and if the original instruction there
was a prefix instruction.
Yes, this patch will fail to detect such scenario. I think I should
read the instruction directly from file, like what copy_insn() does.
With that, I'll get original instruction rather that 'trap'.
I'll think more along this line.
Ravi