lkml.org 
[lkml]   [2021]   [Feb]   [8]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2] powerpc/uprobes: Validation for prefixed instruction
From
Date


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

\
 
 \ /
  Last update: 2021-02-08 12:31    [W:0.129 / U:0.040 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site