| Message ID | 1592457029-18547-4-git-send-email-amit.kachhap@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: add Armv8.6 pointer authentication | expand |
On Thu, Jun 18, 2020 at 10:40:29AM +0530, Amit Daniel Kachhap wrote: > This patch disables the probing of authenticate ptrauth instruction (AUT*) > which falls under the hint instructions region. This is done to disallow > probe of authenticate instruction which may lead to ptrauth faults with the > addition of Armv8.6 enhanced ptrauth features. > > The corresponding append pac ptrauth instruction (PAC*) is not disabled > and they can still be probed. Seems sensible. Might be worth noting here why we think this is reasonable: AUT* instructions make no sense at function entry points, so most realistic probes would be unaffected by this change. Since stepping on older hardware is safe, we could make this conditional based on cpufeatures. It hardly seems worth it, though. > Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com> > --- > Change since v2: > * Modified this patch to consider the merged changes for whitelisting > of nops by commit 47d67e4d19184e ("arm64: insn: Report PAC and BTI"). > > arch/arm64/kernel/insn.c | 6 ------ > arch/arm64/kernel/probes/decode-insn.c | 2 +- > 2 files changed, 1 insertion(+), 7 deletions(-) > > diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c > index 684d871ae38d..9cd10edefc96 100644 > --- a/arch/arm64/kernel/insn.c > +++ b/arch/arm64/kernel/insn.c > @@ -60,16 +60,10 @@ bool __kprobes aarch64_insn_is_steppable_hint(u32 insn) > case AARCH64_INSN_HINT_XPACLRI: > case AARCH64_INSN_HINT_PACIA_1716: > case AARCH64_INSN_HINT_PACIB_1716: > - case AARCH64_INSN_HINT_AUTIA_1716: > - case AARCH64_INSN_HINT_AUTIB_1716: > case AARCH64_INSN_HINT_PACIAZ: > case AARCH64_INSN_HINT_PACIASP: > case AARCH64_INSN_HINT_PACIBZ: > case AARCH64_INSN_HINT_PACIBSP: > - case AARCH64_INSN_HINT_AUTIAZ: > - case AARCH64_INSN_HINT_AUTIASP: > - case AARCH64_INSN_HINT_AUTIBZ: > - case AARCH64_INSN_HINT_AUTIBSP: > case AARCH64_INSN_HINT_BTI: > case AARCH64_INSN_HINT_BTIC: > case AARCH64_INSN_HINT_BTIJ: > diff --git a/arch/arm64/kernel/probes/decode-insn.c b/arch/arm64/kernel/probes/decode-insn.c > index 263d5fba4c8a..c26c638b260e 100644 > --- a/arch/arm64/kernel/probes/decode-insn.c > +++ b/arch/arm64/kernel/probes/decode-insn.c > @@ -42,7 +42,7 @@ static bool __kprobes aarch64_insn_is_steppable(u32 insn) > != AARCH64_INSN_SPCLREG_DAIF; > > /* > - * The HINT instruction is is problematic when single-stepping, > + * The HINT instruction is problematic when single-stepping, Nit: doesn't matter too much, but ideally this should be a separate patch (or just don't bother). Cheers ---Dave
On Mon, Jun 22, 2020 at 03:40:26PM +0100, Dave Martin wrote: > Since stepping on older hardware is safe, we could make this conditional > based on cpufeatures. It hardly seems worth it, though. It might be worth it if we had a data structure that mapped the HINTs onto the supported features in the system, but that's definitely a separate thing.
On Mon, Jun 22, 2020 at 03:40:26PM +0100, Dave Martin wrote: > On Thu, Jun 18, 2020 at 10:40:29AM +0530, Amit Daniel Kachhap wrote: > > This patch disables the probing of authenticate ptrauth instruction (AUT*) > > which falls under the hint instructions region. This is done to disallow > > probe of authenticate instruction which may lead to ptrauth faults with the > > addition of Armv8.6 enhanced ptrauth features. > > diff --git a/arch/arm64/kernel/probes/decode-insn.c b/arch/arm64/kernel/probes/decode-insn.c > > index 263d5fba4c8a..c26c638b260e 100644 > > --- a/arch/arm64/kernel/probes/decode-insn.c > > +++ b/arch/arm64/kernel/probes/decode-insn.c > > @@ -42,7 +42,7 @@ static bool __kprobes aarch64_insn_is_steppable(u32 insn) > > != AARCH64_INSN_SPCLREG_DAIF; > > > > /* > > - * The HINT instruction is is problematic when single-stepping, > > + * The HINT instruction is problematic when single-stepping, > > Nit: doesn't matter too much, but ideally this should be a separate > patch (or just don't bother). Agreed. I also think if we change this at all we should drop the comment entirely: it's somewhat misleading given it implies NOP is the only HINT space instruction that can be sfely stepped, and doesn't suggest *why* hints might be proboematic. Thanks, Mark.
On Mon, Jun 22, 2020 at 05:57:49PM +0100, Mark Rutland wrote: > On Mon, Jun 22, 2020 at 03:40:26PM +0100, Dave Martin wrote: > > On Thu, Jun 18, 2020 at 10:40:29AM +0530, Amit Daniel Kachhap wrote: > > > - * The HINT instruction is is problematic when single-stepping, > > > + * The HINT instruction is problematic when single-stepping, > > Nit: doesn't matter too much, but ideally this should be a separate > > patch (or just don't bother). > Agreed. I also think if we change this at all we should drop the comment > entirely: it's somewhat misleading given it implies NOP is the only HINT > space instruction that can be sfely stepped, and doesn't suggest *why* > hints might be proboematic. Given the rename of _is_nop() to _is_steppable_hint() we should probably push an explanation to _is_steppable_hint() rather than having something here but I do think it's worth having something more than just the plain code. I do agree that the current comment really doesn't say much though.
On Mon, Jun 22, 2020 at 06:13:46PM +0100, Mark Brown wrote: > On Mon, Jun 22, 2020 at 05:57:49PM +0100, Mark Rutland wrote: > > On Mon, Jun 22, 2020 at 03:40:26PM +0100, Dave Martin wrote: > > > On Thu, Jun 18, 2020 at 10:40:29AM +0530, Amit Daniel Kachhap wrote: > > > > > - * The HINT instruction is is problematic when single-stepping, > > > > + * The HINT instruction is problematic when single-stepping, > > > > Nit: doesn't matter too much, but ideally this should be a separate > > > patch (or just don't bother). > > > Agreed. I also think if we change this at all we should drop the comment > > entirely: it's somewhat misleading given it implies NOP is the only HINT > > space instruction that can be sfely stepped, and doesn't suggest *why* > > hints might be proboematic. > > Given the rename of _is_nop() to _is_steppable_hint() we should probably > push an explanation to _is_steppable_hint() rather than having something > here but I do think it's worth having something more than just the plain > code. I do agree that the current comment really doesn't say much > though. That's fair. It's clearly material for a separate patch, regardless! Mark.
On 6/22/20 8:10 PM, Dave Martin wrote: > On Thu, Jun 18, 2020 at 10:40:29AM +0530, Amit Daniel Kachhap wrote: >> This patch disables the probing of authenticate ptrauth instruction (AUT*) >> which falls under the hint instructions region. This is done to disallow >> probe of authenticate instruction which may lead to ptrauth faults with the >> addition of Armv8.6 enhanced ptrauth features. >> >> The corresponding append pac ptrauth instruction (PAC*) is not disabled >> and they can still be probed. > > Seems sensible. Might be worth noting here why we think this is > reasonable: AUT* instructions make no sense at function entry points, > so most realistic probes would be unaffected by this change. Ok sure it make sense to add these details. Thanks for pointing this out. > > Since stepping on older hardware is safe, we could make this conditional > based on cpufeatures. It hardly seems worth it, though. Yes agreed. > >> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com> >> --- >> >> /* >> - * The HINT instruction is is problematic when single-stepping, >> + * The HINT instruction is problematic when single-stepping, > > Nit: doesn't matter too much, but ideally this should be a separate > patch (or just don't bother). ok. > > Cheers > ---Dave >
On 6/22/20 11:16 PM, Mark Rutland wrote: > On Mon, Jun 22, 2020 at 06:13:46PM +0100, Mark Brown wrote: >> On Mon, Jun 22, 2020 at 05:57:49PM +0100, Mark Rutland wrote: >>> On Mon, Jun 22, 2020 at 03:40:26PM +0100, Dave Martin wrote: >>>> On Thu, Jun 18, 2020 at 10:40:29AM +0530, Amit Daniel Kachhap wrote: >> >>>>> - * The HINT instruction is is problematic when single-stepping, >>>>> + * The HINT instruction is problematic when single-stepping, >> >>>> Nit: doesn't matter too much, but ideally this should be a separate >>>> patch (or just don't bother). >> >>> Agreed. I also think if we change this at all we should drop the comment >>> entirely: it's somewhat misleading given it implies NOP is the only HINT >>> space instruction that can be sfely stepped, and doesn't suggest *why* >>> hints might be proboematic. >> >> Given the rename of _is_nop() to _is_steppable_hint() we should probably >> push an explanation to _is_steppable_hint() rather than having something >> here but I do think it's worth having something more than just the plain >> code. I do agree that the current comment really doesn't say much >> though. > > That's fair. It's clearly material for a separate patch, regardless! > > Mark. ok sure. I will add a comment here in a separate patch. Thanks, Amit >
diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c index 684d871ae38d..9cd10edefc96 100644 --- a/arch/arm64/kernel/insn.c +++ b/arch/arm64/kernel/insn.c @@ -60,16 +60,10 @@ bool __kprobes aarch64_insn_is_steppable_hint(u32 insn) case AARCH64_INSN_HINT_XPACLRI: case AARCH64_INSN_HINT_PACIA_1716: case AARCH64_INSN_HINT_PACIB_1716: - case AARCH64_INSN_HINT_AUTIA_1716: - case AARCH64_INSN_HINT_AUTIB_1716: case AARCH64_INSN_HINT_PACIAZ: case AARCH64_INSN_HINT_PACIASP: case AARCH64_INSN_HINT_PACIBZ: case AARCH64_INSN_HINT_PACIBSP: - case AARCH64_INSN_HINT_AUTIAZ: - case AARCH64_INSN_HINT_AUTIASP: - case AARCH64_INSN_HINT_AUTIBZ: - case AARCH64_INSN_HINT_AUTIBSP: case AARCH64_INSN_HINT_BTI: case AARCH64_INSN_HINT_BTIC: case AARCH64_INSN_HINT_BTIJ: diff --git a/arch/arm64/kernel/probes/decode-insn.c b/arch/arm64/kernel/probes/decode-insn.c index 263d5fba4c8a..c26c638b260e 100644 --- a/arch/arm64/kernel/probes/decode-insn.c +++ b/arch/arm64/kernel/probes/decode-insn.c @@ -42,7 +42,7 @@ static bool __kprobes aarch64_insn_is_steppable(u32 insn) != AARCH64_INSN_SPCLREG_DAIF; /* - * The HINT instruction is is problematic when single-stepping, + * The HINT instruction is problematic when single-stepping, * except for the NOP case. */ if (aarch64_insn_is_hint(insn))
This patch disables the probing of authenticate ptrauth instruction (AUT*) which falls under the hint instructions region. This is done to disallow probe of authenticate instruction which may lead to ptrauth faults with the addition of Armv8.6 enhanced ptrauth features. The corresponding append pac ptrauth instruction (PAC*) is not disabled and they can still be probed. Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com> --- Change since v2: * Modified this patch to consider the merged changes for whitelisting of nops by commit 47d67e4d19184e ("arm64: insn: Report PAC and BTI"). arch/arm64/kernel/insn.c | 6 ------ arch/arm64/kernel/probes/decode-insn.c | 2 +- 2 files changed, 1 insertion(+), 7 deletions(-)