From: "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> To: Michael Ellerman <mpe@ellerman.id.au> Cc: Ravi Bangoria <ravi.bangoria@linux.ibm.com>, oleg@redhat.com, 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, christophe.leroy@csgroup.eu Subject: Re: [PATCH v4] powerpc/uprobes: Validation for prefixed instruction Date: Tue, 9 Mar 2021 16:51:15 +0530 [thread overview] Message-ID: <20210309112115.GG145@DESKTOP-TDPLP67.localdomain> (raw) In-Reply-To: <87ft14r6sa.fsf@mpe.ellerman.id.au> On 2021/03/09 08:54PM, Michael Ellerman wrote: > Ravi Bangoria <ravi.bangoria@linux.ibm.com> writes: > > As per ISA 3.1, prefixed instruction should not cross 64-byte > > boundary. So don't allow Uprobe on such prefixed instruction. > > > > There are two ways probed instruction is changed in mapped pages. > > First, when Uprobe is activated, it searches for all the relevant > > pages and replace instruction in them. In this case, if that probe > > is on the 64-byte unaligned prefixed instruction, error out > > directly. Second, when Uprobe is already active and user maps a > > relevant page via mmap(), instruction is replaced via mmap() code > > path. But because Uprobe is invalid, entire mmap() operation can > > not be stopped. In this case just print an error and continue. > > > > Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com> > > Acked-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> > > Do we have a Fixes: tag for this? Since this is an additional check we are adding, I don't think we should add a Fixes: tag. Nothing is broken per-se -- we're just adding more checks to catch simple mistakes. Also, like Oleg pointed out, there are still many other ways for users to shoot themselves in the foot with uprobes and prefixed instructions, if they so desire. However, if you still think we should add a Fixes: tag, we can perhaps use the below commit since I didn't see any specific commit adding support for prefixed instructions for uprobes: Fixes: 650b55b707fdfa ("powerpc: Add prefixed instructions to instruction data type") > > > --- > > v3: https://lore.kernel.org/r/20210304050529.59391-1-ravi.bangoria@linux.ibm.com > > v3->v4: > > - CONFIG_PPC64 check was not required, remove it. > > - Use SZ_ macros instead of hardcoded numbers. > > > > arch/powerpc/kernel/uprobes.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c > > index e8a63713e655..4cbfff6e94a3 100644 > > --- a/arch/powerpc/kernel/uprobes.c > > +++ b/arch/powerpc/kernel/uprobes.c > > @@ -41,6 +41,13 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, > > if (addr & 0x03) > > return -EINVAL; > > > > + if (cpu_has_feature(CPU_FTR_ARCH_31) && > > + ppc_inst_prefixed(auprobe->insn) && > > + (addr & (SZ_64 - 4)) == SZ_64 - 4) { > > + pr_info_ratelimited("Cannot register a uprobe on 64 byte unaligned prefixed instruction\n"); > > + return -EINVAL; > > I realise we already did the 0x03 check above, but I still think this > would be clearer simply as: > > (addr & 0x3f == 60) Indeed, I like the use of `60' there -- hex is overrated ;) - Naveen
WARNING: multiple messages have this Message-ID (diff)
From: "Naveen N. Rao" <naveen.n.rao@linux.vnet.ibm.com> To: Michael Ellerman <mpe@ellerman.id.au> Cc: Ravi Bangoria <ravi.bangoria@linux.ibm.com>, jniethe5@gmail.com, oleg@redhat.com, rostedt@goodmis.org, linux-kernel@vger.kernel.org, paulus@samba.org, sandipan@linux.ibm.com, naveen.n.rao@linux.ibm.com, linuxppc-dev@lists.ozlabs.org Subject: Re: [PATCH v4] powerpc/uprobes: Validation for prefixed instruction Date: Tue, 9 Mar 2021 16:51:15 +0530 [thread overview] Message-ID: <20210309112115.GG145@DESKTOP-TDPLP67.localdomain> (raw) In-Reply-To: <87ft14r6sa.fsf@mpe.ellerman.id.au> On 2021/03/09 08:54PM, Michael Ellerman wrote: > Ravi Bangoria <ravi.bangoria@linux.ibm.com> writes: > > As per ISA 3.1, prefixed instruction should not cross 64-byte > > boundary. So don't allow Uprobe on such prefixed instruction. > > > > There are two ways probed instruction is changed in mapped pages. > > First, when Uprobe is activated, it searches for all the relevant > > pages and replace instruction in them. In this case, if that probe > > is on the 64-byte unaligned prefixed instruction, error out > > directly. Second, when Uprobe is already active and user maps a > > relevant page via mmap(), instruction is replaced via mmap() code > > path. But because Uprobe is invalid, entire mmap() operation can > > not be stopped. In this case just print an error and continue. > > > > Signed-off-by: Ravi Bangoria <ravi.bangoria@linux.ibm.com> > > Acked-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> > > Do we have a Fixes: tag for this? Since this is an additional check we are adding, I don't think we should add a Fixes: tag. Nothing is broken per-se -- we're just adding more checks to catch simple mistakes. Also, like Oleg pointed out, there are still many other ways for users to shoot themselves in the foot with uprobes and prefixed instructions, if they so desire. However, if you still think we should add a Fixes: tag, we can perhaps use the below commit since I didn't see any specific commit adding support for prefixed instructions for uprobes: Fixes: 650b55b707fdfa ("powerpc: Add prefixed instructions to instruction data type") > > > --- > > v3: https://lore.kernel.org/r/20210304050529.59391-1-ravi.bangoria@linux.ibm.com > > v3->v4: > > - CONFIG_PPC64 check was not required, remove it. > > - Use SZ_ macros instead of hardcoded numbers. > > > > arch/powerpc/kernel/uprobes.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c > > index e8a63713e655..4cbfff6e94a3 100644 > > --- a/arch/powerpc/kernel/uprobes.c > > +++ b/arch/powerpc/kernel/uprobes.c > > @@ -41,6 +41,13 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, > > if (addr & 0x03) > > return -EINVAL; > > > > + if (cpu_has_feature(CPU_FTR_ARCH_31) && > > + ppc_inst_prefixed(auprobe->insn) && > > + (addr & (SZ_64 - 4)) == SZ_64 - 4) { > > + pr_info_ratelimited("Cannot register a uprobe on 64 byte unaligned prefixed instruction\n"); > > + return -EINVAL; > > I realise we already did the 0x03 check above, but I still think this > would be clearer simply as: > > (addr & 0x3f == 60) Indeed, I like the use of `60' there -- hex is overrated ;) - Naveen
next prev parent reply other threads:[~2021-03-09 11:23 UTC|newest] Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-03-05 11:54 [PATCH v4] powerpc/uprobes: Validation for prefixed instruction Ravi Bangoria 2021-03-05 11:54 ` Ravi Bangoria 2021-03-08 4:33 ` Sandipan Das 2021-03-08 4:33 ` Sandipan Das 2021-03-09 9:54 ` Michael Ellerman 2021-03-09 9:54 ` Michael Ellerman 2021-03-09 11:21 ` Naveen N. Rao [this message] 2021-03-09 11:21 ` Naveen N. Rao 2021-03-09 12:58 ` Ravi Bangoria 2021-03-09 12:58 ` Ravi Bangoria 2021-03-10 5:13 ` Michael Ellerman 2021-03-10 5:13 ` Michael Ellerman
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=20210309112115.GG145@DESKTOP-TDPLP67.localdomain \ --to=naveen.n.rao@linux.vnet.ibm.com \ --cc=christophe.leroy@csgroup.eu \ --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=ravi.bangoria@linux.ibm.com \ --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.