[PATCH v2] powerpc/uprobes: Validation for prefixed instruction

[PATCH v2] powerpc/uprobes: Validation for prefixed instruction

[Ravi Bangoria]

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@linux.ibm.com>
---
v1: http://lore.kernel.org/r/20210119091234.76317-1-ravi.bangoria@linux.ibm.com
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@in.ibm.com>
  */
 #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 "
+				   "unaligned prefixed instruction\n");
+		return -EINVAL;
+	}
+
+	suffix = prefix;
+	if (get_instr(mm, addr - 4, &prefix) < 0)
+		return -EINVAL;
+
+	inst = ppc_inst_prefix(prefix, suffix);
+	if (ppc_inst_prefixed(inst)) {
+		printk_ratelimited("Cannot register a uprobe on the second "
+				   "word of prefixed instruction\n");
+		return -EINVAL;
+	}
+	return 0;
+}
+#else
+static int validate_prefixed_instr(struct mm_struct *mm, unsigned long addr)
+{
+	return 0;
+}
+#endif
+
 /**
  * arch_uprobe_analyze_insn
  * @mm: the probed address space.
@@ -41,7 +105,7 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe,
 	if (addr & 0x03)
 		return -EINVAL;
 
-	return 0;
+	return validate_prefixed_instr(mm, addr);
 }
 
 /*
-- 
2.26.2

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

[Ravi Bangoria]

On 2/4/21 4:17 PM, 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.

@mpe,

arch_uprobe_analyze_insn() can return early if
cpu_has_feature(CPU_FTR_ARCH_31) is not set. But that will
miss out a rare scenario of user running binary with prefixed
instruction on p10 predecessors. Please let me know if I
should add cpu_has_feature(CPU_FTR_ARCH_31) or not.

- Ravi

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

[Naveen N. Rao]

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@linux.ibm.com>
> ---
> v1: http://lore.kernel.org/r/20210119091234.76317-1-ravi.bangoria@linux.ibm.com
> 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@in.ibm.com>
>   */
>  #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)

In the future, it might be worthwhile to add IS_PREFIX() as a macro 
similar to IS_MTMSRD() if there are more such uses.

Along with this, if you also add the below to the start of this 
function, you can get rid of the #ifdef:

	if (!IS_ENABLED(CONFIG_PPC64))
		return 0;


- Naveen

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

[Naveen N. Rao]

On 2021/02/04 04:19PM, Ravi Bangoria wrote:
> 
> 
> On 2/4/21 4:17 PM, 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.
> 
> @mpe,
> 
> arch_uprobe_analyze_insn() can return early if
> cpu_has_feature(CPU_FTR_ARCH_31) is not set. But that will
> miss out a rare scenario of user running binary with prefixed
> instruction on p10 predecessors. Please let me know if I
> should add cpu_has_feature(CPU_FTR_ARCH_31) or not.

The check you are adding is very specific to prefixed instructions, so 
it makes sense to add a cpu feature check for v3.1.

On older processors, those are invalid instructions like any other. The 
instruction emulation infrastructure will refuse to emulate it and the 
instruction will be single stepped.

- Naveen

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

[Naveen N. Rao]

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@linux.ibm.com>
> > ---
> > v1: http://lore.kernel.org/r/20210119091234.76317-1-ravi.bangoria@linux.ibm.com
> > 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@in.ibm.com>
> >   */
> >  #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.

- Naveen

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

[Oleg Nesterov]

On 02/04, Ravi Bangoria wrote:
>
> +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;

"vma" is not used, and I don't think you need FOLL_SPLIT_PMD.

Otherwise I can't really comment this ppc-specific change.

To be honest, I don't even understand why do we need this fix. Sure, the
breakpoint in the middle of 64-bit insn won't work, why do we care? The
user should know what does he do.

Not to mention we can't really trust get_user_pages() in that this page
can be modified by mm owner or debugger...

But I won't argue.

Oleg.

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

[Ravi Bangoria]

On 2/6/21 11:36 PM, Oleg Nesterov wrote:
> On 02/04, Ravi Bangoria wrote:
>>
>> +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;
> 
> "vma" is not used,

Ok.

> and I don't think you need FOLL_SPLIT_PMD.
Isn't it needed if the target page is hugepage?

> Otherwise I can't really comment this ppc-specific change.
> 
> To be honest, I don't even understand why do we need this fix. Sure, the
> breakpoint in the middle of 64-bit insn won't work, why do we care? The
> user should know what does he do.

That's a valid point. This patch is to protract user from doing
invalid thing.

Though, there is one minor scenario where this patch will help. If
the original prefixed instruction is 64 byte unaligned, and say
user probes it, Uprobe infrastructure will emulate such instruction
transparently without notifying user that the instruction is
improperly aligned.

> Not to mention we can't really trust get_user_pages() in that this page
> can be modified by mm owner or debugger...

As Naveen pointed out, there might be existing uprobe on the prefix
and this patch will fail to detect such scenario. So I'm thinking to
read the instruction directly from file backed page (like copy_insn),
in which case I won't use get_user_pages().

Thanks Oleg for the review!

Ravi

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

[Ravi Bangoria]

On 2/4/21 6:45 PM, Naveen N. Rao wrote:
> On 2021/02/04 04:19PM, Ravi Bangoria wrote:
>>
>>
>> On 2/4/21 4:17 PM, 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.
>>
>> @mpe,
>>
>> arch_uprobe_analyze_insn() can return early if
>> cpu_has_feature(CPU_FTR_ARCH_31) is not set. But that will
>> miss out a rare scenario of user running binary with prefixed
>> instruction on p10 predecessors. Please let me know if I
>> should add cpu_has_feature(CPU_FTR_ARCH_31) or not.
> 
> The check you are adding is very specific to prefixed instructions, so
> it makes sense to add a cpu feature check for v3.1.
> 
> On older processors, those are invalid instructions like any other. The
> instruction emulation infrastructure will refuse to emulate it and the
> instruction will be single stepped.

Sure will add it.

Ravi

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

[Ravi Bangoria]

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@linux.ibm.com>
>>> ---
>>> v1: http://lore.kernel.org/r/20210119091234.76317-1-ravi.bangoria@linux.ibm.com
>>> 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@in.ibm.com>
>>>    */
>>>   #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

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

[Ravi Bangoria]

On 2/4/21 6:38 PM, 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@linux.ibm.com>
>> ---
>> v1: http://lore.kernel.org/r/20210119091234.76317-1-ravi.bangoria@linux.ibm.com
>> 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@in.ibm.com>
>>    */
>>   #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)

Ok.

> 
> In the future, it might be worthwhile to add IS_PREFIX() as a macro
> similar to IS_MTMSRD() if there are more such uses.
> 
> Along with this, if you also add the below to the start of this
> function, you can get rid of the #ifdef:
> 
> 	if (!IS_ENABLED(CONFIG_PPC64))
> 		return 0;

Yeah this is better.

Thanks for the review, Naveen!
Ravi