| Message ID | 20211206104657.433304-13-alexandre.ghiti@canonical.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Introduce sv48 support without relocatable kernel | expand |
On Tue, Dec 7, 2021 at 11:55 AM Alexandre Ghiti <alexandre.ghiti@canonical.com> wrote: > > Because of the stack canary feature that reads from the current task > structure the stack canary value, the thread pointer register "tp" must > be set before calling any C function from head.S: by chance, setup_vm Shall we disable -fstack-protector for setup_vm() with __attribute__? Actually, we've already init tp later. > and all the functions that it calls does not seem to be part of the > functions where the canary check is done, but in the following commits, > some functions will. > > Fixes: f2c9699f65557a31 ("riscv: Add STACKPROTECTOR supported") > Signed-off-by: Alexandre Ghiti <alexandre.ghiti@canonical.com> > --- > arch/riscv/kernel/head.S | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S > index c3c0ed559770..86f7ee3d210d 100644 > --- a/arch/riscv/kernel/head.S > +++ b/arch/riscv/kernel/head.S > @@ -302,6 +302,7 @@ clear_bss_done: > REG_S a0, (a2) > > /* Initialize page tables and relocate to virtual addresses */ > + la tp, init_task > la sp, init_thread_union + THREAD_SIZE > XIP_FIXUP_OFFSET sp > #ifdef CONFIG_BUILTIN_DTB > -- > 2.32.0 >
On Mon, 20 Dec 2021 at 10:11, Guo Ren <guoren@kernel.org> wrote: > > On Tue, Dec 7, 2021 at 11:55 AM Alexandre Ghiti > <alexandre.ghiti@canonical.com> wrote: > > > > Because of the stack canary feature that reads from the current task > > structure the stack canary value, the thread pointer register "tp" must > > be set before calling any C function from head.S: by chance, setup_vm > Shall we disable -fstack-protector for setup_vm() with __attribute__? Don't use __attribute__((optimize())) for that: it is known to be broken, and documented as debug purposes only in the GCC info pages: https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html > Actually, we've already init tp later. > > > and all the functions that it calls does not seem to be part of the > > functions where the canary check is done, but in the following commits, > > some functions will. > > > > Fixes: f2c9699f65557a31 ("riscv: Add STACKPROTECTOR supported") > > Signed-off-by: Alexandre Ghiti <alexandre.ghiti@canonical.com> > > --- > > arch/riscv/kernel/head.S | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S > > index c3c0ed559770..86f7ee3d210d 100644 > > --- a/arch/riscv/kernel/head.S > > +++ b/arch/riscv/kernel/head.S > > @@ -302,6 +302,7 @@ clear_bss_done: > > REG_S a0, (a2) > > > > /* Initialize page tables and relocate to virtual addresses */ > > + la tp, init_task > > la sp, init_thread_union + THREAD_SIZE > > XIP_FIXUP_OFFSET sp > > #ifdef CONFIG_BUILTIN_DTB > > -- > > 2.32.0 > > > > > -- > Best Regards > Guo Ren > > ML: https://lore.kernel.org/linux-csky/
On Mon, Dec 20, 2021 at 5:17 PM Ard Biesheuvel <ardb@kernel.org> wrote: > > On Mon, 20 Dec 2021 at 10:11, Guo Ren <guoren@kernel.org> wrote: > > > > On Tue, Dec 7, 2021 at 11:55 AM Alexandre Ghiti > > <alexandre.ghiti@canonical.com> wrote: > > > > > > Because of the stack canary feature that reads from the current task > > > structure the stack canary value, the thread pointer register "tp" must > > > be set before calling any C function from head.S: by chance, setup_vm > > Shall we disable -fstack-protector for setup_vm() with __attribute__? > > Don't use __attribute__((optimize())) for that: it is known to be > broken, and documented as debug purposes only in the GCC info pages: > > https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html Oh, thx for the link. > > > > > > Actually, we've already init tp later. > > > > > and all the functions that it calls does not seem to be part of the > > > functions where the canary check is done, but in the following commits, > > > some functions will. > > > > > > Fixes: f2c9699f65557a31 ("riscv: Add STACKPROTECTOR supported") > > > Signed-off-by: Alexandre Ghiti <alexandre.ghiti@canonical.com> > > > --- > > > arch/riscv/kernel/head.S | 1 + > > > 1 file changed, 1 insertion(+) > > > > > > diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S > > > index c3c0ed559770..86f7ee3d210d 100644 > > > --- a/arch/riscv/kernel/head.S > > > +++ b/arch/riscv/kernel/head.S > > > @@ -302,6 +302,7 @@ clear_bss_done: > > > REG_S a0, (a2) > > > > > > /* Initialize page tables and relocate to virtual addresses */ > > > + la tp, init_task > > > la sp, init_thread_union + THREAD_SIZE > > > XIP_FIXUP_OFFSET sp > > > #ifdef CONFIG_BUILTIN_DTB > > > -- > > > 2.32.0 > > > > > > > > > -- > > Best Regards > > Guo Ren > > > > ML: https://lore.kernel.org/linux-csky/
Hi Palmer, I fell onto this issue again today, do you think you could take this patch in for-next? Because I assume it is too late now to take the sv48 patchset: if not, I can respin it today or tomorrow. Thanks, Alex On 12/6/21 11:46, Alexandre Ghiti wrote: > Because of the stack canary feature that reads from the current task > structure the stack canary value, the thread pointer register "tp" must > be set before calling any C function from head.S: by chance, setup_vm > and all the functions that it calls does not seem to be part of the > functions where the canary check is done, but in the following commits, > some functions will. > > Fixes: f2c9699f65557a31 ("riscv: Add STACKPROTECTOR supported") > Signed-off-by: Alexandre Ghiti <alexandre.ghiti@canonical.com> > --- > arch/riscv/kernel/head.S | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S > index c3c0ed559770..86f7ee3d210d 100644 > --- a/arch/riscv/kernel/head.S > +++ b/arch/riscv/kernel/head.S > @@ -302,6 +302,7 @@ clear_bss_done: > REG_S a0, (a2) > > /* Initialize page tables and relocate to virtual addresses */ > + la tp, init_task > la sp, init_thread_union + THREAD_SIZE > XIP_FIXUP_OFFSET sp > #ifdef CONFIG_BUILTIN_DTB
diff --git a/arch/riscv/kernel/head.S b/arch/riscv/kernel/head.S index c3c0ed559770..86f7ee3d210d 100644 --- a/arch/riscv/kernel/head.S +++ b/arch/riscv/kernel/head.S @@ -302,6 +302,7 @@ clear_bss_done: REG_S a0, (a2) /* Initialize page tables and relocate to virtual addresses */ + la tp, init_task la sp, init_thread_union + THREAD_SIZE XIP_FIXUP_OFFSET sp #ifdef CONFIG_BUILTIN_DTB
Because of the stack canary feature that reads from the current task structure the stack canary value, the thread pointer register "tp" must be set before calling any C function from head.S: by chance, setup_vm and all the functions that it calls does not seem to be part of the functions where the canary check is done, but in the following commits, some functions will. Fixes: f2c9699f65557a31 ("riscv: Add STACKPROTECTOR supported") Signed-off-by: Alexandre Ghiti <alexandre.ghiti@canonical.com> --- arch/riscv/kernel/head.S | 1 + 1 file changed, 1 insertion(+)