| Message ID | 20210304050529.59391-1-ravi.bangoria@linux.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v3] powerpc/uprobes: Validation for prefixed instruction | expand |
| Related | show |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | Successfully applied on branch powerpc/merge (626a6c3d2e20da80aaa710104f34ea6037b28b33) |
| snowpatch_ozlabs/build-ppc64le | success | Build succeeded |
| snowpatch_ozlabs/build-ppc64be | success | Build succeeded |
| snowpatch_ozlabs/build-ppc64e | success | Build succeeded |
| snowpatch_ozlabs/build-pmac32 | success | Build succeeded |
| snowpatch_ozlabs/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 14 lines checked |
| snowpatch_ozlabs/needsstable | success | Patch has no Fixes tags |
On 2021/03/04 10:35AM, Ravi Bangoria wrote: > 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> > --- > v2: https://lore.kernel.org/r/20210204104703.273429-1-ravi.bangoria@linux.ibm.com > v2->v3: > - Drop restriction for Uprobe on suffix of prefixed instruction. > It needs lot of code change including generic code but what > we get in return is not worth it. > > arch/powerpc/kernel/uprobes.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c > index e8a63713e655..c400971ebe70 100644 > --- a/arch/powerpc/kernel/uprobes.c > +++ b/arch/powerpc/kernel/uprobes.c > @@ -41,6 +41,14 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, > if (addr & 0x03) > return -EINVAL; > > + if (!IS_ENABLED(CONFIG_PPC64) || !cpu_has_feature(CPU_FTR_ARCH_31)) > + return 0; Sorry, I missed this last time, but I think we can drop the above checks. ppc_inst_prefixed() already factors in the dependency on CONFIG_PPC64, and I don't think we need to confirm if we're running on a ISA V3.1 for the below check. With that: Acked-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com> > + > + if (ppc_inst_prefixed(auprobe->insn) && (addr & 0x3F) == 0x3C) { > + pr_info_ratelimited("Cannot register a uprobe on 64 byte unaligned prefixed instruction\n"); > + return -EINVAL; > + } > + - Naveen
Le 04/03/2021 à 06:05, Ravi Bangoria a écrit : > 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> > --- > v2: https://lore.kernel.org/r/20210204104703.273429-1-ravi.bangoria@linux.ibm.com > v2->v3: > - Drop restriction for Uprobe on suffix of prefixed instruction. > It needs lot of code change including generic code but what > we get in return is not worth it. > > arch/powerpc/kernel/uprobes.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c > index e8a63713e655..c400971ebe70 100644 > --- a/arch/powerpc/kernel/uprobes.c > +++ b/arch/powerpc/kernel/uprobes.c > @@ -41,6 +41,14 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, > if (addr & 0x03) > return -EINVAL; > > + if (!IS_ENABLED(CONFIG_PPC64) || !cpu_has_feature(CPU_FTR_ARCH_31)) cpu_has_feature(CPU_FTR_ARCH_31) should return 'false' when CONFIG_PPC64 is not enabled, no need to double check. > + return 0; > + > + if (ppc_inst_prefixed(auprobe->insn) && (addr & 0x3F) == 0x3C) { Maybe 3C instead of 4F ? : (addr & 0x3C) == 0x3C What about (addr & (SZ_64 - 4)) == SZ_64 - 4 to make it more explicit ? Or ALIGN(addr, SZ_64) != ALIGN(addr + 8, SZ_64) > + pr_info_ratelimited("Cannot register a uprobe on 64 byte unaligned prefixed instruction\n"); > + return -EINVAL; > + } > + > return 0; > } > > Christophe
>> @@ -41,6 +41,14 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, >> if (addr & 0x03) >> return -EINVAL; >> >> + if (!IS_ENABLED(CONFIG_PPC64) || !cpu_has_feature(CPU_FTR_ARCH_31)) >> + return 0; > > Sorry, I missed this last time, but I think we can drop the above > checks. ppc_inst_prefixed() already factors in the dependency on > CONFIG_PPC64, Yeah, makes sense. I initially added CONFIG_PPC64 check because I was using ppc_inst_prefix(x, y) macro which is not available for !CONFIG_PPC64. > and I don't think we need to confirm if we're running on a > ISA V3.1 for the below check. Prefixed instructions would be supported only when ARCH_31 is set. (Ignoring insane scenario where user probes on prefixed instruction on p10 predecessors). So I guess I still need ARCH_31 check? Ravi
On 3/4/21 1:02 PM, Christophe Leroy wrote: > > > Le 04/03/2021 à 06:05, Ravi Bangoria a écrit : >> 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> >> --- >> v2: https://lore.kernel.org/r/20210204104703.273429-1-ravi.bangoria@linux.ibm.com >> v2->v3: >> - Drop restriction for Uprobe on suffix of prefixed instruction. >> It needs lot of code change including generic code but what >> we get in return is not worth it. >> >> arch/powerpc/kernel/uprobes.c | 8 ++++++++ >> 1 file changed, 8 insertions(+) >> >> diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c >> index e8a63713e655..c400971ebe70 100644 >> --- a/arch/powerpc/kernel/uprobes.c >> +++ b/arch/powerpc/kernel/uprobes.c >> @@ -41,6 +41,14 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, >> if (addr & 0x03) >> return -EINVAL; >> + if (!IS_ENABLED(CONFIG_PPC64) || !cpu_has_feature(CPU_FTR_ARCH_31)) > > cpu_has_feature(CPU_FTR_ARCH_31) should return 'false' when CONFIG_PPC64 is not enabled, no need to double check. Ok. I'm going to drop CONFIG_PPC64 check because it's not really required as I replied to Naveen. So, I'll keep CPU_FTR_ARCH_31 check as is. > >> + return 0; >> + >> + if (ppc_inst_prefixed(auprobe->insn) && (addr & 0x3F) == 0x3C) { > > Maybe 3C instead of 4F ? : (addr & 0x3C) == 0x3C Didn't follow. It's not (addr & 0x3C), it's (addr & 0x3F). > > What about > > (addr & (SZ_64 - 4)) == SZ_64 - 4 to make it more explicit ? Yes this is bit better. Though, it should be: (addr & (SZ_64 - 1)) == SZ_64 - 4 Ravi
Le 04/03/2021 à 11:13, Ravi Bangoria a écrit : > > > On 3/4/21 1:02 PM, Christophe Leroy wrote: >> >> >> Le 04/03/2021 à 06:05, Ravi Bangoria a écrit : >>> 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> >>> --- >>> v2: https://lore.kernel.org/r/20210204104703.273429-1-ravi.bangoria@linux.ibm.com >>> v2->v3: >>> - Drop restriction for Uprobe on suffix of prefixed instruction. >>> It needs lot of code change including generic code but what >>> we get in return is not worth it. >>> >>> arch/powerpc/kernel/uprobes.c | 8 ++++++++ >>> 1 file changed, 8 insertions(+) >>> >>> diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c >>> index e8a63713e655..c400971ebe70 100644 >>> --- a/arch/powerpc/kernel/uprobes.c >>> +++ b/arch/powerpc/kernel/uprobes.c >>> @@ -41,6 +41,14 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, >>> if (addr & 0x03) >>> return -EINVAL; >>> + if (!IS_ENABLED(CONFIG_PPC64) || !cpu_has_feature(CPU_FTR_ARCH_31)) >> >> cpu_has_feature(CPU_FTR_ARCH_31) should return 'false' when CONFIG_PPC64 is not enabled, no need >> to double check. > > Ok. > > I'm going to drop CONFIG_PPC64 check because it's not really > required as I replied to Naveen. So, I'll keep CPU_FTR_ARCH_31 > check as is. > >> >>> + return 0; >>> + >>> + if (ppc_inst_prefixed(auprobe->insn) && (addr & 0x3F) == 0x3C) { >> >> Maybe 3C instead of 4F ? : (addr & 0x3C) == 0x3C > > Didn't follow. It's not (addr & 0x3C), it's (addr & 0x3F). Sorry I meant 3c instead of 3f (And usually we don't use capital letters for that). The last two bits are supposed to always be 0, so it doesn't really matter, I just thought it would look better having the same value both sides of the test, ie (addr & 0x3c) == 0x3c. > >> >> What about >> >> (addr & (SZ_64 - 4)) == SZ_64 - 4 to make it more explicit ? > > Yes this is bit better. Though, it should be: > > (addr & (SZ_64 - 1)) == SZ_64 - 4 -1 or -4 should give the same results as instructions are always 32 bits aligned though. Christophe
On 3/4/21 4:21 PM, Christophe Leroy wrote: > > > Le 04/03/2021 à 11:13, Ravi Bangoria a écrit : >> >> >> On 3/4/21 1:02 PM, Christophe Leroy wrote: >>> >>> >>> Le 04/03/2021 à 06:05, Ravi Bangoria a écrit : >>>> 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> >>>> --- >>>> v2: https://lore.kernel.org/r/20210204104703.273429-1-ravi.bangoria@linux.ibm.com >>>> v2->v3: >>>> - Drop restriction for Uprobe on suffix of prefixed instruction. >>>> It needs lot of code change including generic code but what >>>> we get in return is not worth it. >>>> >>>> arch/powerpc/kernel/uprobes.c | 8 ++++++++ >>>> 1 file changed, 8 insertions(+) >>>> >>>> diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c >>>> index e8a63713e655..c400971ebe70 100644 >>>> --- a/arch/powerpc/kernel/uprobes.c >>>> +++ b/arch/powerpc/kernel/uprobes.c >>>> @@ -41,6 +41,14 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, >>>> if (addr & 0x03) >>>> return -EINVAL; >>>> + if (!IS_ENABLED(CONFIG_PPC64) || !cpu_has_feature(CPU_FTR_ARCH_31)) >>> >>> cpu_has_feature(CPU_FTR_ARCH_31) should return 'false' when CONFIG_PPC64 is not enabled, no need to double check. >> >> Ok. >> >> I'm going to drop CONFIG_PPC64 check because it's not really >> required as I replied to Naveen. So, I'll keep CPU_FTR_ARCH_31 >> check as is. >> >>> >>>> + return 0; >>>> + >>>> + if (ppc_inst_prefixed(auprobe->insn) && (addr & 0x3F) == 0x3C) { >>> >>> Maybe 3C instead of 4F ? : (addr & 0x3C) == 0x3C >> >> Didn't follow. It's not (addr & 0x3C), it's (addr & 0x3F). > > Sorry I meant 3c instead of 3f (And usually we don't use capital letters for that). > The last two bits are supposed to always be 0, so it doesn't really matter, I just thought it would look better having the same value both sides of the test, ie (addr & 0x3c) == 0x3c. Ok yeah makes sense. Thanks. > >> >>> >>> What about >>> >>> (addr & (SZ_64 - 4)) == SZ_64 - 4 to make it more explicit ? >> >> Yes this is bit better. Though, it should be: >> >> (addr & (SZ_64 - 1)) == SZ_64 - 4 > > -1 or -4 should give the same results as instructions are always 32 bits aligned though. Got it. Ravi
diff --git a/arch/powerpc/kernel/uprobes.c b/arch/powerpc/kernel/uprobes.c index e8a63713e655..c400971ebe70 100644 --- a/arch/powerpc/kernel/uprobes.c +++ b/arch/powerpc/kernel/uprobes.c @@ -41,6 +41,14 @@ int arch_uprobe_analyze_insn(struct arch_uprobe *auprobe, if (addr & 0x03) return -EINVAL; + if (!IS_ENABLED(CONFIG_PPC64) || !cpu_has_feature(CPU_FTR_ARCH_31)) + return 0; + + if (ppc_inst_prefixed(auprobe->insn) && (addr & 0x3F) == 0x3C) { + pr_info_ratelimited("Cannot register a uprobe on 64 byte unaligned prefixed instruction\n"); + return -EINVAL; + } + return 0; }
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> --- v2: https://lore.kernel.org/r/20210204104703.273429-1-ravi.bangoria@linux.ibm.com v2->v3: - Drop restriction for Uprobe on suffix of prefixed instruction. It needs lot of code change including generic code but what we get in return is not worth it. arch/powerpc/kernel/uprobes.c | 8 ++++++++ 1 file changed, 8 insertions(+)