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;
From:   Ravi Bangoria <ravi.bangoria@linux.ibm.com>
Subject: Re: [PATCH] powerpc/uprobes: Don't allow probe on suffix of prefixed
 instruction
To:     Oleg Nesterov <oleg@redhat.com>
Cc:     mpe@ellerman.id.au, 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: <20210119091234.76317-1-ravi.bangoria@linux.ibm.com>
 <20210119172603.GA16696@redhat.com>
Message-ID: <a760db1e-e953-75be-3ad5-2efc7642db6b@linux.ibm.com>
Date:   Wed, 20 Jan 2021 16:48:21 +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: <20210119172603.GA16696@redhat.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


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