Messages in this thread | | | From | Ravi Bangoria <> | Subject | Re: [PATCH v2] powerpc/uprobes: Validation for prefixed instruction | Date | Mon, 8 Feb 2021 16:36:15 +0530 |
| |
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
| |