From: Amit Kachhap <amit.kachhap@arm.com>
To: Dave Martin <Dave.Martin@arm.com>
Cc: Mark Rutland <mark.rutland@arm.com>,
Kees Cook <keescook@chromium.org>,
Suzuki K Poulose <suzuki.poulose@arm.com>,
Catalin Marinas <catalin.marinas@arm.com>,
Mark Brown <broonie@kernel.org>,
James Morse <james.morse@arm.com>,
Vincenzo Frascino <Vincenzo.Frascino@arm.com>,
Will Deacon <will@kernel.org>,
linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v5 1/6] arm64: kprobe: add checks for ARMv8.3-PAuth combined instructions
Date: Fri, 4 Sep 2020 11:45:12 +0530 [thread overview]
Message-ID: <388d4bbc-5a1e-af51-7b0b-f5c026ffd2f1@arm.com> (raw)
In-Reply-To: <20200902153904.GE6642@arm.com>
Hi,
On 9/2/20 9:09 PM, Dave Martin wrote:
> On Tue, Aug 18, 2020 at 12:41:06PM +0530, Amit Daniel Kachhap wrote:
>> Currently the ARMv8.3-PAuth combined branch instructions (braa, retaa
>> etc.) are not simulated. However the uprobe of such instructions leads
>> to kernel warnings in a loop as they are not explicitly checked.
>>
>> This is fixed by adding definitions of all such instructions and
>> rejecting their probes.
>
> For my understanding, this patch addresses that fact that uprobes
> expects the pc to be at xol_addr + 4 after a stepped instruction, except
> for branch instructions that it explicitly knows about?
>
> If so, this indeed makes sense as a fix to the existing code.
ok
>
>
> LDRAA/LDRAB are not affected by this, right?
>
> I'm wondering what happens if these generate an FPAC exception rather
> than the data abort that would be expected in v8.3. I think the ptrauth
> enhancements allow this, but don't require it.
LDRAA/LDRAB can be simulated by uprobe. As per the Arm Arm(C6.2.134) for
LDRAA/LDRBB,
"If the authentication fails, a Translation fault is generated"
This is similar to ldr behaviour so did't do anything for those
instructions. Although, I should explicitly mention it.
>
> If uprobes can't cope with SIGILLs in xol instructions then we probably
> have bigger problems.
>
> I _think_ the logic in handle_singlestep(), uprobe_deny_signal(),
> arch_uprobe_xol_was_trapped() etc. does handle this scenario, but I
> confess I don't fully understand all the details of it.
I verified with a SIGILL generating xol instruction and it behaved
properly with a SIGILL signal so probably it is fine.
>
> Modulo that:
>
> Reviewed-by: Dave Martin <Dave.Martin@arm.com>
Thanks,
Amit
>
> Cheers
> ---Dave
>
>
>>
>> Warning log:
>> WARNING: CPU: 5 PID: 249 at arch/arm64/kernel/probes/uprobes.c:182 uprobe_single_step_handler+0x34/0x50
>> Modules linked in:
>> CPU: 5 PID: 249 Comm: func Tainted: G W 5.8.0-rc4-00005-ge658591d66d1-dirty #160
>> Hardware name: Foundation-v8A (DT)
>> pstate: 204003c9 (nzCv DAIF +PAN -UAO BTYPE=--)
>> pc : uprobe_single_step_handler+0x34/0x50
>> lr : single_step_handler+0x70/0xf8
>> sp : ffff800012afbe30
>> x29: ffff800012afbe30 x28: ffff000879f00ec0
>> x27: 0000000000000000 x26: 0000000000000000
>> x25: 0000000000000000 x24: 0000000000000000
>> x23: 0000000060001000 x22: 00000000cb000022
>> x21: ffff800011fc5a68 x20: ffff800012afbec0
>> x19: ffff800011fc86c0 x18: 0000000000000000
>> x17: 0000000000000000 x16: 0000000000000000
>> x15: 0000000000000000 x14: 0000000000000000
>> x13: 0000000000000000 x12: 0000000000000000
>> x11: 0000000000000000 x10: 0000000000000000
>> x9 : ffff800010085d50 x8 : 0000000000000000
>> x7 : 0000000000000000 x6 : ffff800011fba9c0
>> x5 : ffff800011fba000 x4 : ffff800012283070
>> x3 : ffff8000100a78e0 x2 : 00000000004005f0
>> x1 : 0000fffffffff008 x0 : ffff800012afbec0
>> Call trace:
>> uprobe_single_step_handler+0x34/0x50
>> single_step_handler+0x70/0xf8
>> do_debug_exception+0xb8/0x130
>> el0_sync_handler+0x7c/0x188
>> el0_sync+0x158/0x180
>>
>> Fixes: 74afda4016a7 ("arm64: compile the kernel with ptrauth return address signing")
>> Fixes: 04ca3204fa09 ("arm64: enable pointer authentication")
>> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
>> ---
>> Changes since v4:
>> * New patch.
>>
>> arch/arm64/include/asm/insn.h | 12 ++++++++++++
>> arch/arm64/kernel/insn.c | 14 ++++++++++++--
>> arch/arm64/kernel/probes/decode-insn.c | 4 +++-
>> 3 files changed, 27 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm64/include/asm/insn.h b/arch/arm64/include/asm/insn.h
>> index 0bc46149e491..324234068fee 100644
>> --- a/arch/arm64/include/asm/insn.h
>> +++ b/arch/arm64/include/asm/insn.h
>> @@ -359,9 +359,21 @@ __AARCH64_INSN_FUNCS(brk, 0xFFE0001F, 0xD4200000)
>> __AARCH64_INSN_FUNCS(exception, 0xFF000000, 0xD4000000)
>> __AARCH64_INSN_FUNCS(hint, 0xFFFFF01F, 0xD503201F)
>> __AARCH64_INSN_FUNCS(br, 0xFFFFFC1F, 0xD61F0000)
>> +__AARCH64_INSN_FUNCS(braaz, 0xFFFFFC1F, 0xD61F081F)
>> +__AARCH64_INSN_FUNCS(brabz, 0xFFFFFC1F, 0xD61F0C1F)
>> +__AARCH64_INSN_FUNCS(braa, 0xFFFFFC00, 0xD71F0800)
>> +__AARCH64_INSN_FUNCS(brab, 0xFFFFFC00, 0xD71F0C00)
>> __AARCH64_INSN_FUNCS(blr, 0xFFFFFC1F, 0xD63F0000)
>> +__AARCH64_INSN_FUNCS(blraaz, 0xFFFFFC1F, 0xD63F081F)
>> +__AARCH64_INSN_FUNCS(blrabz, 0xFFFFFC1F, 0xD63F0C1F)
>> +__AARCH64_INSN_FUNCS(blraa, 0xFFFFFC00, 0xD73F0800)
>> +__AARCH64_INSN_FUNCS(blrab, 0xFFFFFC00, 0xD73F0C00)
>> __AARCH64_INSN_FUNCS(ret, 0xFFFFFC1F, 0xD65F0000)
>> +__AARCH64_INSN_FUNCS(retaa, 0xFFFFFFFF, 0xD65F0BFF)
>> +__AARCH64_INSN_FUNCS(retab, 0xFFFFFFFF, 0xD65F0FFF)
>> __AARCH64_INSN_FUNCS(eret, 0xFFFFFFFF, 0xD69F03E0)
>> +__AARCH64_INSN_FUNCS(eretaa, 0xFFFFFFFF, 0xD69F0BFF)
>> +__AARCH64_INSN_FUNCS(eretab, 0xFFFFFFFF, 0xD69F0FFF)
>> __AARCH64_INSN_FUNCS(mrs, 0xFFF00000, 0xD5300000)
>> __AARCH64_INSN_FUNCS(msr_imm, 0xFFF8F01F, 0xD500401F)
>> __AARCH64_INSN_FUNCS(msr_reg, 0xFFF00000, 0xD5100000)
>> diff --git a/arch/arm64/kernel/insn.c b/arch/arm64/kernel/insn.c
>> index a107375005bc..27d5a52d5058 100644
>> --- a/arch/arm64/kernel/insn.c
>> +++ b/arch/arm64/kernel/insn.c
>> @@ -176,7 +176,7 @@ bool __kprobes aarch64_insn_uses_literal(u32 insn)
>>
>> bool __kprobes aarch64_insn_is_branch(u32 insn)
>> {
>> - /* b, bl, cb*, tb*, b.cond, br, blr */
>> + /* b, bl, cb*, tb*, ret*, b.cond, br*, blr* */
>>
>> return aarch64_insn_is_b(insn) ||
>> aarch64_insn_is_bl(insn) ||
>> @@ -185,9 +185,19 @@ bool __kprobes aarch64_insn_is_branch(u32 insn)
>> aarch64_insn_is_tbz(insn) ||
>> aarch64_insn_is_tbnz(insn) ||
>> aarch64_insn_is_ret(insn) ||
>> + aarch64_insn_is_retaa(insn) ||
>> + aarch64_insn_is_retab(insn) ||
>> aarch64_insn_is_br(insn) ||
>> aarch64_insn_is_blr(insn) ||
>> - aarch64_insn_is_bcond(insn);
>> + aarch64_insn_is_bcond(insn) ||
>> + aarch64_insn_is_braaz(insn) ||
>> + aarch64_insn_is_brabz(insn) ||
>> + aarch64_insn_is_braa(insn) ||
>> + aarch64_insn_is_brab(insn) ||
>> + aarch64_insn_is_blraaz(insn) ||
>> + aarch64_insn_is_blrabz(insn) ||
>> + aarch64_insn_is_blraa(insn) ||
>> + aarch64_insn_is_blrab(insn);
>> }
>>
>> int __kprobes aarch64_insn_patch_text_nosync(void *addr, u32 insn)
>> diff --git a/arch/arm64/kernel/probes/decode-insn.c b/arch/arm64/kernel/probes/decode-insn.c
>> index 263d5fba4c8a..f9eb8210d6d3 100644
>> --- a/arch/arm64/kernel/probes/decode-insn.c
>> +++ b/arch/arm64/kernel/probes/decode-insn.c
>> @@ -29,7 +29,9 @@ static bool __kprobes aarch64_insn_is_steppable(u32 insn)
>> aarch64_insn_is_msr_imm(insn) ||
>> aarch64_insn_is_msr_reg(insn) ||
>> aarch64_insn_is_exception(insn) ||
>> - aarch64_insn_is_eret(insn))
>> + aarch64_insn_is_eret(insn) ||
>> + aarch64_insn_is_eretaa(insn) ||
>> + aarch64_insn_is_eretab(insn))
>> return false;
>>
>> /*
>> --
>> 2.17.1
>>
>>
>> _______________________________________________
>> linux-arm-kernel mailing list
>> linux-arm-kernel@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2020-09-04 6:16 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-08-18 7:11 [PATCH v5 0/6] arm64: add Armv8.3 pointer authentication enhancements Amit Daniel Kachhap
2020-08-18 7:11 ` [PATCH v5 1/6] arm64: kprobe: add checks for ARMv8.3-PAuth combined instructions Amit Daniel Kachhap
2020-09-02 15:39 ` Dave Martin
2020-09-04 6:15 ` Amit Kachhap [this message]
2020-08-18 7:11 ` [PATCH v5 2/6] arm64: traps: Allow force_signal_inject to pass esr error code Amit Daniel Kachhap
2020-09-02 15:39 ` Dave Martin
2020-08-18 7:11 ` [PATCH v5 3/6] arm64: ptrauth: Introduce Armv8.3 pointer authentication enhancements Amit Daniel Kachhap
2020-09-02 15:39 ` Dave Martin
2020-08-18 7:11 ` [PATCH v5 4/6] arm64: cpufeature: Modify address authentication cpufeature to exact Amit Daniel Kachhap
2020-09-02 14:05 ` Suzuki K Poulose
2020-08-18 7:11 ` [PATCH v5 5/6] arm64: kprobe: disable probe of fault prone ptrauth instruction Amit Daniel Kachhap
2020-08-18 7:11 ` [PATCH v5 6/6] arm64: kprobe: clarify the comment of steppable hint instructions Amit Daniel Kachhap
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=388d4bbc-5a1e-af51-7b0b-f5c026ffd2f1@arm.com \
--to=amit.kachhap@arm.com \
--cc=Dave.Martin@arm.com \
--cc=Vincenzo.Frascino@arm.com \
--cc=broonie@kernel.org \
--cc=catalin.marinas@arm.com \
--cc=james.morse@arm.com \
--cc=keescook@chromium.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=mark.rutland@arm.com \
--cc=suzuki.poulose@arm.com \
--cc=will@kernel.org \
/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: link
Be 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.