Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v2] powerpc/uprobes: Validation for prefixed instruction
To:     "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com>
Cc:     mpe@ellerman.id.au, oleg@redhat.com, rostedt@goodmis.org,
        paulus@samba.org, jniethe5@gmail.com, naveen.n.rao@linux.ibm.com,
        sandipan@linux.ibm.com, linuxppc-dev@lists.ozlabs.org,
        linux-kernel@vger.kernel.org,
        Ravi Bangoria <ravi.bangoria@linux.ibm.com>
References: <20210204104703.273429-1-ravi.bangoria@linux.ibm.com>
 <20210204130821.GK210@DESKTOP-TDPLP67.localdomain>
 <20210204161211.GM210@DESKTOP-TDPLP67.localdomain>
From:   Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Message-ID: <a685dcd5-4e2a-dcdc-ae10-eeaf826db362@linux.ibm.com>
Date:   Mon, 8 Feb 2021 16:40:58 +0530
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.6.0
MIME-Version: 1.0
In-Reply-To: <20210204161211.GM210@DESKTOP-TDPLP67.localdomain>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


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