[PATCH] powerpc/uprobes: Don't allow probe on suffix of prefixed instruction

[PATCH] powerpc/uprobes: Don't allow probe on suffix of prefixed instruction

[Ravi Bangoria]

Probe on 2nd word of a prefixed instruction is invalid scenario and
should be restricted.

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@linux.ibm.com>
---
 arch/powerpc/kernel/uprobes.c | 28 ++++++++++++++++++++++++++++
 include/linux/uprobes.h       |  1 +
 kernel/events/uprobes.c       |  8 ++++++++
 3 files changed, 37 insertions(+)

diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c
index e8a63713e655..c73d5a397164 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@in.ibm.com>
  */
 #include <linux/kernel.h>
+#include <linux/highmem.h>
 #include <linux/sched.h>
 #include <linux/ptrace.h>
 #include <linux/uprobes.h>
@@ -44,6 +45,33 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe,
 	return 0;
 }
 
+#ifdef CONFIG_PPC64
+int arch_uprobe_verify_opcode(struct page *page, unsigned long vaddr,
+			      uprobe_opcode_t opcode)
+{
+	uprobe_opcode_t prefix;
+	void *kaddr;
+	struct ppc_inst inst;
+
+	/* Don't check if vaddr is pointing to the beginning of page */
+	if (!(vaddr & ~PAGE_MASK))
+		return 0;
+
+	kaddr = kmap_atomic(page);
+	memcpy(&prefix, kaddr + ((vaddr - 4) & ~PAGE_MASK), UPROBE_SWBP_INSN_SIZE);
+	kunmap_atomic(kaddr);
+
+	inst = ppc_inst_prefix(prefix, opcode);
+
+	if (ppc_inst_prefixed(inst)) {
+		printk_ratelimited("Cannot register a uprobe on the second "
+				   "word of prefixed instruction\n");
+		return -1;
+	}
+	return 0;
+}
+#endif
+
 /*
  * arch_uprobe_pre_xol - prepare to execute out of line.
  * @auprobe: the probepoint information.
diff --git a/include/linux/uprobes.h b/include/linux/uprobes.h
index f46e0ca0169c..5a3b45878e13 100644
--- a/include/linux/uprobes.h
+++ b/include/linux/uprobes.h
@@ -128,6 +128,7 @@ extern bool uprobe_deny_signal(void);
 extern bool arch_uprobe_skip_sstep(struct arch_uprobe *aup, struct pt_regs *regs);
 extern void uprobe_clear_state(struct mm_struct *mm);
 extern int  arch_uprobe_analyze_insn(struct arch_uprobe *aup, struct mm_struct *mm, unsigned long addr);
+int arch_uprobe_verify_opcode(struct page *page, unsigned long vaddr, uprobe_opcode_t opcode);
 extern int  arch_uprobe_pre_xol(struct arch_uprobe *aup, struct pt_regs *regs);
 extern int  arch_uprobe_post_xol(struct arch_uprobe *aup, struct pt_regs *regs);
 extern bool arch_uprobe_xol_was_trapped(struct task_struct *tsk);
diff --git a/kernel/events/uprobes.c b/kernel/events/uprobes.c
index bf9edd8d75be..be02e6c26e3f 100644
--- a/kernel/events/uprobes.c
+++ b/kernel/events/uprobes.c
@@ -255,6 +255,12 @@ static void copy_to_page(struct page *page, unsigned long vaddr, const void *src
 	kunmap_atomic(kaddr);
 }
 
+int __weak arch_uprobe_verify_opcode(struct page *page, unsigned long vaddr,
+				     uprobe_opcode_t opcode)
+{
+	return 0;
+}
+
 static int verify_opcode(struct page *page, unsigned long vaddr, uprobe_opcode_t *new_opcode)
 {
 	uprobe_opcode_t old_opcode;
@@ -275,6 +281,8 @@ static int verify_opcode(struct page *page, unsigned long vaddr, uprobe_opcode_t
 	if (is_swbp_insn(new_opcode)) {
 		if (is_swbp)		/* register: already installed? */
 			return 0;
+		if (arch_uprobe_verify_opcode(page, vaddr, old_opcode))
+			return -EINVAL;
 	} else {
 		if (!is_swbp)		/* unregister: was it changed by us? */
 			return 0;
-- 
2.26.2

Re: [PATCH] powerpc/uprobes: Don't allow probe on suffix of prefixed instruction

[Oleg Nesterov]

On 01/19, Ravi Bangoria wrote:
>
> Probe on 2nd word of a prefixed instruction is invalid scenario and
> should be restricted.

I don't understand this ppc-specific problem, but...

> +#ifdef CONFIG_PPC64
> +int arch_uprobe_verify_opcode(struct page *page, unsigned long vaddr,
> +			      uprobe_opcode_t opcode)
> +{
> +	uprobe_opcode_t prefix;
> +	void *kaddr;
> +	struct ppc_inst inst;
> +
> +	/* Don't check if vaddr is pointing to the beginning of page */
> +	if (!(vaddr & ~PAGE_MASK))
> +		return 0;

So the fix is incomplete? Or insn at the start of page can't be prefixed?

> +int __weak arch_uprobe_verify_opcode(struct page *page, unsigned long vaddr,
> +				     uprobe_opcode_t opcode)
> +{
> +	return 0;
> +}
> +
>  static int verify_opcode(struct page *page, unsigned long vaddr, uprobe_opcode_t *new_opcode)
>  {
>  	uprobe_opcode_t old_opcode;
> @@ -275,6 +281,8 @@ static int verify_opcode(struct page *page, unsigned long vaddr, uprobe_opcode_t
>  	if (is_swbp_insn(new_opcode)) {
>  		if (is_swbp)		/* register: already installed? */
>  			return 0;
> +		if (arch_uprobe_verify_opcode(page, vaddr, old_opcode))
> +			return -EINVAL;

Well, this doesn't look good...

To me it would be better to change the prepare_uprobe() path to copy
the potential prefix into uprobe->arch and check ppc_inst_prefixed()
in arch_uprobe_analyze_insn(). What do you think?

Oleg.

Re: [PATCH] powerpc/uprobes: Don't allow probe on suffix of prefixed instruction

[Ravi Bangoria]

On 1/19/21 10:56 PM, Oleg Nesterov wrote:
> On 01/19, Ravi Bangoria wrote:
>>
>> Probe on 2nd word of a prefixed instruction is invalid scenario and
>> should be restricted.
> 
> I don't understand this ppc-specific problem, but...

So far (upto Power9), instruction size was fixed - 4 bytes. But Power10
introduced a prefixed instruction which consist of 8 bytes, where first
4 bytes is prefix and remaining is suffix.

This patch checks whether the Uprobe is on the 2nd word (suffix) of a
prefixed instruction. If so, consider it as invalid Uprobe.

> 
>> +#ifdef CONFIG_PPC64
>> +int arch_uprobe_verify_opcode(struct page *page, unsigned long vaddr,
>> +			      uprobe_opcode_t opcode)
>> +{
>> +	uprobe_opcode_t prefix;
>> +	void *kaddr;
>> +	struct ppc_inst inst;
>> +
>> +	/* Don't check if vaddr is pointing to the beginning of page */
>> +	if (!(vaddr & ~PAGE_MASK))
>> +		return 0;
> 
> So the fix is incomplete? Or insn at the start of page can't be prefixed?

Prefixed instruction can not cross 64 byte boundary. If it does, kernel
generates SIGBUS. Considering all powerpc supported page sizes to be
multiple of 64 bytes, there will never be a scenario where prefix and
suffix will be on different pages. i.e. a beginning of the page should
never be a suffix.

> 
>> +int __weak arch_uprobe_verify_opcode(struct page *page, unsigned long vaddr,
>> +				     uprobe_opcode_t opcode)
>> +{
>> +	return 0;
>> +}
>> +
>>   static int verify_opcode(struct page *page, unsigned long vaddr, uprobe_opcode_t *new_opcode)
>>   {
>>   	uprobe_opcode_t old_opcode;
>> @@ -275,6 +281,8 @@ static int verify_opcode(struct page *page, unsigned long vaddr, uprobe_opcode_t
>>   	if (is_swbp_insn(new_opcode)) {
>>   		if (is_swbp)		/* register: already installed? */
>>   			return 0;
>> +		if (arch_uprobe_verify_opcode(page, vaddr, old_opcode))
>> +			return -EINVAL;
> 
> Well, this doesn't look good...
> 
> To me it would be better to change the prepare_uprobe() path to copy
> the potential prefix into uprobe->arch and check ppc_inst_prefixed()
> in arch_uprobe_analyze_insn(). What do you think?

Agreed. The only reason I was checking via verify_opcode() is to make the
code more simpler. If I need to check via prepare_uprobe(), I'll need to
abuse uprobe->offset by setting it to uprobe->offset - 4 to read previous
4 bytes of current instruction. Which, IMHO, is not that straightforward
with current implementation of prepare_uprobe().

But while replying here, I'm thinking... I should be able to grab a page
using mm and vaddr, which are already available in arch_uprobe_analyze_insn().
With that, I should be able to do all this inside arch_uprobe_analyze_insn()
only. I'll try this and send v2 if that works.

Thanks for the review.
Ravi