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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 8 Feb 2021 16:36:15 +0530
From:   Ravi Bangoria <ravi.bangoria@...ux.ibm.com>
To:     Oleg Nesterov <oleg@...hat.com>
Cc:     mpe@...erman.id.au, rostedt@...dmis.org, paulus@...ba.org,
        jniethe5@...il.com, naveen.n.rao@...ux.ibm.com,
        sandipan@...ux.ibm.com, linuxppc-dev@...ts.ozlabs.org,
        linux-kernel@...r.kernel.org,
        Ravi Bangoria <ravi.bangoria@...ux.ibm.com>
Subject: Re: [PATCH v2] powerpc/uprobes: Validation for prefixed instruction

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