Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

per-task stack canaries on arm #289

Closed
nickdesaulniers opened this issue Dec 10, 2018 · 11 comments
Closed

per-task stack canaries on arm #289

nickdesaulniers opened this issue Dec 10, 2018 · 11 comments
Assignees
@nickdesaulniers
Labels
[ARCH] arm32 This bug impacts ARCH=arm [ARCH] arm64 This bug impacts ARCH=arm64 feature-request Not a bug per-se [FIXED][LLVM] 13 This bug was fixed in LLVM 13.x polish Not a correctness bug, but will improve code performance or size Reported upstream This bug was filed on LLVM’s issue tracker, Phabricator, or the kernel mailing list. security security implication

Comments

@nickdesaulniers
Copy link
Member

nickdesaulniers commented Dec 10, 2018 •
edited by nathanchance
Loading

https://lore.kernel.org/r/[email protected]/
Ard has a blog post and emailed me about this, too.
@nickdesaulniers nickdesaulniers added the feature-request Not a bug per-se label Dec 10, 2018
@nickdesaulniers
Copy link
Member Author

http://www.workofard.com/2018/01/per-task-stack-canaries-for-arm64/

@ardbiesheuvel
Copy link

Note that the LKML link above is for 32-bit ARM. For arm64, we are working with the GCC community to get this supported in the compiler directly. Please refer to the link below for the current proposal (which is not final yet)

https://gcc.gnu.org/ml/gcc-patches/2018-12/msg00062.html

@kees kees added the [ARCH] arm64 This bug impacts ARCH=arm64 label May 16, 2019
@kees kees changed the title arm gcc plugin for per task stack canaries per-task stack canaries on arm May 16, 2019
@kees kees added the [ARCH] arm32 This bug impacts ARCH=arm label May 16, 2019
@kees
Copy link

kees commented May 16, 2019

Here is the GCC patch for arm64:
https://gcc.gnu.org/git/?p=gcc.git;a=commitdiff;h=359c1bf35e3109d2f3882980b47a5eae46123259

And kernel support:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=0a1213fa7432778b71a1c0166bf56660a3aab030

nathanchance pushed a commit that referenced this issue Oct 18, 2019
@rafaeljw
ACPI: CPPC: Set pcc_data[pcc_ss_id] to NULL in acpi_cppc_processor_ex…
56a0b97 
…it()

When enabling KASAN and DEBUG_TEST_DRIVER_REMOVE, I find this KASAN
warning:

[   20.872057] BUG: KASAN: use-after-free in pcc_data_alloc+0x40/0xb8
[   20.878226] Read of size 4 at addr ffff00236cdeb684 by task swapper/0/1
[   20.884826]
[   20.886309] CPU: 19 PID: 1 Comm: swapper/0 Not tainted 5.4.0-rc1-00009-ge7f7df3db5bf-dirty #289
[   20.894994] Hardware name: Huawei D06 /D06, BIOS Hisilicon D06 UEFI RC0 - V1.16.01 03/15/2019
[   20.903505] Call trace:
[   20.905942]  dump_backtrace+0x0/0x200
[   20.909593]  show_stack+0x14/0x20
[   20.912899]  dump_stack+0xd4/0x130
[   20.916291]  print_address_description.isra.9+0x6c/0x3b8
[   20.921592]  __kasan_report+0x12c/0x23c
[   20.925417]  kasan_report+0xc/0x18
[   20.928808]  __asan_load4+0x94/0xb8
[   20.932286]  pcc_data_alloc+0x40/0xb8
[   20.935938]  acpi_cppc_processor_probe+0x4e8/0xb08
[   20.940717]  __acpi_processor_start+0x48/0xb0
[   20.945062]  acpi_processor_start+0x40/0x60
[   20.949235]  really_probe+0x118/0x548
[   20.952887]  driver_probe_device+0x7c/0x148
[   20.957059]  device_driver_attach+0x94/0xa0
[   20.961231]  __driver_attach+0xa4/0x110
[   20.965055]  bus_for_each_dev+0xe8/0x158
[   20.968966]  driver_attach+0x30/0x40
[   20.972531]  bus_add_driver+0x234/0x2f0
[   20.976356]  driver_register+0xbc/0x1d0
[   20.980182]  acpi_processor_driver_init+0x40/0xe4
[   20.984875]  do_one_initcall+0xb4/0x254
[   20.988700]  kernel_init_freeable+0x24c/0x2f8
[   20.993047]  kernel_init+0x10/0x118
[   20.996524]  ret_from_fork+0x10/0x18
[   21.000087]
[   21.001567] Allocated by task 1:
[   21.004785]  save_stack+0x28/0xc8
[   21.008089]  __kasan_kmalloc.isra.9+0xbc/0xd8
[   21.012435]  kasan_kmalloc+0xc/0x18
[   21.015913]  pcc_data_alloc+0x94/0xb8
[   21.019564]  acpi_cppc_processor_probe+0x4e8/0xb08
[   21.024343]  __acpi_processor_start+0x48/0xb0
[   21.028689]  acpi_processor_start+0x40/0x60
[   21.032860]  really_probe+0x118/0x548
[   21.036512]  driver_probe_device+0x7c/0x148
[   21.040684]  device_driver_attach+0x94/0xa0
[   21.044855]  __driver_attach+0xa4/0x110
[   21.048680]  bus_for_each_dev+0xe8/0x158
[   21.052591]  driver_attach+0x30/0x40
[   21.056155]  bus_add_driver+0x234/0x2f0
[   21.059980]  driver_register+0xbc/0x1d0
[   21.063805]  acpi_processor_driver_init+0x40/0xe4
[   21.068497]  do_one_initcall+0xb4/0x254
[   21.072322]  kernel_init_freeable+0x24c/0x2f8
[   21.076667]  kernel_init+0x10/0x118
[   21.080144]  ret_from_fork+0x10/0x18
[   21.083707]
[   21.085186] Freed by task 1:
[   21.088056]  save_stack+0x28/0xc8
[   21.091360]  __kasan_slab_free+0x118/0x180
[   21.095445]  kasan_slab_free+0x10/0x18
[   21.099183]  kfree+0x80/0x268
[   21.102139]  acpi_cppc_processor_exit+0x1a8/0x1b8
[   21.106832]  acpi_processor_stop+0x70/0x80
[   21.110917]  really_probe+0x174/0x548
[   21.114568]  driver_probe_device+0x7c/0x148
[   21.118740]  device_driver_attach+0x94/0xa0
[   21.122912]  __driver_attach+0xa4/0x110
[   21.126736]  bus_for_each_dev+0xe8/0x158
[   21.130648]  driver_attach+0x30/0x40
[   21.134212]  bus_add_driver+0x234/0x2f0
[   21.0x10/0x18
[   21.161764]
[   21.163244] The buggy address belongs to the object at ffff00236cdeb600
[   21.163244]  which belongs to the cache kmalloc-256 of size 256
[   21.175750] The buggy address is located 132 bytes inside of
[   21.175750]  256-byte region [ffff00236cdeb600, ffff00236cdeb700)
[   21.187473] The buggy address belongs to the page:
[   21.192254] page:fffffe008d937a00 refcount:1 mapcount:0 mapping:ffff002370c0fa00 index:0x0 compound_mapcount: 0
[   21.202331] flags: 0x1ffff00000010200(slab|head)
[   21.206940] raw: 1ffff00000010200 dead000000000100 dead000000000122 ffff002370c0fa00
[   21.214671] raw: 0000000000000000 00000000802a002a 00000001ffffffff 0000000000000000
[   21.222400] page dumped because: kasan: bad access detected
[   21.227959]
[   21.229438] Memory state around the buggy address:
[   21.234218]  ffff00236cdeb580: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[   21.241427]  ffff00236cdeb600: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[   21.248637] >ffff00236cdeb680: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[   21.255845]                    ^
[   21.259062]  ffff00236cdeb700: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[   21.266272]  ffff00236cdeb780: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
[   21.273480] ==================================================================

It seems that global pcc_data[pcc_ss_id] can be freed in
acpi_cppc_processor_exit(), but we may later reference this value, so
NULLify it when freed.

Also remove the useless setting of data "pcc_channel_acquired", which
we're about to free.

Fixes: 85b1407 ("ACPI / CPPC: Make CPPC ACPI driver aware of PCC subspace IDs")
Signed-off-by: John Garry <[email protected]>
Cc: 4.15+ <[email protected]> # 4.15+
Signed-off-by: Rafael J. Wysocki <[email protected]>
@nickdesaulniers nickdesaulniers added the polish Not a correctness bug, but will improve code performance or size label Feb 8, 2020
@nickdesaulniers
Copy link
Member Author

It sounds like CFI may help with this, but per task stack canaries provide additional benefits closer to general stack buffer overflows.

@nickdesaulniers
Copy link
Member Author

Also, it sounds like this might be possible to implement as a flag like -fstack-canary-reg=x18 or such.

@nickdesaulniers
Copy link
Member Author

@kees mentions that this isn't necessarily kernel specific; that this could be used in userspace. In that case, I suggest we look into just implementing this in clang, no plugin needed.

@nickdesaulniers nickdesaulniers added the security security implication label Feb 8, 2020
@kees
Copy link

kees commented Feb 8, 2020

Also tracked in KSPP: KSPP#29

@willdeacon
Copy link

It would be really great to have this feature in clang, as it is supported out of the box by the arm64 Linux kernel. Where do I have to send beer?

@nickdesaulniers
Copy link
Member Author

Work has started on this: https://reviews.llvm.org/D88631. Folks at Intel have supplied the initial implementation. I've left code review comments seeking to make the initial implementation easier for us to extend from other architectures from x86. (cc @topperc ) I'm curious if it's worthwhile to extend this idea/work from PPC/Arm64 to x86 in the kernel?

@nickdesaulniers
Copy link
Member Author

nickdesaulniers commented Mar 4, 2021 •
edited by nathanchance
Loading

https://llvm.org/pr47341

@nickdesaulniers nickdesaulniers added the Reported upstream This bug was filed on LLVM’s issue tracker, Phabricator, or the kernel mailing list. label Mar 4, 2021
@kees kees mentioned this issue Mar 4, 2021
Open
33 tasks
@nickdesaulniers nickdesaulniers self-assigned this Apr 21, 2021
@nickdesaulniers nickdesaulniers added the [PATCH] Exists There is a patch that fixes this issue label Apr 21, 2021
@nickdesaulniers
Copy link
Member Author

https://reviews.llvm.org/D100919

@nickdesaulniers nickdesaulniers added [PATCH] Submitted A patch has been submitted for review and removed [PATCH] Exists There is a patch that fixes this issue labels Apr 21, 2021
@nickdesaulniers nickdesaulniers added [FIXED][LLVM] 13 This bug was fixed in LLVM 13.x and removed [PATCH] Submitted A patch has been submitted for review labels May 17, 2021
arichardson pushed a commit to arichardson/llvm-project that referenced this issue Sep 12, 2021
@nickdesaulniers @arichardson
[AArch64] Support customizing stack protector guard
df56989 
Follow up to D88631 but for aarch64; the Linux kernel uses the command
line flags:

1. -mstack-protector-guard=sysreg
2. -mstack-protector-guard-reg=sp_el0
3. -mstack-protector-guard-offset=0

to use the system register sp_el0 for the stack canary, enabling the
kernel to have a unique stack canary per task (like a thread, but not
limited to userspace as the kernel can preempt itself).

Address pr/47341 for aarch64.

Fixes: ClangBuiltLinux/linux#289
Signed-off-by: Nick Desaulniers <[email protected]>

Reviewed By: xiangzhangllvm, DavidSpickett, dmgreen

Differential Revision: https://reviews.llvm.org/D100919
vgvassilev pushed a commit to vgvassilev/clang that referenced this issue Dec 28, 2022
@nickdesaulniers @vgvassilev
[AArch64] Support customizing stack protector guard
041d14e 
Follow up to D88631 but for aarch64; the Linux kernel uses the command
line flags:

1. -mstack-protector-guard=sysreg
2. -mstack-protector-guard-reg=sp_el0
3. -mstack-protector-guard-offset=0

to use the system register sp_el0 for the stack canary, enabling the
kernel to have a unique stack canary per task (like a thread, but not
limited to userspace as the kernel can preempt itself).

Address pr/47341 for aarch64.

Fixes: ClangBuiltLinux/linux#289
Signed-off-by: Nick Desaulniers <[email protected]>

Reviewed By: xiangzhangllvm, DavidSpickett, dmgreen

Differential Revision: https://reviews.llvm.org/D100919

llvm-monorepo: 0f417789192e74f9d2fad0f6aee4efc394257176
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@nickdesaulniers nickdesaulniers

Labels
[ARCH] arm32 This bug impacts ARCH=arm [ARCH] arm64 This bug impacts ARCH=arm64 feature-request Not a bug per-se [FIXED][LLVM] 13 This bug was fixed in LLVM 13.x polish Not a correctness bug, but will improve code performance or size Reported upstream This bug was filed on LLVM’s issue tracker, Phabricator, or the kernel mailing list. security security implication
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
4 participants
@kees @nickdesaulniers @ardbiesheuvel @willdeacon