| Message ID | 1582117240-15330-3-git-send-email-amit.kachhap@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | arm64: add Armv8.6 pointer authentication | expand |
Hi Amit, On Wed, Feb 19, 2020 at 06:30:40PM +0530, Amit Daniel Kachhap wrote: > This patch disables the probing of authenticate ptrauth instruction > (AUTIASP) which falls under the hint instructions region. This is done > to disallow probe of authenticate instruction in the kernel which may > lead to ptrauth faults with the addition of Armv8.6 enhanced ptrauth > features. > > The corresponding append pac ptrauth instruction (PACIASP) is not disabled > and they can still be probed. > > Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com> > --- > arch/arm64/include/asm/insn.h | 13 +++++++------ > arch/arm64/kernel/insn.c | 1 + > arch/arm64/kernel/probes/decode-insn.c | 2 +- > 3 files changed, 9 insertions(+), 7 deletions(-) > > diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h > index bb313dd..2e01db0 100644 > --- a/arch/arm64/include/asm/insn.h > +++ b/arch/arm64/include/asm/insn.h > @@ -40,12 +40,13 @@ enum aarch64_insn_encoding_class { > }; > > enum aarch64_insn_hint_op { > - AARCH64_INSN_HINT_NOP = 0x0 << 5, > - AARCH64_INSN_HINT_YIELD = 0x1 << 5, > - AARCH64_INSN_HINT_WFE = 0x2 << 5, > - AARCH64_INSN_HINT_WFI = 0x3 << 5, > - AARCH64_INSN_HINT_SEV = 0x4 << 5, > - AARCH64_INSN_HINT_SEVL = 0x5 << 5, > + AARCH64_INSN_HINT_NOP = 0x0 << 5, > + AARCH64_INSN_HINT_YIELD = 0x1 << 5, > + AARCH64_INSN_HINT_WFE = 0x2 << 5, > + AARCH64_INSN_HINT_WFI = 0x3 << 5, > + AARCH64_INSN_HINT_SEV = 0x4 << 5, > + AARCH64_INSN_HINT_SEVL = 0x5 << 5, > + AARCH64_INSN_HINT_AUTIASP = (0x3 << 8) | (0x5 << 5), > }; > > enum aarch64_insn_imm_type { > diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c > index 4a9e773..87f7c8a 100644 > --- a/arch/arm64/kernel/insn.c > +++ b/arch/arm64/kernel/insn.c > @@ -63,6 +63,7 @@ bool __kprobes aarch64_insn_is_nop(u32 insn) > case AARCH64_INSN_HINT_WFI: > case AARCH64_INSN_HINT_SEV: > case AARCH64_INSN_HINT_SEVL: > + case AARCH64_INSN_HINT_AUTIASP: > return false; > default: > return true; I'm afraid that the existing code here is simply wrong, and this is adding to the mess. We have no idea what HINT space instructions will be in the future, so the only sensible implementations of aarch64_insn_is_nop() are something like: bool __kprobes aarch64_insn_is_nop(u32 insn) { return insn == aarch64_insn_gen_hint(AARCH64_INSN_HINT_NOP); } ... and if we want to check for other HINT instructions, they should be checked explicitly. Can you please change aarch64_insn_is_nop() as above? Generally the logic in aarch64_insn_is_steppable() needs to be reworked to a whitelist, but at least chagning aarch64_insn_is_nop() this way is closer to where we want to be. Thanks, Mark.
Hi, On 2/27/20 10:18 PM, Mark Rutland wrote: > Hi Amit, > > On Wed, Feb 19, 2020 at 06:30:40PM +0530, Amit Daniel Kachhap wrote: >> This patch disables the probing of authenticate ptrauth instruction >> (AUTIASP) which falls under the hint instructions region. This is done >> to disallow probe of authenticate instruction in the kernel which may >> lead to ptrauth faults with the addition of Armv8.6 enhanced ptrauth >> features. >> >> The corresponding append pac ptrauth instruction (PACIASP) is not disabled >> and they can still be probed. >> >> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com> >> --- >> arch/arm64/include/asm/insn.h | 13 +++++++------ >> arch/arm64/kernel/insn.c | 1 + >> arch/arm64/kernel/probes/decode-insn.c | 2 +- >> 3 files changed, 9 insertions(+), 7 deletions(-) >> >> diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h >> index bb313dd..2e01db0 100644 >> --- a/arch/arm64/include/asm/insn.h >> +++ b/arch/arm64/include/asm/insn.h >> @@ -40,12 +40,13 @@ enum aarch64_insn_encoding_class { >> }; >> >> enum aarch64_insn_hint_op { >> - AARCH64_INSN_HINT_NOP = 0x0 << 5, >> - AARCH64_INSN_HINT_YIELD = 0x1 << 5, >> - AARCH64_INSN_HINT_WFE = 0x2 << 5, >> - AARCH64_INSN_HINT_WFI = 0x3 << 5, >> - AARCH64_INSN_HINT_SEV = 0x4 << 5, >> - AARCH64_INSN_HINT_SEVL = 0x5 << 5, >> + AARCH64_INSN_HINT_NOP = 0x0 << 5, >> + AARCH64_INSN_HINT_YIELD = 0x1 << 5, >> + AARCH64_INSN_HINT_WFE = 0x2 << 5, >> + AARCH64_INSN_HINT_WFI = 0x3 << 5, >> + AARCH64_INSN_HINT_SEV = 0x4 << 5, >> + AARCH64_INSN_HINT_SEVL = 0x5 << 5, >> + AARCH64_INSN_HINT_AUTIASP = (0x3 << 8) | (0x5 << 5), >> }; >> >> enum aarch64_insn_imm_type { >> diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c >> index 4a9e773..87f7c8a 100644 >> --- a/arch/arm64/kernel/insn.c >> +++ b/arch/arm64/kernel/insn.c >> @@ -63,6 +63,7 @@ bool __kprobes aarch64_insn_is_nop(u32 insn) >> case AARCH64_INSN_HINT_WFI: >> case AARCH64_INSN_HINT_SEV: >> case AARCH64_INSN_HINT_SEVL: >> + case AARCH64_INSN_HINT_AUTIASP: >> return false; >> default: >> return true; > > I'm afraid that the existing code here is simply wrong, and this is > adding to the mess. > > We have no idea what HINT space instructions will be in the future, so > the only sensible implementations of aarch64_insn_is_nop() are something > like: > > bool __kprobes aarch64_insn_is_nop(u32 insn) > { > return insn == aarch64_insn_gen_hint(AARCH64_INSN_HINT_NOP); > } > > ... and if we want to check for other HINT instructions, they should be > checked explicitly. > > Can you please change aarch64_insn_is_nop() as above? Agree that current implementation is unclear. I will implement as you suggested. Cheers, Amit > > Generally the logic in aarch64_insn_is_steppable() needs to be reworked > to a whitelist, but at least chagning aarch64_insn_is_nop() this way is > closer to where we want to be. > > Thanks, > Mark. >
On Fri, Feb 28, 2020 at 04:42:10PM +0530, Amit Kachhap wrote: > On 2/27/20 10:18 PM, Mark Rutland wrote: > > On Wed, Feb 19, 2020 at 06:30:40PM +0530, Amit Daniel Kachhap wrote: > > > diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c > > > index 4a9e773..87f7c8a 100644 > > > --- a/arch/arm64/kernel/insn.c > > > +++ b/arch/arm64/kernel/insn.c > > > @@ -63,6 +63,7 @@ bool __kprobes aarch64_insn_is_nop(u32 insn) > > > case AARCH64_INSN_HINT_WFI: > > > case AARCH64_INSN_HINT_SEV: > > > case AARCH64_INSN_HINT_SEVL: > > > + case AARCH64_INSN_HINT_AUTIASP: > > > return false; > > > default: > > > return true; > > > > I'm afraid that the existing code here is simply wrong, and this is > > adding to the mess. > > > > We have no idea what HINT space instructions will be in the future, so > > the only sensible implementations of aarch64_insn_is_nop() are something > > like: > > > > bool __kprobes aarch64_insn_is_nop(u32 insn) > > { > > return insn == aarch64_insn_gen_hint(AARCH64_INSN_HINT_NOP); > > } > > > > ... and if we want to check for other HINT instructions, they should be > > checked explicitly. > > > > Can you please change aarch64_insn_is_nop() as above? > > Agree that current implementation is unclear. > I will implement as you suggested. Well, please don't throw the baby out with the bath water. The existing code might be badly structured, but I think it's going a bit far to say it's "simply wrong". We need a whitelist /somewhere/ and I'd prefer not to regress the existing behaviour just because the whitelist is embedded in a function with "is_nop" in the name. Will
On Fri, Feb 28, 2020 at 11:18:59AM +0000, Will Deacon wrote: > On Fri, Feb 28, 2020 at 04:42:10PM +0530, Amit Kachhap wrote: > > On 2/27/20 10:18 PM, Mark Rutland wrote: > > > On Wed, Feb 19, 2020 at 06:30:40PM +0530, Amit Daniel Kachhap wrote: > > > > diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c > > > > index 4a9e773..87f7c8a 100644 > > > > --- a/arch/arm64/kernel/insn.c > > > > +++ b/arch/arm64/kernel/insn.c > > > > @@ -63,6 +63,7 @@ bool __kprobes aarch64_insn_is_nop(u32 insn) > > > > case AARCH64_INSN_HINT_WFI: > > > > case AARCH64_INSN_HINT_SEV: > > > > case AARCH64_INSN_HINT_SEVL: > > > > + case AARCH64_INSN_HINT_AUTIASP: > > > > return false; > > > > default: > > > > return true; > > > > > > I'm afraid that the existing code here is simply wrong, and this is > > > adding to the mess. > > > > > > We have no idea what HINT space instructions will be in the future, so > > > the only sensible implementations of aarch64_insn_is_nop() are something > > > like: > > > > > > bool __kprobes aarch64_insn_is_nop(u32 insn) > > > { > > > return insn == aarch64_insn_gen_hint(AARCH64_INSN_HINT_NOP); > > > } > > > > > > ... and if we want to check for other HINT instructions, they should be > > > checked explicitly. > > > > > > Can you please change aarch64_insn_is_nop() as above? > > > > Agree that current implementation is unclear. > > I will implement as you suggested. > > Well, please don't throw the baby out with the bath water. The existing > code might be badly structured, but I think it's going a bit far to say > it's "simply wrong". We need a whitelist /somewhere/ and I'd prefer not > to regress the existing behaviour just because the whitelist is embedded > in a function with "is_nop" in the name. Ok; we can follow up with a more complete cleanup after these patches have been merged. Thanks, Mark.
diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h index bb313dd..2e01db0 100644 --- a/arch/arm64/include/asm/insn.h +++ b/arch/arm64/include/asm/insn.h @@ -40,12 +40,13 @@ enum aarch64_insn_encoding_class { }; enum aarch64_insn_hint_op { - AARCH64_INSN_HINT_NOP = 0x0 << 5, - AARCH64_INSN_HINT_YIELD = 0x1 << 5, - AARCH64_INSN_HINT_WFE = 0x2 << 5, - AARCH64_INSN_HINT_WFI = 0x3 << 5, - AARCH64_INSN_HINT_SEV = 0x4 << 5, - AARCH64_INSN_HINT_SEVL = 0x5 << 5, + AARCH64_INSN_HINT_NOP = 0x0 << 5, + AARCH64_INSN_HINT_YIELD = 0x1 << 5, + AARCH64_INSN_HINT_WFE = 0x2 << 5, + AARCH64_INSN_HINT_WFI = 0x3 << 5, + AARCH64_INSN_HINT_SEV = 0x4 << 5, + AARCH64_INSN_HINT_SEVL = 0x5 << 5, + AARCH64_INSN_HINT_AUTIASP = (0x3 << 8) | (0x5 << 5), }; enum aarch64_insn_imm_type { diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c index 4a9e773..87f7c8a 100644 --- a/arch/arm64/kernel/insn.c +++ b/arch/arm64/kernel/insn.c @@ -63,6 +63,7 @@ bool __kprobes aarch64_insn_is_nop(u32 insn) case AARCH64_INSN_HINT_WFI: case AARCH64_INSN_HINT_SEV: case AARCH64_INSN_HINT_SEVL: + case AARCH64_INSN_HINT_AUTIASP: return false; default: return true; diff --git a/arch/arm64/kernel/probes/decode-insn.c b/arch/arm64/kernel/probes/decode-insn.c index b78fac9..a7caf84 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 (AUTIASP) which falls under the hint instructions region. This is done to disallow probe of authenticate instruction in the kernel which may lead to ptrauth faults with the addition of Armv8.6 enhanced ptrauth features. The corresponding append pac ptrauth instruction (PACIASP) is not disabled and they can still be probed. Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com> --- arch/arm64/include/asm/insn.h | 13 +++++++------ arch/arm64/kernel/insn.c | 1 + arch/arm64/kernel/probes/decode-insn.c | 2 +- 3 files changed, 9 insertions(+), 7 deletions(-)