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 v2] powerpc/uprobes: Validation for 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: <20210204104703.273429-1-ravi.bangoria@linux.ibm.com>
 <20210206180604.GA8897@redhat.com>
Message-ID: <8fe4d62a-8873-d436-d20b-da3027103fe1@linux.ibm.com>
Date:   Mon, 8 Feb 2021 16:36:15 +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: <20210206180604.GA8897@redhat.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


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