* [PATCH v4] powerpc/uprobes: Validation for prefixed instruction
@ 2021-03-05 11:54 ` Ravi Bangoria
0 siblings, 0 replies; 12+ messages in thread
From: Ravi Bangoria @ 2021-03-05 11:54 UTC (permalink / raw)
To: mpe
Cc: ravi.bangoria, oleg, rostedt, paulus, jniethe5, naveen.n.rao,
sandipan, linuxppc-dev, linux-kernel, christophe.leroy
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>
---
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;
+ }
+
return 0;
}
--
2.27.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH v4] powerpc/uprobes: Validation for prefixed instruction
@ 2021-03-05 11:54 ` Ravi Bangoria
0 siblings, 0 replies; 12+ messages in thread
From: Ravi Bangoria @ 2021-03-05 11:54 UTC (permalink / raw)
To: mpe
Cc: ravi.bangoria, jniethe5, oleg, rostedt, linux-kernel, paulus,
sandipan, naveen.n.rao, linuxppc-dev
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>
---
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;
+ }
+
return 0;
}
--
2.27.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH v4] powerpc/uprobes: Validation for prefixed instruction
2021-03-05 11:54 ` Ravi Bangoria
@ 2021-03-08 4:33 ` Sandipan Das
-1 siblings, 0 replies; 12+ messages in thread
From: Sandipan Das @ 2021-03-08 4:33 UTC (permalink / raw)
To: Ravi Bangoria
Cc: mpe, oleg, rostedt, paulus, jniethe5, naveen.n.rao, linuxppc-dev,
linux-kernel, christophe.leroy
On 05/03/21 5:24 pm, 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>
> Acked-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
> 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.
>
Acked-by: Sandipan Das <sandipan@linux.ibm.com>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4] powerpc/uprobes: Validation for prefixed instruction
@ 2021-03-08 4:33 ` Sandipan Das
0 siblings, 0 replies; 12+ messages in thread
From: Sandipan Das @ 2021-03-08 4:33 UTC (permalink / raw)
To: Ravi Bangoria
Cc: oleg, rostedt, linux-kernel, paulus, jniethe5, naveen.n.rao,
linuxppc-dev
On 05/03/21 5:24 pm, 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>
> Acked-by: Naveen N. Rao <naveen.n.rao@linux.vnet.ibm.com>
> ---
> 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.
>
Acked-by: Sandipan Das <sandipan@linux.ibm.com>
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4] powerpc/uprobes: Validation for prefixed instruction
2021-03-05 11:54 ` Ravi Bangoria
@ 2021-03-09 9:54 ` Michael Ellerman
-1 siblings, 0 replies; 12+ messages in thread
From: Michael Ellerman @ 2021-03-09 9:54 UTC (permalink / raw)
To: Ravi Bangoria
Cc: ravi.bangoria, oleg, rostedt, paulus, jniethe5, naveen.n.rao,
sandipan, linuxppc-dev, linux-kernel, christophe.leroy
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?
> ---
> 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)
cheers
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4] powerpc/uprobes: Validation for prefixed instruction
@ 2021-03-09 9:54 ` Michael Ellerman
0 siblings, 0 replies; 12+ messages in thread
From: Michael Ellerman @ 2021-03-09 9:54 UTC (permalink / raw)
To: Ravi Bangoria
Cc: ravi.bangoria, jniethe5, oleg, rostedt, linux-kernel, paulus,
sandipan, naveen.n.rao, linuxppc-dev
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?
> ---
> 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)
cheers
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4] powerpc/uprobes: Validation for prefixed instruction
2021-03-09 9:54 ` Michael Ellerman
@ 2021-03-09 11:21 ` Naveen N. Rao
-1 siblings, 0 replies; 12+ messages in thread
From: Naveen N. Rao @ 2021-03-09 11:21 UTC (permalink / raw)
To: Michael Ellerman
Cc: Ravi Bangoria, oleg, rostedt, paulus, jniethe5, naveen.n.rao,
sandipan, linuxppc-dev, linux-kernel, christophe.leroy
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4] powerpc/uprobes: Validation for prefixed instruction
@ 2021-03-09 11:21 ` Naveen N. Rao
0 siblings, 0 replies; 12+ messages in thread
From: Naveen N. Rao @ 2021-03-09 11:21 UTC (permalink / raw)
To: Michael Ellerman
Cc: Ravi Bangoria, jniethe5, oleg, rostedt, linux-kernel, paulus,
sandipan, naveen.n.rao, linuxppc-dev
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
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4] powerpc/uprobes: Validation for prefixed instruction
2021-03-09 11:21 ` Naveen N. Rao
@ 2021-03-09 12:58 ` Ravi Bangoria
-1 siblings, 0 replies; 12+ messages in thread
From: Ravi Bangoria @ 2021-03-09 12:58 UTC (permalink / raw)
To: Naveen N. Rao, Michael Ellerman
Cc: oleg, rostedt, paulus, jniethe5, naveen.n.rao, sandipan,
linuxppc-dev, linux-kernel, christophe.leroy, Ravi Bangoria
On 3/9/21 4:51 PM, Naveen N. Rao wrote:
> 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")
True. IMO, It doesn't really need any Fixes tag.
>
>>
>>> ---
>>> 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 ;)
Sure. Will resend.
Ravi
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4] powerpc/uprobes: Validation for prefixed instruction
@ 2021-03-09 12:58 ` Ravi Bangoria
0 siblings, 0 replies; 12+ messages in thread
From: Ravi Bangoria @ 2021-03-09 12:58 UTC (permalink / raw)
To: Naveen N. Rao, Michael Ellerman
Cc: Ravi Bangoria, jniethe5, oleg, rostedt, linux-kernel, paulus,
sandipan, naveen.n.rao, linuxppc-dev
On 3/9/21 4:51 PM, Naveen N. Rao wrote:
> 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")
True. IMO, It doesn't really need any Fixes tag.
>
>>
>>> ---
>>> 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 ;)
Sure. Will resend.
Ravi
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4] powerpc/uprobes: Validation for prefixed instruction
2021-03-09 12:58 ` Ravi Bangoria
@ 2021-03-10 5:13 ` Michael Ellerman
-1 siblings, 0 replies; 12+ messages in thread
From: Michael Ellerman @ 2021-03-10 5:13 UTC (permalink / raw)
To: Ravi Bangoria, Naveen N. Rao
Cc: oleg, rostedt, paulus, jniethe5, naveen.n.rao, sandipan,
linuxppc-dev, linux-kernel, christophe.leroy, Ravi Bangoria
Ravi Bangoria <ravi.bangoria@linux.ibm.com> writes:
> On 3/9/21 4:51 PM, Naveen N. Rao wrote:
>> 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")
>
> True. IMO, It doesn't really need any Fixes tag.
Yep OK, I'm happy without a Fixes tag based on that explanation.
>>>> 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 ;)
>
> Sure. Will resend.
Thanks.
cheers
^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH v4] powerpc/uprobes: Validation for prefixed instruction
@ 2021-03-10 5:13 ` Michael Ellerman
0 siblings, 0 replies; 12+ messages in thread
From: Michael Ellerman @ 2021-03-10 5:13 UTC (permalink / raw)
To: Ravi Bangoria, Naveen N. Rao
Cc: Ravi Bangoria, jniethe5, oleg, rostedt, linux-kernel, paulus,
sandipan, naveen.n.rao, linuxppc-dev
Ravi Bangoria <ravi.bangoria@linux.ibm.com> writes:
> On 3/9/21 4:51 PM, Naveen N. Rao wrote:
>> 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")
>
> True. IMO, It doesn't really need any Fixes tag.
Yep OK, I'm happy without a Fixes tag based on that explanation.
>>>> 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 ;)
>
> Sure. Will resend.
Thanks.
cheers
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-03-10 5:15 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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
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.