From: Ravi Bangoria <ravi.bangoria@linux.ibm.com> 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> Subject: Re: [PATCH v2] powerpc/uprobes: Validation for prefixed instruction Date: Mon, 8 Feb 2021 16:36:15 +0530 [thread overview] Message-ID: <8fe4d62a-8873-d436-d20b-da3027103fe1@linux.ibm.com> (raw) In-Reply-To: <20210206180604.GA8897@redhat.com> 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
WARNING: multiple messages have this Message-ID (diff)
From: Ravi Bangoria <ravi.bangoria@linux.ibm.com> To: Oleg Nesterov <oleg@redhat.com> Cc: Ravi Bangoria <ravi.bangoria@linux.ibm.com>, linux-kernel@vger.kernel.org, rostedt@goodmis.org, paulus@samba.org, sandipan@linux.ibm.com, jniethe5@gmail.com, naveen.n.rao@linux.ibm.com, linuxppc-dev@lists.ozlabs.org Subject: Re: [PATCH v2] powerpc/uprobes: Validation for prefixed instruction Date: Mon, 8 Feb 2021 16:36:15 +0530 [thread overview] Message-ID: <8fe4d62a-8873-d436-d20b-da3027103fe1@linux.ibm.com> (raw) In-Reply-To: <20210206180604.GA8897@redhat.com> 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
next prev parent reply other threads:[~2021-02-08 11:19 UTC|newest] Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-02-04 10:47 [PATCH v2] powerpc/uprobes: Validation for prefixed instruction Ravi Bangoria 2021-02-04 10:47 ` Ravi Bangoria 2021-02-04 10:49 ` Ravi Bangoria 2021-02-04 10:49 ` Ravi Bangoria 2021-02-04 13:15 ` Naveen N. Rao 2021-02-04 13:15 ` Naveen N. Rao 2021-02-08 11:08 ` Ravi Bangoria 2021-02-08 11:08 ` Ravi Bangoria 2021-02-04 14:19 ` Ananth N Mavinakayanahalli 2021-02-04 13:08 ` Naveen N. Rao 2021-02-04 13:08 ` Naveen N. Rao 2021-02-04 16:12 ` Naveen N. Rao 2021-02-04 16:12 ` Naveen N. Rao 2021-02-08 11:10 ` Ravi Bangoria 2021-02-08 11:10 ` Ravi Bangoria 2021-02-08 11:13 ` Ravi Bangoria 2021-02-08 11:13 ` Ravi Bangoria 2021-02-06 18:06 ` Oleg Nesterov 2021-02-06 18:06 ` Oleg Nesterov 2021-02-08 11:06 ` Ravi Bangoria [this message] 2021-02-08 11:06 ` Ravi Bangoria
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=8fe4d62a-8873-d436-d20b-da3027103fe1@linux.ibm.com \ --to=ravi.bangoria@linux.ibm.com \ --cc=jniethe5@gmail.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linuxppc-dev@lists.ozlabs.org \ --cc=mpe@ellerman.id.au \ --cc=naveen.n.rao@linux.ibm.com \ --cc=oleg@redhat.com \ --cc=paulus@samba.org \ --cc=rostedt@goodmis.org \ --cc=sandipan@linux.ibm.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.