[v2,14/14] lkdtm: arm64: test kernel pointer authentication
diff mbox series

Message ID 1574166746-27197-15-git-send-email-amit.kachhap@arm.com
State New
Headers show
Series
  • arm64: return address signing
Related show

Commit Message

Amit Kachhap Nov. 19, 2019, 12:32 p.m. UTC
This test is specific for arm64. When in-kernel Pointer Authentication
config is enabled, the return address stored in the stack is signed. This
feature helps in ROP kind of attack. If the matching signature is corrupted
then this will fail in authentication and lead to abort.

e.g.
echo CORRUPT_PAC > /sys/kernel/debug/provoke-crash/DIRECT

[   13.118166] lkdtm: Performing direct entry CORRUPT_PAC
[   13.118298] lkdtm: Clearing PAC from the return address
[   13.118466] Unable to handle kernel paging request at virtual address bfff8000108648ec
[   13.118626] Mem abort info:
[   13.118666]   ESR = 0x86000004
[   13.118866]   EC = 0x21: IABT (current EL), IL = 32 bits
[   13.118966]   SET = 0, FnV = 0
[   13.119117]   EA = 0, S1PTW = 0

Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
---
Change since last version:
 * New patch

 drivers/misc/lkdtm/bugs.c  | 17 +++++++++++++++++
 drivers/misc/lkdtm/core.c  |  1 +
 drivers/misc/lkdtm/lkdtm.h |  1 +
 3 files changed, 19 insertions(+)

Comments

Ard Biesheuvel Nov. 21, 2019, 5:39 p.m. UTC | #1
On Tue, 19 Nov 2019 at 13:33, Amit Daniel Kachhap <amit.kachhap@arm.com> wrote:
>
> This test is specific for arm64. When in-kernel Pointer Authentication
> config is enabled, the return address stored in the stack is signed. This
> feature helps in ROP kind of attack. If the matching signature is corrupted
> then this will fail in authentication and lead to abort.
>
> e.g.
> echo CORRUPT_PAC > /sys/kernel/debug/provoke-crash/DIRECT
>
> [   13.118166] lkdtm: Performing direct entry CORRUPT_PAC
> [   13.118298] lkdtm: Clearing PAC from the return address
> [   13.118466] Unable to handle kernel paging request at virtual address bfff8000108648ec
> [   13.118626] Mem abort info:
> [   13.118666]   ESR = 0x86000004
> [   13.118866]   EC = 0x21: IABT (current EL), IL = 32 bits
> [   13.118966]   SET = 0, FnV = 0
> [   13.119117]   EA = 0, S1PTW = 0
>
> Cc: Kees Cook <keescook@chromium.org>
> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
> ---
> Change since last version:
>  * New patch
>
>  drivers/misc/lkdtm/bugs.c  | 17 +++++++++++++++++
>  drivers/misc/lkdtm/core.c  |  1 +
>  drivers/misc/lkdtm/lkdtm.h |  1 +
>  3 files changed, 19 insertions(+)
>
> diff --git a/drivers/misc/lkdtm/bugs.c b/drivers/misc/lkdtm/bugs.c
> index 7284a22..c9bb493 100644
> --- a/drivers/misc/lkdtm/bugs.c
> +++ b/drivers/misc/lkdtm/bugs.c
> @@ -337,3 +337,20 @@ void lkdtm_UNSET_SMEP(void)
>         pr_err("FAIL: this test is x86_64-only\n");
>  #endif
>  }
> +
> +void lkdtm_CORRUPT_PAC(void)
> +{
> +#if IS_ENABLED(CONFIG_ARM64_PTR_AUTH)
> +       u64 ret;
> +
> +       pr_info("Clearing PAC from the return address\n");
> +       /*
> +        * __builtin_return_address masks the PAC bits of return
> +        * address, so set the same again.
> +        */
> +       ret = (u64)__builtin_return_address(0);
> +       asm volatile("str %0, [sp, 8]" : : "r" (ret) : "memory");

This looks a bit fragile to me. You are making assumptions about the
location of the return address in the stack frame which are not
guaranteed to hold.

Couldn't you do this test simply by changing the key?

> +#else
> +       pr_err("FAIL: For arm64 pointer authentication capable systems only\n");
> +#endif
> +}
> diff --git a/drivers/misc/lkdtm/core.c b/drivers/misc/lkdtm/core.c
> index cbc4c90..b9c9927 100644
> --- a/drivers/misc/lkdtm/core.c
> +++ b/drivers/misc/lkdtm/core.c
> @@ -116,6 +116,7 @@ static const struct crashtype crashtypes[] = {
>         CRASHTYPE(STACK_GUARD_PAGE_LEADING),
>         CRASHTYPE(STACK_GUARD_PAGE_TRAILING),
>         CRASHTYPE(UNSET_SMEP),
> +       CRASHTYPE(CORRUPT_PAC),
>         CRASHTYPE(UNALIGNED_LOAD_STORE_WRITE),
>         CRASHTYPE(OVERWRITE_ALLOCATION),
>         CRASHTYPE(WRITE_AFTER_FREE),
> diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h
> index ab446e0..bf12b68 100644
> --- a/drivers/misc/lkdtm/lkdtm.h
> +++ b/drivers/misc/lkdtm/lkdtm.h
> @@ -28,6 +28,7 @@ void lkdtm_CORRUPT_USER_DS(void);
>  void lkdtm_STACK_GUARD_PAGE_LEADING(void);
>  void lkdtm_STACK_GUARD_PAGE_TRAILING(void);
>  void lkdtm_UNSET_SMEP(void);
> +void lkdtm_CORRUPT_PAC(void);
>
>  /* lkdtm_heap.c */
>  void __init lkdtm_heap_init(void);
> --
> 2.7.4
>
Richard Henderson Nov. 22, 2019, 6:51 p.m. UTC | #2
On 11/21/19 5:39 PM, Ard Biesheuvel wrote:
> On Tue, 19 Nov 2019 at 13:33, Amit Daniel Kachhap <amit.kachhap@arm.com> wrote:
>>
>> This test is specific for arm64. When in-kernel Pointer Authentication
>> config is enabled, the return address stored in the stack is signed. This
>> feature helps in ROP kind of attack. If the matching signature is corrupted
>> then this will fail in authentication and lead to abort.
>>
>> e.g.
>> echo CORRUPT_PAC > /sys/kernel/debug/provoke-crash/DIRECT
>>
>> [   13.118166] lkdtm: Performing direct entry CORRUPT_PAC
>> [   13.118298] lkdtm: Clearing PAC from the return address
>> [   13.118466] Unable to handle kernel paging request at virtual address bfff8000108648ec
>> [   13.118626] Mem abort info:
>> [   13.118666]   ESR = 0x86000004
>> [   13.118866]   EC = 0x21: IABT (current EL), IL = 32 bits
>> [   13.118966]   SET = 0, FnV = 0
>> [   13.119117]   EA = 0, S1PTW = 0
>>
>> Cc: Kees Cook <keescook@chromium.org>
>> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
>> ---
>> Change since last version:
>>  * New patch
>>
>>  drivers/misc/lkdtm/bugs.c  | 17 +++++++++++++++++
>>  drivers/misc/lkdtm/core.c  |  1 +
>>  drivers/misc/lkdtm/lkdtm.h |  1 +
>>  3 files changed, 19 insertions(+)
>>
>> diff --git a/drivers/misc/lkdtm/bugs.c b/drivers/misc/lkdtm/bugs.c
>> index 7284a22..c9bb493 100644
>> --- a/drivers/misc/lkdtm/bugs.c
>> +++ b/drivers/misc/lkdtm/bugs.c
>> @@ -337,3 +337,20 @@ void lkdtm_UNSET_SMEP(void)
>>         pr_err("FAIL: this test is x86_64-only\n");
>>  #endif
>>  }
>> +
>> +void lkdtm_CORRUPT_PAC(void)
>> +{
>> +#if IS_ENABLED(CONFIG_ARM64_PTR_AUTH)
>> +       u64 ret;
>> +
>> +       pr_info("Clearing PAC from the return address\n");
>> +       /*
>> +        * __builtin_return_address masks the PAC bits of return
>> +        * address, so set the same again.
>> +        */
>> +       ret = (u64)__builtin_return_address(0);
>> +       asm volatile("str %0, [sp, 8]" : : "r" (ret) : "memory");
> 
> This looks a bit fragile to me. You are making assumptions about the
> location of the return address in the stack frame which are not
> guaranteed to hold.

Indeed.

> Couldn't you do this test simply by changing the key?

That, at least, means you don't have to know the stack frame layout.  However,
there's a chance (1/32767, I think, for the 48-bit vma case w/o TBI) that
changing the key will result in the same hash.

Even when the stack frame happens to be layed out as Amit guesses, the result
is akin to changing the key, such that hash(key, salt, ptr) == 0.

While testing this in qemu, I iterate until I find a <key, salt, ptr> tuple
that definitely produces a different hash.  Usually this loop iterates just
once, but the occasional failures that I got without iterating were annoying
(with TBI enabled in userspace, the chance drops to 1/127, so much more frequent).


r~
Amit Kachhap Nov. 25, 2019, 5:34 a.m. UTC | #3
Hi,

On 11/21/19 11:09 PM, Ard Biesheuvel wrote:
> On Tue, 19 Nov 2019 at 13:33, Amit Daniel Kachhap <amit.kachhap@arm.com> wrote:
>>
>> This test is specific for arm64. When in-kernel Pointer Authentication
>> config is enabled, the return address stored in the stack is signed. This
>> feature helps in ROP kind of attack. If the matching signature is corrupted
>> then this will fail in authentication and lead to abort.
>>
>> e.g.
>> echo CORRUPT_PAC > /sys/kernel/debug/provoke-crash/DIRECT
>>
>> [   13.118166] lkdtm: Performing direct entry CORRUPT_PAC
>> [   13.118298] lkdtm: Clearing PAC from the return address
>> [   13.118466] Unable to handle kernel paging request at virtual address bfff8000108648ec
>> [   13.118626] Mem abort info:
>> [   13.118666]   ESR = 0x86000004
>> [   13.118866]   EC = 0x21: IABT (current EL), IL = 32 bits
>> [   13.118966]   SET = 0, FnV = 0
>> [   13.119117]   EA = 0, S1PTW = 0
>>
>> Cc: Kees Cook <keescook@chromium.org>
>> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
>> ---
>> Change since last version:
>>   * New patch
>>
>>   drivers/misc/lkdtm/bugs.c  | 17 +++++++++++++++++
>>   drivers/misc/lkdtm/core.c  |  1 +
>>   drivers/misc/lkdtm/lkdtm.h |  1 +
>>   3 files changed, 19 insertions(+)
>>
>> diff --git a/drivers/misc/lkdtm/bugs.c b/drivers/misc/lkdtm/bugs.c
>> index 7284a22..c9bb493 100644
>> --- a/drivers/misc/lkdtm/bugs.c
>> +++ b/drivers/misc/lkdtm/bugs.c
>> @@ -337,3 +337,20 @@ void lkdtm_UNSET_SMEP(void)
>>          pr_err("FAIL: this test is x86_64-only\n");
>>   #endif
>>   }
>> +
>> +void lkdtm_CORRUPT_PAC(void)
>> +{
>> +#if IS_ENABLED(CONFIG_ARM64_PTR_AUTH)
>> +       u64 ret;
>> +
>> +       pr_info("Clearing PAC from the return address\n");
>> +       /*
>> +        * __builtin_return_address masks the PAC bits of return
>> +        * address, so set the same again.
>> +        */
>> +       ret = (u64)__builtin_return_address(0);
>> +       asm volatile("str %0, [sp, 8]" : : "r" (ret) : "memory");
> 
> This looks a bit fragile to me. You are making assumptions about the
> location of the return address in the stack frame which are not
> guaranteed to hold.
Yes agreed.
> 
> Couldn't you do this test simply by changing the key?
Yes it should be possible. I will update it in my next iteration.

Regards,
Amit D
> 
>> +#else
>> +       pr_err("FAIL: For arm64 pointer authentication capable systems only\n");
>> +#endif
>> +}
>> diff --git a/drivers/misc/lkdtm/core.c b/drivers/misc/lkdtm/core.c
>> index cbc4c90..b9c9927 100644
>> --- a/drivers/misc/lkdtm/core.c
>> +++ b/drivers/misc/lkdtm/core.c
>> @@ -116,6 +116,7 @@ static const struct crashtype crashtypes[] = {
>>          CRASHTYPE(STACK_GUARD_PAGE_LEADING),
>>          CRASHTYPE(STACK_GUARD_PAGE_TRAILING),
>>          CRASHTYPE(UNSET_SMEP),
>> +       CRASHTYPE(CORRUPT_PAC),
>>          CRASHTYPE(UNALIGNED_LOAD_STORE_WRITE),
>>          CRASHTYPE(OVERWRITE_ALLOCATION),
>>          CRASHTYPE(WRITE_AFTER_FREE),
>> diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h
>> index ab446e0..bf12b68 100644
>> --- a/drivers/misc/lkdtm/lkdtm.h
>> +++ b/drivers/misc/lkdtm/lkdtm.h
>> @@ -28,6 +28,7 @@ void lkdtm_CORRUPT_USER_DS(void);
>>   void lkdtm_STACK_GUARD_PAGE_LEADING(void);
>>   void lkdtm_STACK_GUARD_PAGE_TRAILING(void);
>>   void lkdtm_UNSET_SMEP(void);
>> +void lkdtm_CORRUPT_PAC(void);
>>
>>   /* lkdtm_heap.c */
>>   void __init lkdtm_heap_init(void);
>> --
>> 2.7.4
>>
Amit Kachhap Nov. 25, 2019, 9:25 a.m. UTC | #4
On 11/23/19 12:21 AM, Richard Henderson wrote:
> On 11/21/19 5:39 PM, Ard Biesheuvel wrote:
>> On Tue, 19 Nov 2019 at 13:33, Amit Daniel Kachhap <amit.kachhap@arm.com> wrote:
>>>
>>> This test is specific for arm64. When in-kernel Pointer Authentication
>>> config is enabled, the return address stored in the stack is signed. This
>>> feature helps in ROP kind of attack. If the matching signature is corrupted
>>> then this will fail in authentication and lead to abort.
>>>
>>> e.g.
>>> echo CORRUPT_PAC > /sys/kernel/debug/provoke-crash/DIRECT
>>>
>>> [   13.118166] lkdtm: Performing direct entry CORRUPT_PAC
>>> [   13.118298] lkdtm: Clearing PAC from the return address
>>> [   13.118466] Unable to handle kernel paging request at virtual address bfff8000108648ec
>>> [   13.118626] Mem abort info:
>>> [   13.118666]   ESR = 0x86000004
>>> [   13.118866]   EC = 0x21: IABT (current EL), IL = 32 bits
>>> [   13.118966]   SET = 0, FnV = 0
>>> [   13.119117]   EA = 0, S1PTW = 0
>>>
>>> Cc: Kees Cook <keescook@chromium.org>
>>> Signed-off-by: Amit Daniel Kachhap <amit.kachhap@arm.com>
>>> ---
>>> Change since last version:
>>>   * New patch
>>>
>>>   drivers/misc/lkdtm/bugs.c  | 17 +++++++++++++++++
>>>   drivers/misc/lkdtm/core.c  |  1 +
>>>   drivers/misc/lkdtm/lkdtm.h |  1 +
>>>   3 files changed, 19 insertions(+)
>>>
>>> diff --git a/drivers/misc/lkdtm/bugs.c b/drivers/misc/lkdtm/bugs.c
>>> index 7284a22..c9bb493 100644
>>> --- a/drivers/misc/lkdtm/bugs.c
>>> +++ b/drivers/misc/lkdtm/bugs.c
>>> @@ -337,3 +337,20 @@ void lkdtm_UNSET_SMEP(void)
>>>          pr_err("FAIL: this test is x86_64-only\n");
>>>   #endif
>>>   }
>>> +
>>> +void lkdtm_CORRUPT_PAC(void)
>>> +{
>>> +#if IS_ENABLED(CONFIG_ARM64_PTR_AUTH)
>>> +       u64 ret;
>>> +
>>> +       pr_info("Clearing PAC from the return address\n");
>>> +       /*
>>> +        * __builtin_return_address masks the PAC bits of return
>>> +        * address, so set the same again.
>>> +        */
>>> +       ret = (u64)__builtin_return_address(0);
>>> +       asm volatile("str %0, [sp, 8]" : : "r" (ret) : "memory");
>>
>> This looks a bit fragile to me. You are making assumptions about the
>> location of the return address in the stack frame which are not
>> guaranteed to hold.
> 
> Indeed.
> 
>> Couldn't you do this test simply by changing the key?
> 
> That, at least, means you don't have to know the stack frame layout.  However,
> there's a chance (1/32767, I think, for the 48-bit vma case w/o TBI) that
> changing the key will result in the same hash.
> 
> Even when the stack frame happens to be layed out as Amit guesses, the result
> is akin to changing the key, such that hash(key, salt, ptr) == 0.
> 
> While testing this in qemu, I iterate until I find a <key, salt, ptr> tuple
> that definitely produces a different hash.  Usually this loop iterates just
> once, but the occasional failures that I got without iterating were annoying
> (with TBI enabled in userspace, the chance drops to 1/127, so much more frequent).

Nice suggestion. I will check doing it this way in the next iteration.

> 
> 
> r~
>

Patch
diff mbox series

diff --git a/drivers/misc/lkdtm/bugs.c b/drivers/misc/lkdtm/bugs.c
index 7284a22..c9bb493 100644
--- a/drivers/misc/lkdtm/bugs.c
+++ b/drivers/misc/lkdtm/bugs.c
@@ -337,3 +337,20 @@  void lkdtm_UNSET_SMEP(void)
 	pr_err("FAIL: this test is x86_64-only\n");
 #endif
 }
+
+void lkdtm_CORRUPT_PAC(void)
+{
+#if IS_ENABLED(CONFIG_ARM64_PTR_AUTH)
+	u64 ret;
+
+	pr_info("Clearing PAC from the return address\n");
+	/*
+	 * __builtin_return_address masks the PAC bits of return
+	 * address, so set the same again.
+	 */
+	ret = (u64)__builtin_return_address(0);
+	asm volatile("str %0, [sp, 8]" : : "r" (ret) : "memory");
+#else
+	pr_err("FAIL: For arm64 pointer authentication capable systems only\n");
+#endif
+}
diff --git a/drivers/misc/lkdtm/core.c b/drivers/misc/lkdtm/core.c
index cbc4c90..b9c9927 100644
--- a/drivers/misc/lkdtm/core.c
+++ b/drivers/misc/lkdtm/core.c
@@ -116,6 +116,7 @@  static const struct crashtype crashtypes[] = {
 	CRASHTYPE(STACK_GUARD_PAGE_LEADING),
 	CRASHTYPE(STACK_GUARD_PAGE_TRAILING),
 	CRASHTYPE(UNSET_SMEP),
+	CRASHTYPE(CORRUPT_PAC),
 	CRASHTYPE(UNALIGNED_LOAD_STORE_WRITE),
 	CRASHTYPE(OVERWRITE_ALLOCATION),
 	CRASHTYPE(WRITE_AFTER_FREE),
diff --git a/drivers/misc/lkdtm/lkdtm.h b/drivers/misc/lkdtm/lkdtm.h
index ab446e0..bf12b68 100644
--- a/drivers/misc/lkdtm/lkdtm.h
+++ b/drivers/misc/lkdtm/lkdtm.h
@@ -28,6 +28,7 @@  void lkdtm_CORRUPT_USER_DS(void);
 void lkdtm_STACK_GUARD_PAGE_LEADING(void);
 void lkdtm_STACK_GUARD_PAGE_TRAILING(void);
 void lkdtm_UNSET_SMEP(void);
+void lkdtm_CORRUPT_PAC(void);
 
 /* lkdtm_heap.c */
 void __init lkdtm_heap_init(void);