* [PATCH 1/3] Revert "riscv/efistub: Ensure GP-relative addressing is not used"
@ 2024-03-06 8:56 ` Yunhui Cui
0 siblings, 0 replies; 48+ messages in thread
From: Yunhui Cui @ 2024-03-06 8:56 UTC (permalink / raw)
To: paul.walmsley, palmer, aou, ardb, cuiyunhui, xuzhipeng.1973,
alexghiti, samitolvanen, bp, xiao.w.wang, jan.kiszka,
kirill.shutemov, nathan, linux-riscv, linux-kernel, linux-efi
This reverts commit afb2a4fb84555ef9e61061f6ea63ed7087b295d5.
Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
---
drivers/firmware/efi/libstub/Makefile | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
index 31eb1e287ce1..475f37796779 100644
--- a/drivers/firmware/efi/libstub/Makefile
+++ b/drivers/firmware/efi/libstub/Makefile
@@ -28,7 +28,7 @@ cflags-$(CONFIG_ARM) += -DEFI_HAVE_STRLEN -DEFI_HAVE_STRNLEN \
-DEFI_HAVE_MEMCHR -DEFI_HAVE_STRRCHR \
-DEFI_HAVE_STRCMP -fno-builtin -fpic \
$(call cc-option,-mno-single-pic-base)
-cflags-$(CONFIG_RISCV) += -fpic -DNO_ALTERNATIVE -mno-relax
+cflags-$(CONFIG_RISCV) += -fpic -DNO_ALTERNATIVE
cflags-$(CONFIG_LOONGARCH) += -fpie
cflags-$(CONFIG_EFI_PARAMS_FROM_FDT) += -I$(srctree)/scripts/dtc/libfdt
--
2.20.1
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH 1/3] Revert "riscv/efistub: Ensure GP-relative addressing is not used"
@ 2024-03-06 8:56 ` Yunhui Cui
0 siblings, 0 replies; 48+ messages in thread
From: Yunhui Cui @ 2024-03-06 8:56 UTC (permalink / raw)
To: paul.walmsley, palmer, aou, ardb, cuiyunhui, xuzhipeng.1973,
alexghiti, samitolvanen, bp, xiao.w.wang, jan.kiszka,
kirill.shutemov, nathan, linux-riscv, linux-kernel, linux-efi
This reverts commit afb2a4fb84555ef9e61061f6ea63ed7087b295d5.
Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
---
drivers/firmware/efi/libstub/Makefile | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
index 31eb1e287ce1..475f37796779 100644
--- a/drivers/firmware/efi/libstub/Makefile
+++ b/drivers/firmware/efi/libstub/Makefile
@@ -28,7 +28,7 @@ cflags-$(CONFIG_ARM) += -DEFI_HAVE_STRLEN -DEFI_HAVE_STRNLEN \
-DEFI_HAVE_MEMCHR -DEFI_HAVE_STRRCHR \
-DEFI_HAVE_STRCMP -fno-builtin -fpic \
$(call cc-option,-mno-single-pic-base)
-cflags-$(CONFIG_RISCV) += -fpic -DNO_ALTERNATIVE -mno-relax
+cflags-$(CONFIG_RISCV) += -fpic -DNO_ALTERNATIVE
cflags-$(CONFIG_LOONGARCH) += -fpie
cflags-$(CONFIG_EFI_PARAMS_FROM_FDT) += -I$(srctree)/scripts/dtc/libfdt
--
2.20.1
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH 2/3] Revert "riscv/efistub: Tighten ELF relocation check"
2024-03-06 8:56 ` Yunhui Cui
@ 2024-03-06 8:56 ` Yunhui Cui
-1 siblings, 0 replies; 48+ messages in thread
From: Yunhui Cui @ 2024-03-06 8:56 UTC (permalink / raw)
To: paul.walmsley, palmer, aou, ardb, cuiyunhui, xuzhipeng.1973,
alexghiti, samitolvanen, bp, xiao.w.wang, jan.kiszka,
kirill.shutemov, nathan, linux-riscv, linux-kernel, linux-efi
This reverts commit d2baf8cc82c17459fca019a12348efcf86bfec29.
Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
---
drivers/firmware/efi/libstub/Makefile | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
index 475f37796779..a223bd10564b 100644
--- a/drivers/firmware/efi/libstub/Makefile
+++ b/drivers/firmware/efi/libstub/Makefile
@@ -143,7 +143,7 @@ STUBCOPY_RELOC-$(CONFIG_ARM64) := R_AARCH64_ABS
# exist.
STUBCOPY_FLAGS-$(CONFIG_RISCV) += --prefix-alloc-sections=.init \
--prefix-symbols=__efistub_
-STUBCOPY_RELOC-$(CONFIG_RISCV) := -E R_RISCV_HI20\|R_RISCV_$(BITS)\|R_RISCV_RELAX
+STUBCOPY_RELOC-$(CONFIG_RISCV) := R_RISCV_HI20
# For LoongArch, keep all the symbols in .init section and make sure that no
# absolute symbols references exist.
--
2.20.1
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH 2/3] Revert "riscv/efistub: Tighten ELF relocation check"
@ 2024-03-06 8:56 ` Yunhui Cui
0 siblings, 0 replies; 48+ messages in thread
From: Yunhui Cui @ 2024-03-06 8:56 UTC (permalink / raw)
To: paul.walmsley, palmer, aou, ardb, cuiyunhui, xuzhipeng.1973,
alexghiti, samitolvanen, bp, xiao.w.wang, jan.kiszka,
kirill.shutemov, nathan, linux-riscv, linux-kernel, linux-efi
This reverts commit d2baf8cc82c17459fca019a12348efcf86bfec29.
Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
---
drivers/firmware/efi/libstub/Makefile | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
index 475f37796779..a223bd10564b 100644
--- a/drivers/firmware/efi/libstub/Makefile
+++ b/drivers/firmware/efi/libstub/Makefile
@@ -143,7 +143,7 @@ STUBCOPY_RELOC-$(CONFIG_ARM64) := R_AARCH64_ABS
# exist.
STUBCOPY_FLAGS-$(CONFIG_RISCV) += --prefix-alloc-sections=.init \
--prefix-symbols=__efistub_
-STUBCOPY_RELOC-$(CONFIG_RISCV) := -E R_RISCV_HI20\|R_RISCV_$(BITS)\|R_RISCV_RELAX
+STUBCOPY_RELOC-$(CONFIG_RISCV) := R_RISCV_HI20
# For LoongArch, keep all the symbols in .init section and make sure that no
# absolute symbols references exist.
--
2.20.1
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH 3/3] efistub: fix missed the initialization of gp
2024-03-06 8:56 ` Yunhui Cui
@ 2024-03-06 8:56 ` Yunhui Cui
-1 siblings, 0 replies; 48+ messages in thread
From: Yunhui Cui @ 2024-03-06 8:56 UTC (permalink / raw)
To: paul.walmsley, palmer, aou, ardb, cuiyunhui, xuzhipeng.1973,
alexghiti, samitolvanen, bp, xiao.w.wang, jan.kiszka,
kirill.shutemov, nathan, linux-riscv, linux-kernel, linux-efi
Compared with gcc version 12, gcc version 13 uses the gp
register for compilation optimization, but the efistub module
does not initialize gp.
Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
Co-Developed-by: Zhipeng Xu <xuzhipeng.1973@bytedance.com>
---
| 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
--git a/arch/riscv/kernel/efi-header.S b/arch/riscv/kernel/efi-header.S
index 515b2dfbca75..fa17c08c092a 100644
--- a/arch/riscv/kernel/efi-header.S
+++ b/arch/riscv/kernel/efi-header.S
@@ -40,7 +40,7 @@ optional_header:
.long __pecoff_data_virt_end - __pecoff_text_end // SizeOfInitializedData
#endif
.long 0 // SizeOfUninitializedData
- .long __efistub_efi_pe_entry - _start // AddressOfEntryPoint
+ .long _efistub_entry - _start // AddressOfEntryPoint
.long efi_header_end - _start // BaseOfCode
#ifdef CONFIG_32BIT
.long __pecoff_text_end - _start // BaseOfData
@@ -121,4 +121,13 @@ section_table:
.balign 0x1000
efi_header_end:
+
+ .global _efistub_entry
+_efistub_entry:
+ /* Reload the global pointer */
+ load_global_pointer
+
+ call __efistub_efi_pe_entry
+ ret
+
.endm
--
2.20.1
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH 3/3] efistub: fix missed the initialization of gp
@ 2024-03-06 8:56 ` Yunhui Cui
0 siblings, 0 replies; 48+ messages in thread
From: Yunhui Cui @ 2024-03-06 8:56 UTC (permalink / raw)
To: paul.walmsley, palmer, aou, ardb, cuiyunhui, xuzhipeng.1973,
alexghiti, samitolvanen, bp, xiao.w.wang, jan.kiszka,
kirill.shutemov, nathan, linux-riscv, linux-kernel, linux-efi
Compared with gcc version 12, gcc version 13 uses the gp
register for compilation optimization, but the efistub module
does not initialize gp.
Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
Co-Developed-by: Zhipeng Xu <xuzhipeng.1973@bytedance.com>
---
| 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
--git a/arch/riscv/kernel/efi-header.S b/arch/riscv/kernel/efi-header.S
index 515b2dfbca75..fa17c08c092a 100644
--- a/arch/riscv/kernel/efi-header.S
+++ b/arch/riscv/kernel/efi-header.S
@@ -40,7 +40,7 @@ optional_header:
.long __pecoff_data_virt_end - __pecoff_text_end // SizeOfInitializedData
#endif
.long 0 // SizeOfUninitializedData
- .long __efistub_efi_pe_entry - _start // AddressOfEntryPoint
+ .long _efistub_entry - _start // AddressOfEntryPoint
.long efi_header_end - _start // BaseOfCode
#ifdef CONFIG_32BIT
.long __pecoff_text_end - _start // BaseOfData
@@ -121,4 +121,13 @@ section_table:
.balign 0x1000
efi_header_end:
+
+ .global _efistub_entry
+_efistub_entry:
+ /* Reload the global pointer */
+ load_global_pointer
+
+ call __efistub_efi_pe_entry
+ ret
+
.endm
--
2.20.1
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply related [flat|nested] 48+ messages in thread
* Re: [PATCH 1/3] Revert "riscv/efistub: Ensure GP-relative addressing is not used"
2024-03-06 8:56 ` Yunhui Cui
@ 2024-03-06 9:02 ` Conor Dooley
-1 siblings, 0 replies; 48+ messages in thread
From: Conor Dooley @ 2024-03-06 9:02 UTC (permalink / raw)
To: Yunhui Cui
Cc: paul.walmsley, palmer, aou, ardb, xuzhipeng.1973, alexghiti,
samitolvanen, bp, xiao.w.wang, jan.kiszka, kirill.shutemov,
nathan, linux-riscv, linux-kernel, linux-efi
[-- Attachment #1: Type: text/plain, Size: 1135 bytes --]
On Wed, Mar 06, 2024 at 04:56:20PM +0800, Yunhui Cui wrote:
> This reverts commit afb2a4fb84555ef9e61061f6ea63ed7087b295d5.
>
> Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
Reverts are commits too. You still need to write commit messages
justifying the revert.
Thanks,
Conor.
> ---
> drivers/firmware/efi/libstub/Makefile | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> index 31eb1e287ce1..475f37796779 100644
> --- a/drivers/firmware/efi/libstub/Makefile
> +++ b/drivers/firmware/efi/libstub/Makefile
> @@ -28,7 +28,7 @@ cflags-$(CONFIG_ARM) += -DEFI_HAVE_STRLEN -DEFI_HAVE_STRNLEN \
> -DEFI_HAVE_MEMCHR -DEFI_HAVE_STRRCHR \
> -DEFI_HAVE_STRCMP -fno-builtin -fpic \
> $(call cc-option,-mno-single-pic-base)
> -cflags-$(CONFIG_RISCV) += -fpic -DNO_ALTERNATIVE -mno-relax
> +cflags-$(CONFIG_RISCV) += -fpic -DNO_ALTERNATIVE
> cflags-$(CONFIG_LOONGARCH) += -fpie
>
> cflags-$(CONFIG_EFI_PARAMS_FROM_FDT) += -I$(srctree)/scripts/dtc/libfdt
> --
> 2.20.1
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 1/3] Revert "riscv/efistub: Ensure GP-relative addressing is not used"
@ 2024-03-06 9:02 ` Conor Dooley
0 siblings, 0 replies; 48+ messages in thread
From: Conor Dooley @ 2024-03-06 9:02 UTC (permalink / raw)
To: Yunhui Cui
Cc: paul.walmsley, palmer, aou, ardb, xuzhipeng.1973, alexghiti,
samitolvanen, bp, xiao.w.wang, jan.kiszka, kirill.shutemov,
nathan, linux-riscv, linux-kernel, linux-efi
[-- Attachment #1.1: Type: text/plain, Size: 1135 bytes --]
On Wed, Mar 06, 2024 at 04:56:20PM +0800, Yunhui Cui wrote:
> This reverts commit afb2a4fb84555ef9e61061f6ea63ed7087b295d5.
>
> Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
Reverts are commits too. You still need to write commit messages
justifying the revert.
Thanks,
Conor.
> ---
> drivers/firmware/efi/libstub/Makefile | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> index 31eb1e287ce1..475f37796779 100644
> --- a/drivers/firmware/efi/libstub/Makefile
> +++ b/drivers/firmware/efi/libstub/Makefile
> @@ -28,7 +28,7 @@ cflags-$(CONFIG_ARM) += -DEFI_HAVE_STRLEN -DEFI_HAVE_STRNLEN \
> -DEFI_HAVE_MEMCHR -DEFI_HAVE_STRRCHR \
> -DEFI_HAVE_STRCMP -fno-builtin -fpic \
> $(call cc-option,-mno-single-pic-base)
> -cflags-$(CONFIG_RISCV) += -fpic -DNO_ALTERNATIVE -mno-relax
> +cflags-$(CONFIG_RISCV) += -fpic -DNO_ALTERNATIVE
> cflags-$(CONFIG_LOONGARCH) += -fpie
>
> cflags-$(CONFIG_EFI_PARAMS_FROM_FDT) += -I$(srctree)/scripts/dtc/libfdt
> --
> 2.20.1
>
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]
[-- Attachment #2: Type: text/plain, Size: 161 bytes --]
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 3/3] efistub: fix missed the initialization of gp
2024-03-06 8:56 ` Yunhui Cui
@ 2024-03-06 9:36 ` Ard Biesheuvel
-1 siblings, 0 replies; 48+ messages in thread
From: Ard Biesheuvel @ 2024-03-06 9:36 UTC (permalink / raw)
To: Yunhui Cui
Cc: paul.walmsley, palmer, aou, xuzhipeng.1973, alexghiti,
samitolvanen, bp, xiao.w.wang, jan.kiszka, kirill.shutemov,
nathan, linux-riscv, linux-kernel, linux-efi
On Wed, 6 Mar 2024 at 09:56, Yunhui Cui <cuiyunhui@bytedance.com> wrote:
>
> Compared with gcc version 12, gcc version 13 uses the gp
> register for compilation optimization, but the efistub module
> does not initialize gp.
>
> Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> Co-Developed-by: Zhipeng Xu <xuzhipeng.1973@bytedance.com>
This needs a sign-off, and your signoff needs to come after.
> ---
> arch/riscv/kernel/efi-header.S | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/arch/riscv/kernel/efi-header.S b/arch/riscv/kernel/efi-header.S
> index 515b2dfbca75..fa17c08c092a 100644
> --- a/arch/riscv/kernel/efi-header.S
> +++ b/arch/riscv/kernel/efi-header.S
> @@ -40,7 +40,7 @@ optional_header:
> .long __pecoff_data_virt_end - __pecoff_text_end // SizeOfInitializedData
> #endif
> .long 0 // SizeOfUninitializedData
> - .long __efistub_efi_pe_entry - _start // AddressOfEntryPoint
> + .long _efistub_entry - _start // AddressOfEntryPoint
> .long efi_header_end - _start // BaseOfCode
> #ifdef CONFIG_32BIT
> .long __pecoff_text_end - _start // BaseOfData
> @@ -121,4 +121,13 @@ section_table:
>
> .balign 0x1000
> efi_header_end:
> +
> + .global _efistub_entry
> +_efistub_entry:
This should go into .text or .init.text, not the header.
> + /* Reload the global pointer */
> + load_global_pointer
> +
What is supposed to happen here if CONFIG_SHADOW_CALL_STACK=y? The EFI
stub Makefile removes the SCS CFLAGS, so the stub will be built
without shadow call stack support, which I guess means that it might
use GP as a global pointer as usual?
> + call __efistub_efi_pe_entry
> + ret
> +
You are returning to the firmware here, but after modifying the GP
register. Shouldn't you restore it to its old value?
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 3/3] efistub: fix missed the initialization of gp
@ 2024-03-06 9:36 ` Ard Biesheuvel
0 siblings, 0 replies; 48+ messages in thread
From: Ard Biesheuvel @ 2024-03-06 9:36 UTC (permalink / raw)
To: Yunhui Cui
Cc: paul.walmsley, palmer, aou, xuzhipeng.1973, alexghiti,
samitolvanen, bp, xiao.w.wang, jan.kiszka, kirill.shutemov,
nathan, linux-riscv, linux-kernel, linux-efi
On Wed, 6 Mar 2024 at 09:56, Yunhui Cui <cuiyunhui@bytedance.com> wrote:
>
> Compared with gcc version 12, gcc version 13 uses the gp
> register for compilation optimization, but the efistub module
> does not initialize gp.
>
> Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> Co-Developed-by: Zhipeng Xu <xuzhipeng.1973@bytedance.com>
This needs a sign-off, and your signoff needs to come after.
> ---
> arch/riscv/kernel/efi-header.S | 11 ++++++++++-
> 1 file changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/arch/riscv/kernel/efi-header.S b/arch/riscv/kernel/efi-header.S
> index 515b2dfbca75..fa17c08c092a 100644
> --- a/arch/riscv/kernel/efi-header.S
> +++ b/arch/riscv/kernel/efi-header.S
> @@ -40,7 +40,7 @@ optional_header:
> .long __pecoff_data_virt_end - __pecoff_text_end // SizeOfInitializedData
> #endif
> .long 0 // SizeOfUninitializedData
> - .long __efistub_efi_pe_entry - _start // AddressOfEntryPoint
> + .long _efistub_entry - _start // AddressOfEntryPoint
> .long efi_header_end - _start // BaseOfCode
> #ifdef CONFIG_32BIT
> .long __pecoff_text_end - _start // BaseOfData
> @@ -121,4 +121,13 @@ section_table:
>
> .balign 0x1000
> efi_header_end:
> +
> + .global _efistub_entry
> +_efistub_entry:
This should go into .text or .init.text, not the header.
> + /* Reload the global pointer */
> + load_global_pointer
> +
What is supposed to happen here if CONFIG_SHADOW_CALL_STACK=y? The EFI
stub Makefile removes the SCS CFLAGS, so the stub will be built
without shadow call stack support, which I guess means that it might
use GP as a global pointer as usual?
> + call __efistub_efi_pe_entry
> + ret
> +
You are returning to the firmware here, but after modifying the GP
register. Shouldn't you restore it to its old value?
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 1/3] Revert "riscv/efistub: Ensure GP-relative addressing is not used"
2024-03-06 9:02 ` Conor Dooley
@ 2024-03-06 9:38 ` Ard Biesheuvel
-1 siblings, 0 replies; 48+ messages in thread
From: Ard Biesheuvel @ 2024-03-06 9:38 UTC (permalink / raw)
To: Conor Dooley
Cc: Yunhui Cui, paul.walmsley, palmer, aou, xuzhipeng.1973,
alexghiti, samitolvanen, bp, xiao.w.wang, jan.kiszka,
kirill.shutemov, nathan, linux-riscv, linux-kernel, linux-efi
On Wed, 6 Mar 2024 at 10:03, Conor Dooley <conor.dooley@microchip.com> wrote:
>
> On Wed, Mar 06, 2024 at 04:56:20PM +0800, Yunhui Cui wrote:
> > This reverts commit afb2a4fb84555ef9e61061f6ea63ed7087b295d5.
> >
> > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
>
> Reverts are commits too. You still need to write commit messages
> justifying the revert.
>
Indeed. Also, please revert them in the right [reverse] order so we
don't inadvertently break bisect.
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 1/3] Revert "riscv/efistub: Ensure GP-relative addressing is not used"
@ 2024-03-06 9:38 ` Ard Biesheuvel
0 siblings, 0 replies; 48+ messages in thread
From: Ard Biesheuvel @ 2024-03-06 9:38 UTC (permalink / raw)
To: Conor Dooley
Cc: Yunhui Cui, paul.walmsley, palmer, aou, xuzhipeng.1973,
alexghiti, samitolvanen, bp, xiao.w.wang, jan.kiszka,
kirill.shutemov, nathan, linux-riscv, linux-kernel, linux-efi
On Wed, 6 Mar 2024 at 10:03, Conor Dooley <conor.dooley@microchip.com> wrote:
>
> On Wed, Mar 06, 2024 at 04:56:20PM +0800, Yunhui Cui wrote:
> > This reverts commit afb2a4fb84555ef9e61061f6ea63ed7087b295d5.
> >
> > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
>
> Reverts are commits too. You still need to write commit messages
> justifying the revert.
>
Indeed. Also, please revert them in the right [reverse] order so we
don't inadvertently break bisect.
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [External] Re: [PATCH 3/3] efistub: fix missed the initialization of gp
2024-03-06 9:36 ` Ard Biesheuvel
@ 2024-03-06 12:34 ` yunhui cui
-1 siblings, 0 replies; 48+ messages in thread
From: yunhui cui @ 2024-03-06 12:34 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: paul.walmsley, palmer, aou, xuzhipeng.1973, alexghiti,
samitolvanen, bp, xiao.w.wang, jan.kiszka, kirill.shutemov,
nathan, linux-riscv, linux-kernel, linux-efi
Hi Ard,
On Wed, Mar 6, 2024 at 5:36 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 6 Mar 2024 at 09:56, Yunhui Cui <cuiyunhui@bytedance.com> wrote:
> >
> > Compared with gcc version 12, gcc version 13 uses the gp
> > register for compilation optimization, but the efistub module
> > does not initialize gp.
> >
> > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> > Co-Developed-by: Zhipeng Xu <xuzhipeng.1973@bytedance.com>
>
> This needs a sign-off, and your signoff needs to come after.
>
> > ---
> > arch/riscv/kernel/efi-header.S | 11 ++++++++++-
> > 1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/riscv/kernel/efi-header.S b/arch/riscv/kernel/efi-header.S
> > index 515b2dfbca75..fa17c08c092a 100644
> > --- a/arch/riscv/kernel/efi-header.S
> > +++ b/arch/riscv/kernel/efi-header.S
> > @@ -40,7 +40,7 @@ optional_header:
> > .long __pecoff_data_virt_end - __pecoff_text_end // SizeOfInitializedData
> > #endif
> > .long 0 // SizeOfUninitializedData
> > - .long __efistub_efi_pe_entry - _start // AddressOfEntryPoint
> > + .long _efistub_entry - _start // AddressOfEntryPoint
> > .long efi_header_end - _start // BaseOfCode
> > #ifdef CONFIG_32BIT
> > .long __pecoff_text_end - _start // BaseOfData
> > @@ -121,4 +121,13 @@ section_table:
> >
> > .balign 0x1000
> > efi_header_end:
> > +
> > + .global _efistub_entry
> > +_efistub_entry:
>
> This should go into .text or .init.text, not the header.
>
> > + /* Reload the global pointer */
> > + load_global_pointer
> > +
>
> What is supposed to happen here if CONFIG_SHADOW_CALL_STACK=y? The EFI
> stub Makefile removes the SCS CFLAGS, so the stub will be built
> without shadow call stack support, which I guess means that it might
> use GP as a global pointer as usual?
>
> > + call __efistub_efi_pe_entry
> > + ret
> > +
>
> You are returning to the firmware here, but after modifying the GP
> register. Shouldn't you restore it to its old value?
There is no need to restore the value of the gp register. Where gp is
needed, the gp register must first be initialized. And here is the
entry.
Regarding your first two comments above, I plan to make the following
changes in v2,
efi_header_end:
+
+ __INIT
+ .global _efistub_entry
+_efistub_entry:
+ /* Reload the global pointer */
+.option push
+.option norelax
+ la gp, __global_pointer$
+.option pop
+
+ call __efistub_efi_pe_entry
+ ret
+ __HEAD
+
.endm
what do you think?
Thanks,
Yunhui
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [External] Re: [PATCH 3/3] efistub: fix missed the initialization of gp
@ 2024-03-06 12:34 ` yunhui cui
0 siblings, 0 replies; 48+ messages in thread
From: yunhui cui @ 2024-03-06 12:34 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: paul.walmsley, palmer, aou, xuzhipeng.1973, alexghiti,
samitolvanen, bp, xiao.w.wang, jan.kiszka, kirill.shutemov,
nathan, linux-riscv, linux-kernel, linux-efi
Hi Ard,
On Wed, Mar 6, 2024 at 5:36 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 6 Mar 2024 at 09:56, Yunhui Cui <cuiyunhui@bytedance.com> wrote:
> >
> > Compared with gcc version 12, gcc version 13 uses the gp
> > register for compilation optimization, but the efistub module
> > does not initialize gp.
> >
> > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> > Co-Developed-by: Zhipeng Xu <xuzhipeng.1973@bytedance.com>
>
> This needs a sign-off, and your signoff needs to come after.
>
> > ---
> > arch/riscv/kernel/efi-header.S | 11 ++++++++++-
> > 1 file changed, 10 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/riscv/kernel/efi-header.S b/arch/riscv/kernel/efi-header.S
> > index 515b2dfbca75..fa17c08c092a 100644
> > --- a/arch/riscv/kernel/efi-header.S
> > +++ b/arch/riscv/kernel/efi-header.S
> > @@ -40,7 +40,7 @@ optional_header:
> > .long __pecoff_data_virt_end - __pecoff_text_end // SizeOfInitializedData
> > #endif
> > .long 0 // SizeOfUninitializedData
> > - .long __efistub_efi_pe_entry - _start // AddressOfEntryPoint
> > + .long _efistub_entry - _start // AddressOfEntryPoint
> > .long efi_header_end - _start // BaseOfCode
> > #ifdef CONFIG_32BIT
> > .long __pecoff_text_end - _start // BaseOfData
> > @@ -121,4 +121,13 @@ section_table:
> >
> > .balign 0x1000
> > efi_header_end:
> > +
> > + .global _efistub_entry
> > +_efistub_entry:
>
> This should go into .text or .init.text, not the header.
>
> > + /* Reload the global pointer */
> > + load_global_pointer
> > +
>
> What is supposed to happen here if CONFIG_SHADOW_CALL_STACK=y? The EFI
> stub Makefile removes the SCS CFLAGS, so the stub will be built
> without shadow call stack support, which I guess means that it might
> use GP as a global pointer as usual?
>
> > + call __efistub_efi_pe_entry
> > + ret
> > +
>
> You are returning to the firmware here, but after modifying the GP
> register. Shouldn't you restore it to its old value?
There is no need to restore the value of the gp register. Where gp is
needed, the gp register must first be initialized. And here is the
entry.
Regarding your first two comments above, I plan to make the following
changes in v2,
efi_header_end:
+
+ __INIT
+ .global _efistub_entry
+_efistub_entry:
+ /* Reload the global pointer */
+.option push
+.option norelax
+ la gp, __global_pointer$
+.option pop
+
+ call __efistub_efi_pe_entry
+ ret
+ __HEAD
+
.endm
what do you think?
Thanks,
Yunhui
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [External] Re: [PATCH 1/3] Revert "riscv/efistub: Ensure GP-relative addressing is not used"
2024-03-06 9:38 ` Ard Biesheuvel
@ 2024-03-06 12:37 ` yunhui cui
-1 siblings, 0 replies; 48+ messages in thread
From: yunhui cui @ 2024-03-06 12:37 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Conor Dooley, paul.walmsley, palmer, aou, xuzhipeng.1973,
alexghiti, samitolvanen, bp, xiao.w.wang, jan.kiszka,
kirill.shutemov, nathan, linux-riscv, linux-kernel, linux-efi
Hi Ard, Conor,
On Wed, Mar 6, 2024 at 5:38 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 6 Mar 2024 at 10:03, Conor Dooley <conor.dooley@microchip.com> wrote:
> >
> > On Wed, Mar 06, 2024 at 04:56:20PM +0800, Yunhui Cui wrote:
> > > This reverts commit afb2a4fb84555ef9e61061f6ea63ed7087b295d5.
> > >
> > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> >
> > Reverts are commits too. You still need to write commit messages
> > justifying the revert.
> >
>
> Indeed. Also, please revert them in the right [reverse] order so we
> don't inadvertently break bisect.
Okay, I'll update it in v2.
Thanks,
Yunhui
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [External] Re: [PATCH 1/3] Revert "riscv/efistub: Ensure GP-relative addressing is not used"
@ 2024-03-06 12:37 ` yunhui cui
0 siblings, 0 replies; 48+ messages in thread
From: yunhui cui @ 2024-03-06 12:37 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Conor Dooley, paul.walmsley, palmer, aou, xuzhipeng.1973,
alexghiti, samitolvanen, bp, xiao.w.wang, jan.kiszka,
kirill.shutemov, nathan, linux-riscv, linux-kernel, linux-efi
Hi Ard, Conor,
On Wed, Mar 6, 2024 at 5:38 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 6 Mar 2024 at 10:03, Conor Dooley <conor.dooley@microchip.com> wrote:
> >
> > On Wed, Mar 06, 2024 at 04:56:20PM +0800, Yunhui Cui wrote:
> > > This reverts commit afb2a4fb84555ef9e61061f6ea63ed7087b295d5.
> > >
> > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> >
> > Reverts are commits too. You still need to write commit messages
> > justifying the revert.
> >
>
> Indeed. Also, please revert them in the right [reverse] order so we
> don't inadvertently break bisect.
Okay, I'll update it in v2.
Thanks,
Yunhui
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 1/3] Revert "riscv/efistub: Ensure GP-relative addressing is not used"
2024-03-06 8:56 ` Yunhui Cui
@ 2024-03-06 12:52 ` Jan Kiszka
-1 siblings, 0 replies; 48+ messages in thread
From: Jan Kiszka @ 2024-03-06 12:52 UTC (permalink / raw)
To: Yunhui Cui, paul.walmsley, palmer, aou, ardb, xuzhipeng.1973,
alexghiti, samitolvanen, bp, xiao.w.wang, kirill.shutemov,
nathan, linux-riscv, linux-kernel, linux-efi
On 06.03.24 09:56, Yunhui Cui wrote:
> This reverts commit afb2a4fb84555ef9e61061f6ea63ed7087b295d5.
>
This comes without a reason - which is likely something around "will fix
this properly later". But then you regress first and only fix
afterwards. Can't that be done the other way around?
Jan
> Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> ---
> drivers/firmware/efi/libstub/Makefile | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> index 31eb1e287ce1..475f37796779 100644
> --- a/drivers/firmware/efi/libstub/Makefile
> +++ b/drivers/firmware/efi/libstub/Makefile
> @@ -28,7 +28,7 @@ cflags-$(CONFIG_ARM) += -DEFI_HAVE_STRLEN -DEFI_HAVE_STRNLEN \
> -DEFI_HAVE_MEMCHR -DEFI_HAVE_STRRCHR \
> -DEFI_HAVE_STRCMP -fno-builtin -fpic \
> $(call cc-option,-mno-single-pic-base)
> -cflags-$(CONFIG_RISCV) += -fpic -DNO_ALTERNATIVE -mno-relax
> +cflags-$(CONFIG_RISCV) += -fpic -DNO_ALTERNATIVE
> cflags-$(CONFIG_LOONGARCH) += -fpie
>
> cflags-$(CONFIG_EFI_PARAMS_FROM_FDT) += -I$(srctree)/scripts/dtc/libfdt
--
Siemens AG, Technology
Linux Expert Center
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 1/3] Revert "riscv/efistub: Ensure GP-relative addressing is not used"
@ 2024-03-06 12:52 ` Jan Kiszka
0 siblings, 0 replies; 48+ messages in thread
From: Jan Kiszka @ 2024-03-06 12:52 UTC (permalink / raw)
To: Yunhui Cui, paul.walmsley, palmer, aou, ardb, xuzhipeng.1973,
alexghiti, samitolvanen, bp, xiao.w.wang, kirill.shutemov,
nathan, linux-riscv, linux-kernel, linux-efi
On 06.03.24 09:56, Yunhui Cui wrote:
> This reverts commit afb2a4fb84555ef9e61061f6ea63ed7087b295d5.
>
This comes without a reason - which is likely something around "will fix
this properly later". But then you regress first and only fix
afterwards. Can't that be done the other way around?
Jan
> Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> ---
> drivers/firmware/efi/libstub/Makefile | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> index 31eb1e287ce1..475f37796779 100644
> --- a/drivers/firmware/efi/libstub/Makefile
> +++ b/drivers/firmware/efi/libstub/Makefile
> @@ -28,7 +28,7 @@ cflags-$(CONFIG_ARM) += -DEFI_HAVE_STRLEN -DEFI_HAVE_STRNLEN \
> -DEFI_HAVE_MEMCHR -DEFI_HAVE_STRRCHR \
> -DEFI_HAVE_STRCMP -fno-builtin -fpic \
> $(call cc-option,-mno-single-pic-base)
> -cflags-$(CONFIG_RISCV) += -fpic -DNO_ALTERNATIVE -mno-relax
> +cflags-$(CONFIG_RISCV) += -fpic -DNO_ALTERNATIVE
> cflags-$(CONFIG_LOONGARCH) += -fpie
>
> cflags-$(CONFIG_EFI_PARAMS_FROM_FDT) += -I$(srctree)/scripts/dtc/libfdt
--
Siemens AG, Technology
Linux Expert Center
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [External] Re: [PATCH 3/3] efistub: fix missed the initialization of gp
2024-03-06 12:34 ` yunhui cui
@ 2024-03-06 13:02 ` Ard Biesheuvel
-1 siblings, 0 replies; 48+ messages in thread
From: Ard Biesheuvel @ 2024-03-06 13:02 UTC (permalink / raw)
To: yunhui cui
Cc: paul.walmsley, palmer, aou, xuzhipeng.1973, alexghiti,
samitolvanen, bp, xiao.w.wang, jan.kiszka, kirill.shutemov,
nathan, linux-riscv, linux-kernel, linux-efi
On Wed, 6 Mar 2024 at 13:34, yunhui cui <cuiyunhui@bytedance.com> wrote:
>
> Hi Ard,
>
> On Wed, Mar 6, 2024 at 5:36 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Wed, 6 Mar 2024 at 09:56, Yunhui Cui <cuiyunhui@bytedance.com> wrote:
> > >
> > > Compared with gcc version 12, gcc version 13 uses the gp
> > > register for compilation optimization, but the efistub module
> > > does not initialize gp.
> > >
> > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> > > Co-Developed-by: Zhipeng Xu <xuzhipeng.1973@bytedance.com>
> >
> > This needs a sign-off, and your signoff needs to come after.
> >
> > > ---
> > > arch/riscv/kernel/efi-header.S | 11 ++++++++++-
> > > 1 file changed, 10 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/riscv/kernel/efi-header.S b/arch/riscv/kernel/efi-header.S
> > > index 515b2dfbca75..fa17c08c092a 100644
> > > --- a/arch/riscv/kernel/efi-header.S
> > > +++ b/arch/riscv/kernel/efi-header.S
> > > @@ -40,7 +40,7 @@ optional_header:
> > > .long __pecoff_data_virt_end - __pecoff_text_end // SizeOfInitializedData
> > > #endif
> > > .long 0 // SizeOfUninitializedData
> > > - .long __efistub_efi_pe_entry - _start // AddressOfEntryPoint
> > > + .long _efistub_entry - _start // AddressOfEntryPoint
> > > .long efi_header_end - _start // BaseOfCode
> > > #ifdef CONFIG_32BIT
> > > .long __pecoff_text_end - _start // BaseOfData
> > > @@ -121,4 +121,13 @@ section_table:
> > >
> > > .balign 0x1000
> > > efi_header_end:
> > > +
> > > + .global _efistub_entry
> > > +_efistub_entry:
> >
> > This should go into .text or .init.text, not the header.
> >
> > > + /* Reload the global pointer */
> > > + load_global_pointer
> > > +
> >
> > What is supposed to happen here if CONFIG_SHADOW_CALL_STACK=y? The EFI
> > stub Makefile removes the SCS CFLAGS, so the stub will be built
> > without shadow call stack support, which I guess means that it might
> > use GP as a global pointer as usual?
> >
> > > + call __efistub_efi_pe_entry
> > > + ret
> > > +
> >
> > You are returning to the firmware here, but after modifying the GP
> > register. Shouldn't you restore it to its old value?
> There is no need to restore the value of the gp register. Where gp is
> needed, the gp register must first be initialized. And here is the
> entry.
>
But how should the firmware know that GP was corrupted after calling
the kernel's EFI entrypoint? The EFI stub can return to the firmware
if it encounters any errors while still running in the EFI boot
services.
> Regarding your first two comments above, I plan to make the following
> changes in v2,
> efi_header_end:
> +
> + __INIT
> + .global _efistub_entry
> +_efistub_entry:
> + /* Reload the global pointer */
> +.option push
> +.option norelax
> + la gp, __global_pointer$
> +.option pop
> +
> + call __efistub_efi_pe_entry
> + ret
> + __HEAD
> +
> .endm
>
> what do you think?
>
This looks ok to me, but I would still like to understand why it is ok
to return to the firmware with a modified GP value.
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [External] Re: [PATCH 3/3] efistub: fix missed the initialization of gp
@ 2024-03-06 13:02 ` Ard Biesheuvel
0 siblings, 0 replies; 48+ messages in thread
From: Ard Biesheuvel @ 2024-03-06 13:02 UTC (permalink / raw)
To: yunhui cui
Cc: paul.walmsley, palmer, aou, xuzhipeng.1973, alexghiti,
samitolvanen, bp, xiao.w.wang, jan.kiszka, kirill.shutemov,
nathan, linux-riscv, linux-kernel, linux-efi
On Wed, 6 Mar 2024 at 13:34, yunhui cui <cuiyunhui@bytedance.com> wrote:
>
> Hi Ard,
>
> On Wed, Mar 6, 2024 at 5:36 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Wed, 6 Mar 2024 at 09:56, Yunhui Cui <cuiyunhui@bytedance.com> wrote:
> > >
> > > Compared with gcc version 12, gcc version 13 uses the gp
> > > register for compilation optimization, but the efistub module
> > > does not initialize gp.
> > >
> > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> > > Co-Developed-by: Zhipeng Xu <xuzhipeng.1973@bytedance.com>
> >
> > This needs a sign-off, and your signoff needs to come after.
> >
> > > ---
> > > arch/riscv/kernel/efi-header.S | 11 ++++++++++-
> > > 1 file changed, 10 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/arch/riscv/kernel/efi-header.S b/arch/riscv/kernel/efi-header.S
> > > index 515b2dfbca75..fa17c08c092a 100644
> > > --- a/arch/riscv/kernel/efi-header.S
> > > +++ b/arch/riscv/kernel/efi-header.S
> > > @@ -40,7 +40,7 @@ optional_header:
> > > .long __pecoff_data_virt_end - __pecoff_text_end // SizeOfInitializedData
> > > #endif
> > > .long 0 // SizeOfUninitializedData
> > > - .long __efistub_efi_pe_entry - _start // AddressOfEntryPoint
> > > + .long _efistub_entry - _start // AddressOfEntryPoint
> > > .long efi_header_end - _start // BaseOfCode
> > > #ifdef CONFIG_32BIT
> > > .long __pecoff_text_end - _start // BaseOfData
> > > @@ -121,4 +121,13 @@ section_table:
> > >
> > > .balign 0x1000
> > > efi_header_end:
> > > +
> > > + .global _efistub_entry
> > > +_efistub_entry:
> >
> > This should go into .text or .init.text, not the header.
> >
> > > + /* Reload the global pointer */
> > > + load_global_pointer
> > > +
> >
> > What is supposed to happen here if CONFIG_SHADOW_CALL_STACK=y? The EFI
> > stub Makefile removes the SCS CFLAGS, so the stub will be built
> > without shadow call stack support, which I guess means that it might
> > use GP as a global pointer as usual?
> >
> > > + call __efistub_efi_pe_entry
> > > + ret
> > > +
> >
> > You are returning to the firmware here, but after modifying the GP
> > register. Shouldn't you restore it to its old value?
> There is no need to restore the value of the gp register. Where gp is
> needed, the gp register must first be initialized. And here is the
> entry.
>
But how should the firmware know that GP was corrupted after calling
the kernel's EFI entrypoint? The EFI stub can return to the firmware
if it encounters any errors while still running in the EFI boot
services.
> Regarding your first two comments above, I plan to make the following
> changes in v2,
> efi_header_end:
> +
> + __INIT
> + .global _efistub_entry
> +_efistub_entry:
> + /* Reload the global pointer */
> +.option push
> +.option norelax
> + la gp, __global_pointer$
> +.option pop
> +
> + call __efistub_efi_pe_entry
> + ret
> + __HEAD
> +
> .endm
>
> what do you think?
>
This looks ok to me, but I would still like to understand why it is ok
to return to the firmware with a modified GP value.
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [External] Re: [PATCH 1/3] Revert "riscv/efistub: Ensure GP-relative addressing is not used"
2024-03-06 12:52 ` Jan Kiszka
@ 2024-03-06 13:07 ` yunhui cui
-1 siblings, 0 replies; 48+ messages in thread
From: yunhui cui @ 2024-03-06 13:07 UTC (permalink / raw)
To: Jan Kiszka
Cc: paul.walmsley, palmer, aou, ardb, xuzhipeng.1973, alexghiti,
samitolvanen, bp, xiao.w.wang, kirill.shutemov, nathan,
linux-riscv, linux-kernel, linux-efi
Hi Jan,
On Wed, Mar 6, 2024 at 8:52 PM Jan Kiszka <jan.kiszka@siemens.com> wrote:
>
> On 06.03.24 09:56, Yunhui Cui wrote:
> > This reverts commit afb2a4fb84555ef9e61061f6ea63ed7087b295d5.
> >
>
> This comes without a reason - which is likely something around "will fix
> this properly later". But then you regress first and only fix
> afterwards. Can't that be done the other way around?
Sorry, I don't quite understand what you mean. Can you help explain it
more clearly? Do you mean "delete mno-relax instead of revert
directly?"
Thanks,
Yunhui
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [External] Re: [PATCH 1/3] Revert "riscv/efistub: Ensure GP-relative addressing is not used"
@ 2024-03-06 13:07 ` yunhui cui
0 siblings, 0 replies; 48+ messages in thread
From: yunhui cui @ 2024-03-06 13:07 UTC (permalink / raw)
To: Jan Kiszka
Cc: paul.walmsley, palmer, aou, ardb, xuzhipeng.1973, alexghiti,
samitolvanen, bp, xiao.w.wang, kirill.shutemov, nathan,
linux-riscv, linux-kernel, linux-efi
Hi Jan,
On Wed, Mar 6, 2024 at 8:52 PM Jan Kiszka <jan.kiszka@siemens.com> wrote:
>
> On 06.03.24 09:56, Yunhui Cui wrote:
> > This reverts commit afb2a4fb84555ef9e61061f6ea63ed7087b295d5.
> >
>
> This comes without a reason - which is likely something around "will fix
> this properly later". But then you regress first and only fix
> afterwards. Can't that be done the other way around?
Sorry, I don't quite understand what you mean. Can you help explain it
more clearly? Do you mean "delete mno-relax instead of revert
directly?"
Thanks,
Yunhui
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [External] Re: [PATCH 3/3] efistub: fix missed the initialization of gp
2024-03-06 13:02 ` Ard Biesheuvel
@ 2024-03-06 13:09 ` Ard Biesheuvel
-1 siblings, 0 replies; 48+ messages in thread
From: Ard Biesheuvel @ 2024-03-06 13:09 UTC (permalink / raw)
To: yunhui cui
Cc: paul.walmsley, palmer, aou, xuzhipeng.1973, alexghiti,
samitolvanen, bp, xiao.w.wang, jan.kiszka, kirill.shutemov,
nathan, linux-riscv, linux-kernel, linux-efi
On Wed, 6 Mar 2024 at 14:02, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 6 Mar 2024 at 13:34, yunhui cui <cuiyunhui@bytedance.com> wrote:
> >
> > Hi Ard,
> >
> > On Wed, Mar 6, 2024 at 5:36 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> > >
> > > On Wed, 6 Mar 2024 at 09:56, Yunhui Cui <cuiyunhui@bytedance.com> wrote:
> > > >
> > > > Compared with gcc version 12, gcc version 13 uses the gp
> > > > register for compilation optimization, but the efistub module
> > > > does not initialize gp.
> > > >
> > > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> > > > Co-Developed-by: Zhipeng Xu <xuzhipeng.1973@bytedance.com>
> > >
> > > This needs a sign-off, and your signoff needs to come after.
> > >
> > > > ---
> > > > arch/riscv/kernel/efi-header.S | 11 ++++++++++-
> > > > 1 file changed, 10 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/arch/riscv/kernel/efi-header.S b/arch/riscv/kernel/efi-header.S
> > > > index 515b2dfbca75..fa17c08c092a 100644
> > > > --- a/arch/riscv/kernel/efi-header.S
> > > > +++ b/arch/riscv/kernel/efi-header.S
> > > > @@ -40,7 +40,7 @@ optional_header:
> > > > .long __pecoff_data_virt_end - __pecoff_text_end // SizeOfInitializedData
> > > > #endif
> > > > .long 0 // SizeOfUninitializedData
> > > > - .long __efistub_efi_pe_entry - _start // AddressOfEntryPoint
> > > > + .long _efistub_entry - _start // AddressOfEntryPoint
> > > > .long efi_header_end - _start // BaseOfCode
> > > > #ifdef CONFIG_32BIT
> > > > .long __pecoff_text_end - _start // BaseOfData
> > > > @@ -121,4 +121,13 @@ section_table:
> > > >
> > > > .balign 0x1000
> > > > efi_header_end:
> > > > +
> > > > + .global _efistub_entry
> > > > +_efistub_entry:
> > >
> > > This should go into .text or .init.text, not the header.
> > >
> > > > + /* Reload the global pointer */
> > > > + load_global_pointer
> > > > +
> > >
> > > What is supposed to happen here if CONFIG_SHADOW_CALL_STACK=y? The EFI
> > > stub Makefile removes the SCS CFLAGS, so the stub will be built
> > > without shadow call stack support, which I guess means that it might
> > > use GP as a global pointer as usual?
> > >
> > > > + call __efistub_efi_pe_entry
> > > > + ret
> > > > +
> > >
> > > You are returning to the firmware here, but after modifying the GP
> > > register. Shouldn't you restore it to its old value?
> > There is no need to restore the value of the gp register. Where gp is
> > needed, the gp register must first be initialized. And here is the
> > entry.
> >
>
> But how should the firmware know that GP was corrupted after calling
> the kernel's EFI entrypoint? The EFI stub can return to the firmware
> if it encounters any errors while still running in the EFI boot
> services.
>
Actually, I wonder if GP can be modified at all before
ExitBootServices(). The EFI timer interrupt is still live at this
point, and so the firmware is being called behind your back, and might
rely on GP retaining its original value.
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [External] Re: [PATCH 3/3] efistub: fix missed the initialization of gp
@ 2024-03-06 13:09 ` Ard Biesheuvel
0 siblings, 0 replies; 48+ messages in thread
From: Ard Biesheuvel @ 2024-03-06 13:09 UTC (permalink / raw)
To: yunhui cui
Cc: paul.walmsley, palmer, aou, xuzhipeng.1973, alexghiti,
samitolvanen, bp, xiao.w.wang, jan.kiszka, kirill.shutemov,
nathan, linux-riscv, linux-kernel, linux-efi
On Wed, 6 Mar 2024 at 14:02, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 6 Mar 2024 at 13:34, yunhui cui <cuiyunhui@bytedance.com> wrote:
> >
> > Hi Ard,
> >
> > On Wed, Mar 6, 2024 at 5:36 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> > >
> > > On Wed, 6 Mar 2024 at 09:56, Yunhui Cui <cuiyunhui@bytedance.com> wrote:
> > > >
> > > > Compared with gcc version 12, gcc version 13 uses the gp
> > > > register for compilation optimization, but the efistub module
> > > > does not initialize gp.
> > > >
> > > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> > > > Co-Developed-by: Zhipeng Xu <xuzhipeng.1973@bytedance.com>
> > >
> > > This needs a sign-off, and your signoff needs to come after.
> > >
> > > > ---
> > > > arch/riscv/kernel/efi-header.S | 11 ++++++++++-
> > > > 1 file changed, 10 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/arch/riscv/kernel/efi-header.S b/arch/riscv/kernel/efi-header.S
> > > > index 515b2dfbca75..fa17c08c092a 100644
> > > > --- a/arch/riscv/kernel/efi-header.S
> > > > +++ b/arch/riscv/kernel/efi-header.S
> > > > @@ -40,7 +40,7 @@ optional_header:
> > > > .long __pecoff_data_virt_end - __pecoff_text_end // SizeOfInitializedData
> > > > #endif
> > > > .long 0 // SizeOfUninitializedData
> > > > - .long __efistub_efi_pe_entry - _start // AddressOfEntryPoint
> > > > + .long _efistub_entry - _start // AddressOfEntryPoint
> > > > .long efi_header_end - _start // BaseOfCode
> > > > #ifdef CONFIG_32BIT
> > > > .long __pecoff_text_end - _start // BaseOfData
> > > > @@ -121,4 +121,13 @@ section_table:
> > > >
> > > > .balign 0x1000
> > > > efi_header_end:
> > > > +
> > > > + .global _efistub_entry
> > > > +_efistub_entry:
> > >
> > > This should go into .text or .init.text, not the header.
> > >
> > > > + /* Reload the global pointer */
> > > > + load_global_pointer
> > > > +
> > >
> > > What is supposed to happen here if CONFIG_SHADOW_CALL_STACK=y? The EFI
> > > stub Makefile removes the SCS CFLAGS, so the stub will be built
> > > without shadow call stack support, which I guess means that it might
> > > use GP as a global pointer as usual?
> > >
> > > > + call __efistub_efi_pe_entry
> > > > + ret
> > > > +
> > >
> > > You are returning to the firmware here, but after modifying the GP
> > > register. Shouldn't you restore it to its old value?
> > There is no need to restore the value of the gp register. Where gp is
> > needed, the gp register must first be initialized. And here is the
> > entry.
> >
>
> But how should the firmware know that GP was corrupted after calling
> the kernel's EFI entrypoint? The EFI stub can return to the firmware
> if it encounters any errors while still running in the EFI boot
> services.
>
Actually, I wonder if GP can be modified at all before
ExitBootServices(). The EFI timer interrupt is still live at this
point, and so the firmware is being called behind your back, and might
rely on GP retaining its original value.
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [External] Re: [PATCH 1/3] Revert "riscv/efistub: Ensure GP-relative addressing is not used"
2024-03-06 13:07 ` yunhui cui
@ 2024-03-06 13:11 ` Ard Biesheuvel
-1 siblings, 0 replies; 48+ messages in thread
From: Ard Biesheuvel @ 2024-03-06 13:11 UTC (permalink / raw)
To: yunhui cui
Cc: Jan Kiszka, paul.walmsley, palmer, aou, xuzhipeng.1973,
alexghiti, samitolvanen, bp, xiao.w.wang, kirill.shutemov,
nathan, linux-riscv, linux-kernel, linux-efi
On Wed, 6 Mar 2024 at 14:08, yunhui cui <cuiyunhui@bytedance.com> wrote:
>
> Hi Jan,
>
> On Wed, Mar 6, 2024 at 8:52 PM Jan Kiszka <jan.kiszka@siemens.com> wrote:
> >
> > On 06.03.24 09:56, Yunhui Cui wrote:
> > > This reverts commit afb2a4fb84555ef9e61061f6ea63ed7087b295d5.
> > >
> >
> > This comes without a reason - which is likely something around "will fix
> > this properly later". But then you regress first and only fix
> > afterwards. Can't that be done the other way around?
>
> Sorry, I don't quite understand what you mean. Can you help explain it
> more clearly? Do you mean "delete mno-relax instead of revert
> directly?"
>
You should order your patches in a way that does not create
intermediate states (between 1-2 or between 2-3) where the original
problem is being recreated.
So in this case, you should
a) fix the issue
b) revert the existing patches in *opposite* order
However, I don't think the EFI stub can use GP - please refer to my other reply.
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [External] Re: [PATCH 1/3] Revert "riscv/efistub: Ensure GP-relative addressing is not used"
@ 2024-03-06 13:11 ` Ard Biesheuvel
0 siblings, 0 replies; 48+ messages in thread
From: Ard Biesheuvel @ 2024-03-06 13:11 UTC (permalink / raw)
To: yunhui cui
Cc: Jan Kiszka, paul.walmsley, palmer, aou, xuzhipeng.1973,
alexghiti, samitolvanen, bp, xiao.w.wang, kirill.shutemov,
nathan, linux-riscv, linux-kernel, linux-efi
On Wed, 6 Mar 2024 at 14:08, yunhui cui <cuiyunhui@bytedance.com> wrote:
>
> Hi Jan,
>
> On Wed, Mar 6, 2024 at 8:52 PM Jan Kiszka <jan.kiszka@siemens.com> wrote:
> >
> > On 06.03.24 09:56, Yunhui Cui wrote:
> > > This reverts commit afb2a4fb84555ef9e61061f6ea63ed7087b295d5.
> > >
> >
> > This comes without a reason - which is likely something around "will fix
> > this properly later". But then you regress first and only fix
> > afterwards. Can't that be done the other way around?
>
> Sorry, I don't quite understand what you mean. Can you help explain it
> more clearly? Do you mean "delete mno-relax instead of revert
> directly?"
>
You should order your patches in a way that does not create
intermediate states (between 1-2 or between 2-3) where the original
problem is being recreated.
So in this case, you should
a) fix the issue
b) revert the existing patches in *opposite* order
However, I don't think the EFI stub can use GP - please refer to my other reply.
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [External] Re: [PATCH 1/3] Revert "riscv/efistub: Ensure GP-relative addressing is not used"
2024-03-06 13:11 ` Ard Biesheuvel
@ 2024-03-06 13:26 ` yunhui cui
-1 siblings, 0 replies; 48+ messages in thread
From: yunhui cui @ 2024-03-06 13:26 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Jan Kiszka, paul.walmsley, palmer, aou, xuzhipeng.1973,
alexghiti, samitolvanen, bp, xiao.w.wang, kirill.shutemov,
nathan, linux-riscv, linux-kernel, linux-efi
Hi Ard,
On Wed, Mar 6, 2024 at 9:11 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 6 Mar 2024 at 14:08, yunhui cui <cuiyunhui@bytedance.com> wrote:
> >
> > Hi Jan,
> >
> > On Wed, Mar 6, 2024 at 8:52 PM Jan Kiszka <jan.kiszka@siemens.com> wrote:
> > >
> > > On 06.03.24 09:56, Yunhui Cui wrote:
> > > > This reverts commit afb2a4fb84555ef9e61061f6ea63ed7087b295d5.
> > > >
> > >
> > > This comes without a reason - which is likely something around "will fix
> > > this properly later". But then you regress first and only fix
> > > afterwards. Can't that be done the other way around?
> >
> > Sorry, I don't quite understand what you mean. Can you help explain it
> > more clearly? Do you mean "delete mno-relax instead of revert
> > directly?"
> >
>
> You should order your patches in a way that does not create
> intermediate states (between 1-2 or between 2-3) where the original
> problem is being recreated.
>
> So in this case, you should
> a) fix the issue
> b) revert the existing patches in *opposite* order
Simply, I plan to remove "-mno-relax" and
"\|R_RISCV_$(BITS)\|R_RISCV_RELAX" in the third patch (fix patch).
> However, I don't think the EFI stub can use GP - please refer to my other reply.
The problem we encountered is that gcc 13 will optimize efistub using gp.
Thanks,
Yunhui
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [External] Re: [PATCH 1/3] Revert "riscv/efistub: Ensure GP-relative addressing is not used"
@ 2024-03-06 13:26 ` yunhui cui
0 siblings, 0 replies; 48+ messages in thread
From: yunhui cui @ 2024-03-06 13:26 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Jan Kiszka, paul.walmsley, palmer, aou, xuzhipeng.1973,
alexghiti, samitolvanen, bp, xiao.w.wang, kirill.shutemov,
nathan, linux-riscv, linux-kernel, linux-efi
Hi Ard,
On Wed, Mar 6, 2024 at 9:11 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 6 Mar 2024 at 14:08, yunhui cui <cuiyunhui@bytedance.com> wrote:
> >
> > Hi Jan,
> >
> > On Wed, Mar 6, 2024 at 8:52 PM Jan Kiszka <jan.kiszka@siemens.com> wrote:
> > >
> > > On 06.03.24 09:56, Yunhui Cui wrote:
> > > > This reverts commit afb2a4fb84555ef9e61061f6ea63ed7087b295d5.
> > > >
> > >
> > > This comes without a reason - which is likely something around "will fix
> > > this properly later". But then you regress first and only fix
> > > afterwards. Can't that be done the other way around?
> >
> > Sorry, I don't quite understand what you mean. Can you help explain it
> > more clearly? Do you mean "delete mno-relax instead of revert
> > directly?"
> >
>
> You should order your patches in a way that does not create
> intermediate states (between 1-2 or between 2-3) where the original
> problem is being recreated.
>
> So in this case, you should
> a) fix the issue
> b) revert the existing patches in *opposite* order
Simply, I plan to remove "-mno-relax" and
"\|R_RISCV_$(BITS)\|R_RISCV_RELAX" in the third patch (fix patch).
> However, I don't think the EFI stub can use GP - please refer to my other reply.
The problem we encountered is that gcc 13 will optimize efistub using gp.
Thanks,
Yunhui
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [External] Re: [PATCH 3/3] efistub: fix missed the initialization of gp
2024-03-06 13:09 ` Ard Biesheuvel
@ 2024-03-06 13:30 ` yunhui cui
-1 siblings, 0 replies; 48+ messages in thread
From: yunhui cui @ 2024-03-06 13:30 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: paul.walmsley, palmer, aou, xuzhipeng.1973, alexghiti,
samitolvanen, bp, xiao.w.wang, jan.kiszka, kirill.shutemov,
nathan, linux-riscv, linux-kernel, linux-efi
Hi Ard,
On Wed, Mar 6, 2024 at 9:09 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 6 Mar 2024 at 14:02, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Wed, 6 Mar 2024 at 13:34, yunhui cui <cuiyunhui@bytedance.com> wrote:
> > >
> > > Hi Ard,
> > >
> > > On Wed, Mar 6, 2024 at 5:36 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> > > >
> > > > On Wed, 6 Mar 2024 at 09:56, Yunhui Cui <cuiyunhui@bytedance.com> wrote:
> > > > >
> > > > > Compared with gcc version 12, gcc version 13 uses the gp
> > > > > register for compilation optimization, but the efistub module
> > > > > does not initialize gp.
> > > > >
> > > > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> > > > > Co-Developed-by: Zhipeng Xu <xuzhipeng.1973@bytedance.com>
> > > >
> > > > This needs a sign-off, and your signoff needs to come after.
> > > >
> > > > > ---
> > > > > arch/riscv/kernel/efi-header.S | 11 ++++++++++-
> > > > > 1 file changed, 10 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/arch/riscv/kernel/efi-header.S b/arch/riscv/kernel/efi-header.S
> > > > > index 515b2dfbca75..fa17c08c092a 100644
> > > > > --- a/arch/riscv/kernel/efi-header.S
> > > > > +++ b/arch/riscv/kernel/efi-header.S
> > > > > @@ -40,7 +40,7 @@ optional_header:
> > > > > .long __pecoff_data_virt_end - __pecoff_text_end // SizeOfInitializedData
> > > > > #endif
> > > > > .long 0 // SizeOfUninitializedData
> > > > > - .long __efistub_efi_pe_entry - _start // AddressOfEntryPoint
> > > > > + .long _efistub_entry - _start // AddressOfEntryPoint
> > > > > .long efi_header_end - _start // BaseOfCode
> > > > > #ifdef CONFIG_32BIT
> > > > > .long __pecoff_text_end - _start // BaseOfData
> > > > > @@ -121,4 +121,13 @@ section_table:
> > > > >
> > > > > .balign 0x1000
> > > > > efi_header_end:
> > > > > +
> > > > > + .global _efistub_entry
> > > > > +_efistub_entry:
> > > >
> > > > This should go into .text or .init.text, not the header.
> > > >
> > > > > + /* Reload the global pointer */
> > > > > + load_global_pointer
> > > > > +
> > > >
> > > > What is supposed to happen here if CONFIG_SHADOW_CALL_STACK=y? The EFI
> > > > stub Makefile removes the SCS CFLAGS, so the stub will be built
> > > > without shadow call stack support, which I guess means that it might
> > > > use GP as a global pointer as usual?
> > > >
> > > > > + call __efistub_efi_pe_entry
> > > > > + ret
> > > > > +
> > > >
> > > > You are returning to the firmware here, but after modifying the GP
> > > > register. Shouldn't you restore it to its old value?
> > > There is no need to restore the value of the gp register. Where gp is
> > > needed, the gp register must first be initialized. And here is the
> > > entry.
> > >
> >
> > But how should the firmware know that GP was corrupted after calling
> > the kernel's EFI entrypoint? The EFI stub can return to the firmware
> > if it encounters any errors while still running in the EFI boot
> > services.
> >
>
> Actually, I wonder if GP can be modified at all before
> ExitBootServices(). The EFI timer interrupt is still live at this
> point, and so the firmware is being called behind your back, and might
> rely on GP retaining its original value.
OK, in v2 I will restore the value of gp as follows:
efi_header_end:
+
+ __INIT
+ .global_efistub_entry
+_efistub_entry:
+ /* Reload the global pointer */
+.option push
+.option norelax
+ addi sp,sp,-8
+ sd gp,0(sp)
+ la gp, __global_pointer$
+.option pop
+ call __efistub_efi_pe_entry
+ld gp,0(sp)
+addi sp,sp,8
+ret
+ __HEAD
+
.endm
what do you think?
Thanks,
Yunhui
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [External] Re: [PATCH 3/3] efistub: fix missed the initialization of gp
@ 2024-03-06 13:30 ` yunhui cui
0 siblings, 0 replies; 48+ messages in thread
From: yunhui cui @ 2024-03-06 13:30 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: paul.walmsley, palmer, aou, xuzhipeng.1973, alexghiti,
samitolvanen, bp, xiao.w.wang, jan.kiszka, kirill.shutemov,
nathan, linux-riscv, linux-kernel, linux-efi
Hi Ard,
On Wed, Mar 6, 2024 at 9:09 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 6 Mar 2024 at 14:02, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Wed, 6 Mar 2024 at 13:34, yunhui cui <cuiyunhui@bytedance.com> wrote:
> > >
> > > Hi Ard,
> > >
> > > On Wed, Mar 6, 2024 at 5:36 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> > > >
> > > > On Wed, 6 Mar 2024 at 09:56, Yunhui Cui <cuiyunhui@bytedance.com> wrote:
> > > > >
> > > > > Compared with gcc version 12, gcc version 13 uses the gp
> > > > > register for compilation optimization, but the efistub module
> > > > > does not initialize gp.
> > > > >
> > > > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> > > > > Co-Developed-by: Zhipeng Xu <xuzhipeng.1973@bytedance.com>
> > > >
> > > > This needs a sign-off, and your signoff needs to come after.
> > > >
> > > > > ---
> > > > > arch/riscv/kernel/efi-header.S | 11 ++++++++++-
> > > > > 1 file changed, 10 insertions(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/arch/riscv/kernel/efi-header.S b/arch/riscv/kernel/efi-header.S
> > > > > index 515b2dfbca75..fa17c08c092a 100644
> > > > > --- a/arch/riscv/kernel/efi-header.S
> > > > > +++ b/arch/riscv/kernel/efi-header.S
> > > > > @@ -40,7 +40,7 @@ optional_header:
> > > > > .long __pecoff_data_virt_end - __pecoff_text_end // SizeOfInitializedData
> > > > > #endif
> > > > > .long 0 // SizeOfUninitializedData
> > > > > - .long __efistub_efi_pe_entry - _start // AddressOfEntryPoint
> > > > > + .long _efistub_entry - _start // AddressOfEntryPoint
> > > > > .long efi_header_end - _start // BaseOfCode
> > > > > #ifdef CONFIG_32BIT
> > > > > .long __pecoff_text_end - _start // BaseOfData
> > > > > @@ -121,4 +121,13 @@ section_table:
> > > > >
> > > > > .balign 0x1000
> > > > > efi_header_end:
> > > > > +
> > > > > + .global _efistub_entry
> > > > > +_efistub_entry:
> > > >
> > > > This should go into .text or .init.text, not the header.
> > > >
> > > > > + /* Reload the global pointer */
> > > > > + load_global_pointer
> > > > > +
> > > >
> > > > What is supposed to happen here if CONFIG_SHADOW_CALL_STACK=y? The EFI
> > > > stub Makefile removes the SCS CFLAGS, so the stub will be built
> > > > without shadow call stack support, which I guess means that it might
> > > > use GP as a global pointer as usual?
> > > >
> > > > > + call __efistub_efi_pe_entry
> > > > > + ret
> > > > > +
> > > >
> > > > You are returning to the firmware here, but after modifying the GP
> > > > register. Shouldn't you restore it to its old value?
> > > There is no need to restore the value of the gp register. Where gp is
> > > needed, the gp register must first be initialized. And here is the
> > > entry.
> > >
> >
> > But how should the firmware know that GP was corrupted after calling
> > the kernel's EFI entrypoint? The EFI stub can return to the firmware
> > if it encounters any errors while still running in the EFI boot
> > services.
> >
>
> Actually, I wonder if GP can be modified at all before
> ExitBootServices(). The EFI timer interrupt is still live at this
> point, and so the firmware is being called behind your back, and might
> rely on GP retaining its original value.
OK, in v2 I will restore the value of gp as follows:
efi_header_end:
+
+ __INIT
+ .global_efistub_entry
+_efistub_entry:
+ /* Reload the global pointer */
+.option push
+.option norelax
+ addi sp,sp,-8
+ sd gp,0(sp)
+ la gp, __global_pointer$
+.option pop
+ call __efistub_efi_pe_entry
+ld gp,0(sp)
+addi sp,sp,8
+ret
+ __HEAD
+
.endm
what do you think?
Thanks,
Yunhui
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [External] Re: [PATCH 1/3] Revert "riscv/efistub: Ensure GP-relative addressing is not used"
2024-03-06 13:26 ` yunhui cui
@ 2024-03-06 13:46 ` Ard Biesheuvel
-1 siblings, 0 replies; 48+ messages in thread
From: Ard Biesheuvel @ 2024-03-06 13:46 UTC (permalink / raw)
To: yunhui cui
Cc: Jan Kiszka, paul.walmsley, palmer, aou, xuzhipeng.1973,
alexghiti, samitolvanen, bp, xiao.w.wang, kirill.shutemov,
nathan, linux-riscv, linux-kernel, linux-efi
On Wed, 6 Mar 2024 at 14:27, yunhui cui <cuiyunhui@bytedance.com> wrote:
>
> Hi Ard,
>
> On Wed, Mar 6, 2024 at 9:11 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Wed, 6 Mar 2024 at 14:08, yunhui cui <cuiyunhui@bytedance.com> wrote:
> > >
> > > Hi Jan,
> > >
> > > On Wed, Mar 6, 2024 at 8:52 PM Jan Kiszka <jan.kiszka@siemens.com> wrote:
> > > >
> > > > On 06.03.24 09:56, Yunhui Cui wrote:
> > > > > This reverts commit afb2a4fb84555ef9e61061f6ea63ed7087b295d5.
> > > > >
> > > >
> > > > This comes without a reason - which is likely something around "will fix
> > > > this properly later". But then you regress first and only fix
> > > > afterwards. Can't that be done the other way around?
> > >
> > > Sorry, I don't quite understand what you mean. Can you help explain it
> > > more clearly? Do you mean "delete mno-relax instead of revert
> > > directly?"
> > >
> >
> > You should order your patches in a way that does not create
> > intermediate states (between 1-2 or between 2-3) where the original
> > problem is being recreated.
> >
> > So in this case, you should
> > a) fix the issue
> > b) revert the existing patches in *opposite* order
> Simply, I plan to remove "-mno-relax" and
> "\|R_RISCV_$(BITS)\|R_RISCV_RELAX" in the third patch (fix patch).
>
Why is that better than the current approach?
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [External] Re: [PATCH 1/3] Revert "riscv/efistub: Ensure GP-relative addressing is not used"
@ 2024-03-06 13:46 ` Ard Biesheuvel
0 siblings, 0 replies; 48+ messages in thread
From: Ard Biesheuvel @ 2024-03-06 13:46 UTC (permalink / raw)
To: yunhui cui
Cc: Jan Kiszka, paul.walmsley, palmer, aou, xuzhipeng.1973,
alexghiti, samitolvanen, bp, xiao.w.wang, kirill.shutemov,
nathan, linux-riscv, linux-kernel, linux-efi
On Wed, 6 Mar 2024 at 14:27, yunhui cui <cuiyunhui@bytedance.com> wrote:
>
> Hi Ard,
>
> On Wed, Mar 6, 2024 at 9:11 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Wed, 6 Mar 2024 at 14:08, yunhui cui <cuiyunhui@bytedance.com> wrote:
> > >
> > > Hi Jan,
> > >
> > > On Wed, Mar 6, 2024 at 8:52 PM Jan Kiszka <jan.kiszka@siemens.com> wrote:
> > > >
> > > > On 06.03.24 09:56, Yunhui Cui wrote:
> > > > > This reverts commit afb2a4fb84555ef9e61061f6ea63ed7087b295d5.
> > > > >
> > > >
> > > > This comes without a reason - which is likely something around "will fix
> > > > this properly later". But then you regress first and only fix
> > > > afterwards. Can't that be done the other way around?
> > >
> > > Sorry, I don't quite understand what you mean. Can you help explain it
> > > more clearly? Do you mean "delete mno-relax instead of revert
> > > directly?"
> > >
> >
> > You should order your patches in a way that does not create
> > intermediate states (between 1-2 or between 2-3) where the original
> > problem is being recreated.
> >
> > So in this case, you should
> > a) fix the issue
> > b) revert the existing patches in *opposite* order
> Simply, I plan to remove "-mno-relax" and
> "\|R_RISCV_$(BITS)\|R_RISCV_RELAX" in the third patch (fix patch).
>
Why is that better than the current approach?
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [External] Re: [PATCH 3/3] efistub: fix missed the initialization of gp
2024-03-06 13:09 ` Ard Biesheuvel
@ 2024-03-06 15:21 ` Palmer Dabbelt
-1 siblings, 0 replies; 48+ messages in thread
From: Palmer Dabbelt @ 2024-03-06 15:21 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: cuiyunhui, Paul Walmsley, aou, xuzhipeng.1973, alexghiti,
samitolvanen, bp, xiao.w.wang, jan.kiszka, kirill.shutemov,
nathan, linux-riscv, linux-kernel, linux-efi
On Wed, 06 Mar 2024 05:09:07 PST (-0800), Ard Biesheuvel wrote:
> On Wed, 6 Mar 2024 at 14:02, Ard Biesheuvel <ardb@kernel.org> wrote:
>>
>> On Wed, 6 Mar 2024 at 13:34, yunhui cui <cuiyunhui@bytedance.com> wrote:
>> >
>> > Hi Ard,
>> >
>> > On Wed, Mar 6, 2024 at 5:36 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>> > >
>> > > On Wed, 6 Mar 2024 at 09:56, Yunhui Cui <cuiyunhui@bytedance.com> wrote:
>> > > >
>> > > > Compared with gcc version 12, gcc version 13 uses the gp
>> > > > register for compilation optimization, but the efistub module
>> > > > does not initialize gp.
>> > > >
>> > > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
>> > > > Co-Developed-by: Zhipeng Xu <xuzhipeng.1973@bytedance.com>
>> > >
>> > > This needs a sign-off, and your signoff needs to come after.
>> > >
>> > > > ---
>> > > > arch/riscv/kernel/efi-header.S | 11 ++++++++++-
>> > > > 1 file changed, 10 insertions(+), 1 deletion(-)
>> > > >
>> > > > diff --git a/arch/riscv/kernel/efi-header.S b/arch/riscv/kernel/efi-header.S
>> > > > index 515b2dfbca75..fa17c08c092a 100644
>> > > > --- a/arch/riscv/kernel/efi-header.S
>> > > > +++ b/arch/riscv/kernel/efi-header.S
>> > > > @@ -40,7 +40,7 @@ optional_header:
>> > > > .long __pecoff_data_virt_end - __pecoff_text_end // SizeOfInitializedData
>> > > > #endif
>> > > > .long 0 // SizeOfUninitializedData
>> > > > - .long __efistub_efi_pe_entry - _start // AddressOfEntryPoint
>> > > > + .long _efistub_entry - _start // AddressOfEntryPoint
>> > > > .long efi_header_end - _start // BaseOfCode
>> > > > #ifdef CONFIG_32BIT
>> > > > .long __pecoff_text_end - _start // BaseOfData
>> > > > @@ -121,4 +121,13 @@ section_table:
>> > > >
>> > > > .balign 0x1000
>> > > > efi_header_end:
>> > > > +
>> > > > + .global _efistub_entry
>> > > > +_efistub_entry:
>> > >
>> > > This should go into .text or .init.text, not the header.
>> > >
>> > > > + /* Reload the global pointer */
>> > > > + load_global_pointer
>> > > > +
>> > >
>> > > What is supposed to happen here if CONFIG_SHADOW_CALL_STACK=y? The EFI
>> > > stub Makefile removes the SCS CFLAGS, so the stub will be built
>> > > without shadow call stack support, which I guess means that it might
>> > > use GP as a global pointer as usual?
>> > >
>> > > > + call __efistub_efi_pe_entry
>> > > > + ret
>> > > > +
>> > >
>> > > You are returning to the firmware here, but after modifying the GP
>> > > register. Shouldn't you restore it to its old value?
>> > There is no need to restore the value of the gp register. Where gp is
>> > needed, the gp register must first be initialized. And here is the
>> > entry.
>> >
>>
>> But how should the firmware know that GP was corrupted after calling
>> the kernel's EFI entrypoint? The EFI stub can return to the firmware
>> if it encounters any errors while still running in the EFI boot
>> services.
>>
>
> Actually, I wonder if GP can be modified at all before
> ExitBootServices(). The EFI timer interrupt is still live at this
> point, and so the firmware is being called behind your back, and might
> rely on GP retaining its original value.
[A few of us are talking on IRC as I'm writing this...]
The UEFI spec says "UEFI firmware must neither trust the
values of tp and gp nor make an assumption of owning the write access to
these register in any circumstances". It's kind of vague what "UEFI
firmware" means here, but I think it's reasonable to assume that the
kernel (and thus the EFI stub) is not included there.
So under that interpretation, the kernel (including the EFI stub) would
be allowed to overwrite GP with whatever it wants.
[We're still talking on IRC, though]
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [External] Re: [PATCH 3/3] efistub: fix missed the initialization of gp
@ 2024-03-06 15:21 ` Palmer Dabbelt
0 siblings, 0 replies; 48+ messages in thread
From: Palmer Dabbelt @ 2024-03-06 15:21 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: cuiyunhui, Paul Walmsley, aou, xuzhipeng.1973, alexghiti,
samitolvanen, bp, xiao.w.wang, jan.kiszka, kirill.shutemov,
nathan, linux-riscv, linux-kernel, linux-efi
On Wed, 06 Mar 2024 05:09:07 PST (-0800), Ard Biesheuvel wrote:
> On Wed, 6 Mar 2024 at 14:02, Ard Biesheuvel <ardb@kernel.org> wrote:
>>
>> On Wed, 6 Mar 2024 at 13:34, yunhui cui <cuiyunhui@bytedance.com> wrote:
>> >
>> > Hi Ard,
>> >
>> > On Wed, Mar 6, 2024 at 5:36 PM Ard Biesheuvel <ardb@kernel.org> wrote:
>> > >
>> > > On Wed, 6 Mar 2024 at 09:56, Yunhui Cui <cuiyunhui@bytedance.com> wrote:
>> > > >
>> > > > Compared with gcc version 12, gcc version 13 uses the gp
>> > > > register for compilation optimization, but the efistub module
>> > > > does not initialize gp.
>> > > >
>> > > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
>> > > > Co-Developed-by: Zhipeng Xu <xuzhipeng.1973@bytedance.com>
>> > >
>> > > This needs a sign-off, and your signoff needs to come after.
>> > >
>> > > > ---
>> > > > arch/riscv/kernel/efi-header.S | 11 ++++++++++-
>> > > > 1 file changed, 10 insertions(+), 1 deletion(-)
>> > > >
>> > > > diff --git a/arch/riscv/kernel/efi-header.S b/arch/riscv/kernel/efi-header.S
>> > > > index 515b2dfbca75..fa17c08c092a 100644
>> > > > --- a/arch/riscv/kernel/efi-header.S
>> > > > +++ b/arch/riscv/kernel/efi-header.S
>> > > > @@ -40,7 +40,7 @@ optional_header:
>> > > > .long __pecoff_data_virt_end - __pecoff_text_end // SizeOfInitializedData
>> > > > #endif
>> > > > .long 0 // SizeOfUninitializedData
>> > > > - .long __efistub_efi_pe_entry - _start // AddressOfEntryPoint
>> > > > + .long _efistub_entry - _start // AddressOfEntryPoint
>> > > > .long efi_header_end - _start // BaseOfCode
>> > > > #ifdef CONFIG_32BIT
>> > > > .long __pecoff_text_end - _start // BaseOfData
>> > > > @@ -121,4 +121,13 @@ section_table:
>> > > >
>> > > > .balign 0x1000
>> > > > efi_header_end:
>> > > > +
>> > > > + .global _efistub_entry
>> > > > +_efistub_entry:
>> > >
>> > > This should go into .text or .init.text, not the header.
>> > >
>> > > > + /* Reload the global pointer */
>> > > > + load_global_pointer
>> > > > +
>> > >
>> > > What is supposed to happen here if CONFIG_SHADOW_CALL_STACK=y? The EFI
>> > > stub Makefile removes the SCS CFLAGS, so the stub will be built
>> > > without shadow call stack support, which I guess means that it might
>> > > use GP as a global pointer as usual?
>> > >
>> > > > + call __efistub_efi_pe_entry
>> > > > + ret
>> > > > +
>> > >
>> > > You are returning to the firmware here, but after modifying the GP
>> > > register. Shouldn't you restore it to its old value?
>> > There is no need to restore the value of the gp register. Where gp is
>> > needed, the gp register must first be initialized. And here is the
>> > entry.
>> >
>>
>> But how should the firmware know that GP was corrupted after calling
>> the kernel's EFI entrypoint? The EFI stub can return to the firmware
>> if it encounters any errors while still running in the EFI boot
>> services.
>>
>
> Actually, I wonder if GP can be modified at all before
> ExitBootServices(). The EFI timer interrupt is still live at this
> point, and so the firmware is being called behind your back, and might
> rely on GP retaining its original value.
[A few of us are talking on IRC as I'm writing this...]
The UEFI spec says "UEFI firmware must neither trust the
values of tp and gp nor make an assumption of owning the write access to
these register in any circumstances". It's kind of vague what "UEFI
firmware" means here, but I think it's reasonable to assume that the
kernel (and thus the EFI stub) is not included there.
So under that interpretation, the kernel (including the EFI stub) would
be allowed to overwrite GP with whatever it wants.
[We're still talking on IRC, though]
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [External] Re: [PATCH 3/3] efistub: fix missed the initialization of gp
2024-03-06 15:21 ` Palmer Dabbelt
@ 2024-03-06 15:44 ` Ard Biesheuvel
-1 siblings, 0 replies; 48+ messages in thread
From: Ard Biesheuvel @ 2024-03-06 15:44 UTC (permalink / raw)
To: Palmer Dabbelt
Cc: cuiyunhui, Paul Walmsley, aou, xuzhipeng.1973, alexghiti,
samitolvanen, bp, xiao.w.wang, jan.kiszka, kirill.shutemov,
nathan, linux-riscv, linux-kernel, linux-efi, Conor Dooley,
Heinrich Schuchardt
On Wed, 6 Mar 2024 at 16:21, Palmer Dabbelt <palmer@dabbelt.com> wrote:
>
> On Wed, 06 Mar 2024 05:09:07 PST (-0800), Ard Biesheuvel wrote:
> > On Wed, 6 Mar 2024 at 14:02, Ard Biesheuvel <ardb@kernel.org> wrote:
> >>
> >> On Wed, 6 Mar 2024 at 13:34, yunhui cui <cuiyunhui@bytedance.com> wrote:
> >> >
> >> > Hi Ard,
> >> >
> >> > On Wed, Mar 6, 2024 at 5:36 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> >> > >
> >> > > On Wed, 6 Mar 2024 at 09:56, Yunhui Cui <cuiyunhui@bytedance.com> wrote:
> >> > > >
> >> > > > Compared with gcc version 12, gcc version 13 uses the gp
> >> > > > register for compilation optimization, but the efistub module
> >> > > > does not initialize gp.
> >> > > >
> >> > > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> >> > > > Co-Developed-by: Zhipeng Xu <xuzhipeng.1973@bytedance.com>
> >> > >
> >> > > This needs a sign-off, and your signoff needs to come after.
> >> > >
> >> > > > ---
> >> > > > arch/riscv/kernel/efi-header.S | 11 ++++++++++-
> >> > > > 1 file changed, 10 insertions(+), 1 deletion(-)
> >> > > >
> >> > > > diff --git a/arch/riscv/kernel/efi-header.S b/arch/riscv/kernel/efi-header.S
> >> > > > index 515b2dfbca75..fa17c08c092a 100644
> >> > > > --- a/arch/riscv/kernel/efi-header.S
> >> > > > +++ b/arch/riscv/kernel/efi-header.S
> >> > > > @@ -40,7 +40,7 @@ optional_header:
> >> > > > .long __pecoff_data_virt_end - __pecoff_text_end // SizeOfInitializedData
> >> > > > #endif
> >> > > > .long 0 // SizeOfUninitializedData
> >> > > > - .long __efistub_efi_pe_entry - _start // AddressOfEntryPoint
> >> > > > + .long _efistub_entry - _start // AddressOfEntryPoint
> >> > > > .long efi_header_end - _start // BaseOfCode
> >> > > > #ifdef CONFIG_32BIT
> >> > > > .long __pecoff_text_end - _start // BaseOfData
> >> > > > @@ -121,4 +121,13 @@ section_table:
> >> > > >
> >> > > > .balign 0x1000
> >> > > > efi_header_end:
> >> > > > +
> >> > > > + .global _efistub_entry
> >> > > > +_efistub_entry:
> >> > >
> >> > > This should go into .text or .init.text, not the header.
> >> > >
> >> > > > + /* Reload the global pointer */
> >> > > > + load_global_pointer
> >> > > > +
> >> > >
> >> > > What is supposed to happen here if CONFIG_SHADOW_CALL_STACK=y? The EFI
> >> > > stub Makefile removes the SCS CFLAGS, so the stub will be built
> >> > > without shadow call stack support, which I guess means that it might
> >> > > use GP as a global pointer as usual?
> >> > >
> >> > > > + call __efistub_efi_pe_entry
> >> > > > + ret
> >> > > > +
> >> > >
> >> > > You are returning to the firmware here, but after modifying the GP
> >> > > register. Shouldn't you restore it to its old value?
> >> > There is no need to restore the value of the gp register. Where gp is
> >> > needed, the gp register must first be initialized. And here is the
> >> > entry.
> >> >
> >>
> >> But how should the firmware know that GP was corrupted after calling
> >> the kernel's EFI entrypoint? The EFI stub can return to the firmware
> >> if it encounters any errors while still running in the EFI boot
> >> services.
> >>
> >
> > Actually, I wonder if GP can be modified at all before
> > ExitBootServices(). The EFI timer interrupt is still live at this
> > point, and so the firmware is being called behind your back, and might
> > rely on GP retaining its original value.
>
> [A few of us are talking on IRC as I'm writing this...]
>
> The UEFI spec says "UEFI firmware must neither trust the
> values of tp and gp nor make an assumption of owning the write access to
> these register in any circumstances". It's kind of vague what "UEFI
> firmware" means here, but I think it's reasonable to assume that the
> kernel (and thus the EFI stub) is not included there.
>
> So under that interpretation, the kernel (including the EFI stub) would
> be allowed to overwrite GP with whatever it wants.
>
OK, so even if the UEFI spec seems to suggest that using GP in EFI
applications such as the Linux EFI stub should be safe, I'd still like
to understand why this change is necessary. The patches you are
reverting are supposed to ensure that a) the compiler does not
generate references that can be relaxed to GP based ones, and b) no
R_RISCV_RELAX relocations are present in any of the code that runs in
the context of the EFI firmware.
Are you still seeing GP based symbol references? Is there C code that
gets pulled into the EFI stub that uses GP based relocations perhaps?
(see list below). If any of those are implemented in C, they should
not be used by the EFI stub directly unless they are guaranteed to be
uninstrumented and callable at arbitrary offsets other than the one
they were linked to run at.
__efistub_memcmp = memcmp;
__efistub_memchr = memchr;
__efistub_memcpy = memcpy;
__efistub_memmove = memmove;
__efistub_memset = memset;
__efistub_strlen = strlen;
__efistub_strnlen = strnlen;
__efistub_strcmp = strcmp;
__efistub_strncmp = strncmp;
__efistub_strrchr = strrchr;
__efistub___memcpy = memcpy;
__efistub___memmove = memmove;
__efistub___memset = memset;
__efistub__start = _start;
__efistub__start_kernel = _start_kernel;
(from arch/riscv/kernel/image-vars.h)
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [External] Re: [PATCH 3/3] efistub: fix missed the initialization of gp
@ 2024-03-06 15:44 ` Ard Biesheuvel
0 siblings, 0 replies; 48+ messages in thread
From: Ard Biesheuvel @ 2024-03-06 15:44 UTC (permalink / raw)
To: Palmer Dabbelt
Cc: cuiyunhui, Paul Walmsley, aou, xuzhipeng.1973, alexghiti,
samitolvanen, bp, xiao.w.wang, jan.kiszka, kirill.shutemov,
nathan, linux-riscv, linux-kernel, linux-efi, Conor Dooley,
Heinrich Schuchardt
On Wed, 6 Mar 2024 at 16:21, Palmer Dabbelt <palmer@dabbelt.com> wrote:
>
> On Wed, 06 Mar 2024 05:09:07 PST (-0800), Ard Biesheuvel wrote:
> > On Wed, 6 Mar 2024 at 14:02, Ard Biesheuvel <ardb@kernel.org> wrote:
> >>
> >> On Wed, 6 Mar 2024 at 13:34, yunhui cui <cuiyunhui@bytedance.com> wrote:
> >> >
> >> > Hi Ard,
> >> >
> >> > On Wed, Mar 6, 2024 at 5:36 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> >> > >
> >> > > On Wed, 6 Mar 2024 at 09:56, Yunhui Cui <cuiyunhui@bytedance.com> wrote:
> >> > > >
> >> > > > Compared with gcc version 12, gcc version 13 uses the gp
> >> > > > register for compilation optimization, but the efistub module
> >> > > > does not initialize gp.
> >> > > >
> >> > > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> >> > > > Co-Developed-by: Zhipeng Xu <xuzhipeng.1973@bytedance.com>
> >> > >
> >> > > This needs a sign-off, and your signoff needs to come after.
> >> > >
> >> > > > ---
> >> > > > arch/riscv/kernel/efi-header.S | 11 ++++++++++-
> >> > > > 1 file changed, 10 insertions(+), 1 deletion(-)
> >> > > >
> >> > > > diff --git a/arch/riscv/kernel/efi-header.S b/arch/riscv/kernel/efi-header.S
> >> > > > index 515b2dfbca75..fa17c08c092a 100644
> >> > > > --- a/arch/riscv/kernel/efi-header.S
> >> > > > +++ b/arch/riscv/kernel/efi-header.S
> >> > > > @@ -40,7 +40,7 @@ optional_header:
> >> > > > .long __pecoff_data_virt_end - __pecoff_text_end // SizeOfInitializedData
> >> > > > #endif
> >> > > > .long 0 // SizeOfUninitializedData
> >> > > > - .long __efistub_efi_pe_entry - _start // AddressOfEntryPoint
> >> > > > + .long _efistub_entry - _start // AddressOfEntryPoint
> >> > > > .long efi_header_end - _start // BaseOfCode
> >> > > > #ifdef CONFIG_32BIT
> >> > > > .long __pecoff_text_end - _start // BaseOfData
> >> > > > @@ -121,4 +121,13 @@ section_table:
> >> > > >
> >> > > > .balign 0x1000
> >> > > > efi_header_end:
> >> > > > +
> >> > > > + .global _efistub_entry
> >> > > > +_efistub_entry:
> >> > >
> >> > > This should go into .text or .init.text, not the header.
> >> > >
> >> > > > + /* Reload the global pointer */
> >> > > > + load_global_pointer
> >> > > > +
> >> > >
> >> > > What is supposed to happen here if CONFIG_SHADOW_CALL_STACK=y? The EFI
> >> > > stub Makefile removes the SCS CFLAGS, so the stub will be built
> >> > > without shadow call stack support, which I guess means that it might
> >> > > use GP as a global pointer as usual?
> >> > >
> >> > > > + call __efistub_efi_pe_entry
> >> > > > + ret
> >> > > > +
> >> > >
> >> > > You are returning to the firmware here, but after modifying the GP
> >> > > register. Shouldn't you restore it to its old value?
> >> > There is no need to restore the value of the gp register. Where gp is
> >> > needed, the gp register must first be initialized. And here is the
> >> > entry.
> >> >
> >>
> >> But how should the firmware know that GP was corrupted after calling
> >> the kernel's EFI entrypoint? The EFI stub can return to the firmware
> >> if it encounters any errors while still running in the EFI boot
> >> services.
> >>
> >
> > Actually, I wonder if GP can be modified at all before
> > ExitBootServices(). The EFI timer interrupt is still live at this
> > point, and so the firmware is being called behind your back, and might
> > rely on GP retaining its original value.
>
> [A few of us are talking on IRC as I'm writing this...]
>
> The UEFI spec says "UEFI firmware must neither trust the
> values of tp and gp nor make an assumption of owning the write access to
> these register in any circumstances". It's kind of vague what "UEFI
> firmware" means here, but I think it's reasonable to assume that the
> kernel (and thus the EFI stub) is not included there.
>
> So under that interpretation, the kernel (including the EFI stub) would
> be allowed to overwrite GP with whatever it wants.
>
OK, so even if the UEFI spec seems to suggest that using GP in EFI
applications such as the Linux EFI stub should be safe, I'd still like
to understand why this change is necessary. The patches you are
reverting are supposed to ensure that a) the compiler does not
generate references that can be relaxed to GP based ones, and b) no
R_RISCV_RELAX relocations are present in any of the code that runs in
the context of the EFI firmware.
Are you still seeing GP based symbol references? Is there C code that
gets pulled into the EFI stub that uses GP based relocations perhaps?
(see list below). If any of those are implemented in C, they should
not be used by the EFI stub directly unless they are guaranteed to be
uninstrumented and callable at arbitrary offsets other than the one
they were linked to run at.
__efistub_memcmp = memcmp;
__efistub_memchr = memchr;
__efistub_memcpy = memcpy;
__efistub_memmove = memmove;
__efistub_memset = memset;
__efistub_strlen = strlen;
__efistub_strnlen = strnlen;
__efistub_strcmp = strcmp;
__efistub_strncmp = strncmp;
__efistub_strrchr = strrchr;
__efistub___memcpy = memcpy;
__efistub___memmove = memmove;
__efistub___memset = memset;
__efistub__start = _start;
__efistub__start_kernel = _start_kernel;
(from arch/riscv/kernel/image-vars.h)
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [External] Re: [PATCH 3/3] efistub: fix missed the initialization of gp
2024-03-06 15:44 ` Ard Biesheuvel
@ 2024-03-06 16:15 ` Ard Biesheuvel
-1 siblings, 0 replies; 48+ messages in thread
From: Ard Biesheuvel @ 2024-03-06 16:15 UTC (permalink / raw)
To: Palmer Dabbelt
Cc: cuiyunhui, Paul Walmsley, aou, xuzhipeng.1973, alexghiti,
samitolvanen, bp, xiao.w.wang, jan.kiszka, kirill.shutemov,
nathan, linux-riscv, linux-kernel, linux-efi, Conor Dooley,
Heinrich Schuchardt
On Wed, 6 Mar 2024 at 16:44, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 6 Mar 2024 at 16:21, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> >
> > On Wed, 06 Mar 2024 05:09:07 PST (-0800), Ard Biesheuvel wrote:
> > > On Wed, 6 Mar 2024 at 14:02, Ard Biesheuvel <ardb@kernel.org> wrote:
> > >>
> > >> On Wed, 6 Mar 2024 at 13:34, yunhui cui <cuiyunhui@bytedance.com> wrote:
> > >> >
> > >> > Hi Ard,
> > >> >
> > >> > On Wed, Mar 6, 2024 at 5:36 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> > >> > >
> > >> > > On Wed, 6 Mar 2024 at 09:56, Yunhui Cui <cuiyunhui@bytedance.com> wrote:
> > >> > > >
> > >> > > > Compared with gcc version 12, gcc version 13 uses the gp
> > >> > > > register for compilation optimization, but the efistub module
> > >> > > > does not initialize gp.
> > >> > > >
> > >> > > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> > >> > > > Co-Developed-by: Zhipeng Xu <xuzhipeng.1973@bytedance.com>
> > >> > >
> > >> > > This needs a sign-off, and your signoff needs to come after.
> > >> > >
> > >> > > > ---
> > >> > > > arch/riscv/kernel/efi-header.S | 11 ++++++++++-
> > >> > > > 1 file changed, 10 insertions(+), 1 deletion(-)
> > >> > > >
> > >> > > > diff --git a/arch/riscv/kernel/efi-header.S b/arch/riscv/kernel/efi-header.S
> > >> > > > index 515b2dfbca75..fa17c08c092a 100644
> > >> > > > --- a/arch/riscv/kernel/efi-header.S
> > >> > > > +++ b/arch/riscv/kernel/efi-header.S
> > >> > > > @@ -40,7 +40,7 @@ optional_header:
> > >> > > > .long __pecoff_data_virt_end - __pecoff_text_end // SizeOfInitializedData
> > >> > > > #endif
> > >> > > > .long 0 // SizeOfUninitializedData
> > >> > > > - .long __efistub_efi_pe_entry - _start // AddressOfEntryPoint
> > >> > > > + .long _efistub_entry - _start // AddressOfEntryPoint
> > >> > > > .long efi_header_end - _start // BaseOfCode
> > >> > > > #ifdef CONFIG_32BIT
> > >> > > > .long __pecoff_text_end - _start // BaseOfData
> > >> > > > @@ -121,4 +121,13 @@ section_table:
> > >> > > >
> > >> > > > .balign 0x1000
> > >> > > > efi_header_end:
> > >> > > > +
> > >> > > > + .global _efistub_entry
> > >> > > > +_efistub_entry:
> > >> > >
> > >> > > This should go into .text or .init.text, not the header.
> > >> > >
> > >> > > > + /* Reload the global pointer */
> > >> > > > + load_global_pointer
> > >> > > > +
> > >> > >
> > >> > > What is supposed to happen here if CONFIG_SHADOW_CALL_STACK=y? The EFI
> > >> > > stub Makefile removes the SCS CFLAGS, so the stub will be built
> > >> > > without shadow call stack support, which I guess means that it might
> > >> > > use GP as a global pointer as usual?
> > >> > >
> > >> > > > + call __efistub_efi_pe_entry
> > >> > > > + ret
> > >> > > > +
> > >> > >
> > >> > > You are returning to the firmware here, but after modifying the GP
> > >> > > register. Shouldn't you restore it to its old value?
> > >> > There is no need to restore the value of the gp register. Where gp is
> > >> > needed, the gp register must first be initialized. And here is the
> > >> > entry.
> > >> >
> > >>
> > >> But how should the firmware know that GP was corrupted after calling
> > >> the kernel's EFI entrypoint? The EFI stub can return to the firmware
> > >> if it encounters any errors while still running in the EFI boot
> > >> services.
> > >>
> > >
> > > Actually, I wonder if GP can be modified at all before
> > > ExitBootServices(). The EFI timer interrupt is still live at this
> > > point, and so the firmware is being called behind your back, and might
> > > rely on GP retaining its original value.
> >
> > [A few of us are talking on IRC as I'm writing this...]
> >
> > The UEFI spec says "UEFI firmware must neither trust the
> > values of tp and gp nor make an assumption of owning the write access to
> > these register in any circumstances". It's kind of vague what "UEFI
> > firmware" means here, but I think it's reasonable to assume that the
> > kernel (and thus the EFI stub) is not included there.
> >
> > So under that interpretation, the kernel (including the EFI stub) would
> > be allowed to overwrite GP with whatever it wants.
> >
>
> OK, so even if the UEFI spec seems to suggest that using GP in EFI
> applications such as the Linux EFI stub should be safe, I'd still like
> to understand why this change is necessary. The patches you are
> reverting are supposed to ensure that a) the compiler does not
> generate references that can be relaxed to GP based ones, and b) no
> R_RISCV_RELAX relocations are present in any of the code that runs in
> the context of the EFI firmware.
>
> Are you still seeing GP based symbol references? Is there C code that
> gets pulled into the EFI stub that uses GP based relocations perhaps?
> (see list below). If any of those are implemented in C, they should
> not be used by the EFI stub directly unless they are guaranteed to be
> uninstrumented and callable at arbitrary offsets other than the one
> they were linked to run at.
>
>
> __efistub_memcmp = memcmp;
> __efistub_memchr = memchr;
> __efistub_memcpy = memcpy;
> __efistub_memmove = memmove;
> __efistub_memset = memset;
> __efistub_strlen = strlen;
> __efistub_strnlen = strnlen;
> __efistub_strcmp = strcmp;
> __efistub_strncmp = strncmp;
> __efistub_strrchr = strrchr;
> __efistub___memcpy = memcpy;
> __efistub___memmove = memmove;
> __efistub___memset = memset;
> __efistub__start = _start;
> __efistub__start_kernel = _start_kernel;
>
> (from arch/riscv/kernel/image-vars.h)
Uhm never mind - these are all gone now, I was looking at a v6.1
kernel source tree.
So that means that, as far as I can tell, the only kernel C code that
executes in the context of the EFI firmware is built with -mno-relax
and is checked for the absence of R_RISCV_RELAX relocations. So I fail
to see why these changes are needed.
Yunhui, could you please explain the reason for this series?
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [External] Re: [PATCH 3/3] efistub: fix missed the initialization of gp
@ 2024-03-06 16:15 ` Ard Biesheuvel
0 siblings, 0 replies; 48+ messages in thread
From: Ard Biesheuvel @ 2024-03-06 16:15 UTC (permalink / raw)
To: Palmer Dabbelt
Cc: cuiyunhui, Paul Walmsley, aou, xuzhipeng.1973, alexghiti,
samitolvanen, bp, xiao.w.wang, jan.kiszka, kirill.shutemov,
nathan, linux-riscv, linux-kernel, linux-efi, Conor Dooley,
Heinrich Schuchardt
On Wed, 6 Mar 2024 at 16:44, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 6 Mar 2024 at 16:21, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> >
> > On Wed, 06 Mar 2024 05:09:07 PST (-0800), Ard Biesheuvel wrote:
> > > On Wed, 6 Mar 2024 at 14:02, Ard Biesheuvel <ardb@kernel.org> wrote:
> > >>
> > >> On Wed, 6 Mar 2024 at 13:34, yunhui cui <cuiyunhui@bytedance.com> wrote:
> > >> >
> > >> > Hi Ard,
> > >> >
> > >> > On Wed, Mar 6, 2024 at 5:36 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> > >> > >
> > >> > > On Wed, 6 Mar 2024 at 09:56, Yunhui Cui <cuiyunhui@bytedance.com> wrote:
> > >> > > >
> > >> > > > Compared with gcc version 12, gcc version 13 uses the gp
> > >> > > > register for compilation optimization, but the efistub module
> > >> > > > does not initialize gp.
> > >> > > >
> > >> > > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> > >> > > > Co-Developed-by: Zhipeng Xu <xuzhipeng.1973@bytedance.com>
> > >> > >
> > >> > > This needs a sign-off, and your signoff needs to come after.
> > >> > >
> > >> > > > ---
> > >> > > > arch/riscv/kernel/efi-header.S | 11 ++++++++++-
> > >> > > > 1 file changed, 10 insertions(+), 1 deletion(-)
> > >> > > >
> > >> > > > diff --git a/arch/riscv/kernel/efi-header.S b/arch/riscv/kernel/efi-header.S
> > >> > > > index 515b2dfbca75..fa17c08c092a 100644
> > >> > > > --- a/arch/riscv/kernel/efi-header.S
> > >> > > > +++ b/arch/riscv/kernel/efi-header.S
> > >> > > > @@ -40,7 +40,7 @@ optional_header:
> > >> > > > .long __pecoff_data_virt_end - __pecoff_text_end // SizeOfInitializedData
> > >> > > > #endif
> > >> > > > .long 0 // SizeOfUninitializedData
> > >> > > > - .long __efistub_efi_pe_entry - _start // AddressOfEntryPoint
> > >> > > > + .long _efistub_entry - _start // AddressOfEntryPoint
> > >> > > > .long efi_header_end - _start // BaseOfCode
> > >> > > > #ifdef CONFIG_32BIT
> > >> > > > .long __pecoff_text_end - _start // BaseOfData
> > >> > > > @@ -121,4 +121,13 @@ section_table:
> > >> > > >
> > >> > > > .balign 0x1000
> > >> > > > efi_header_end:
> > >> > > > +
> > >> > > > + .global _efistub_entry
> > >> > > > +_efistub_entry:
> > >> > >
> > >> > > This should go into .text or .init.text, not the header.
> > >> > >
> > >> > > > + /* Reload the global pointer */
> > >> > > > + load_global_pointer
> > >> > > > +
> > >> > >
> > >> > > What is supposed to happen here if CONFIG_SHADOW_CALL_STACK=y? The EFI
> > >> > > stub Makefile removes the SCS CFLAGS, so the stub will be built
> > >> > > without shadow call stack support, which I guess means that it might
> > >> > > use GP as a global pointer as usual?
> > >> > >
> > >> > > > + call __efistub_efi_pe_entry
> > >> > > > + ret
> > >> > > > +
> > >> > >
> > >> > > You are returning to the firmware here, but after modifying the GP
> > >> > > register. Shouldn't you restore it to its old value?
> > >> > There is no need to restore the value of the gp register. Where gp is
> > >> > needed, the gp register must first be initialized. And here is the
> > >> > entry.
> > >> >
> > >>
> > >> But how should the firmware know that GP was corrupted after calling
> > >> the kernel's EFI entrypoint? The EFI stub can return to the firmware
> > >> if it encounters any errors while still running in the EFI boot
> > >> services.
> > >>
> > >
> > > Actually, I wonder if GP can be modified at all before
> > > ExitBootServices(). The EFI timer interrupt is still live at this
> > > point, and so the firmware is being called behind your back, and might
> > > rely on GP retaining its original value.
> >
> > [A few of us are talking on IRC as I'm writing this...]
> >
> > The UEFI spec says "UEFI firmware must neither trust the
> > values of tp and gp nor make an assumption of owning the write access to
> > these register in any circumstances". It's kind of vague what "UEFI
> > firmware" means here, but I think it's reasonable to assume that the
> > kernel (and thus the EFI stub) is not included there.
> >
> > So under that interpretation, the kernel (including the EFI stub) would
> > be allowed to overwrite GP with whatever it wants.
> >
>
> OK, so even if the UEFI spec seems to suggest that using GP in EFI
> applications such as the Linux EFI stub should be safe, I'd still like
> to understand why this change is necessary. The patches you are
> reverting are supposed to ensure that a) the compiler does not
> generate references that can be relaxed to GP based ones, and b) no
> R_RISCV_RELAX relocations are present in any of the code that runs in
> the context of the EFI firmware.
>
> Are you still seeing GP based symbol references? Is there C code that
> gets pulled into the EFI stub that uses GP based relocations perhaps?
> (see list below). If any of those are implemented in C, they should
> not be used by the EFI stub directly unless they are guaranteed to be
> uninstrumented and callable at arbitrary offsets other than the one
> they were linked to run at.
>
>
> __efistub_memcmp = memcmp;
> __efistub_memchr = memchr;
> __efistub_memcpy = memcpy;
> __efistub_memmove = memmove;
> __efistub_memset = memset;
> __efistub_strlen = strlen;
> __efistub_strnlen = strnlen;
> __efistub_strcmp = strcmp;
> __efistub_strncmp = strncmp;
> __efistub_strrchr = strrchr;
> __efistub___memcpy = memcpy;
> __efistub___memmove = memmove;
> __efistub___memset = memset;
> __efistub__start = _start;
> __efistub__start_kernel = _start_kernel;
>
> (from arch/riscv/kernel/image-vars.h)
Uhm never mind - these are all gone now, I was looking at a v6.1
kernel source tree.
So that means that, as far as I can tell, the only kernel C code that
executes in the context of the EFI firmware is built with -mno-relax
and is checked for the absence of R_RISCV_RELAX relocations. So I fail
to see why these changes are needed.
Yunhui, could you please explain the reason for this series?
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [External] Re: [PATCH 3/3] efistub: fix missed the initialization of gp
2024-03-06 16:15 ` Ard Biesheuvel
@ 2024-03-06 17:11 ` Ard Biesheuvel
-1 siblings, 0 replies; 48+ messages in thread
From: Ard Biesheuvel @ 2024-03-06 17:11 UTC (permalink / raw)
To: Palmer Dabbelt
Cc: cuiyunhui, Paul Walmsley, aou, xuzhipeng.1973, alexghiti,
samitolvanen, bp, xiao.w.wang, jan.kiszka, kirill.shutemov,
nathan, linux-riscv, linux-kernel, linux-efi, Conor Dooley,
Heinrich Schuchardt, Pedro Falcato
On Wed, 6 Mar 2024 at 17:15, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 6 Mar 2024 at 16:44, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Wed, 6 Mar 2024 at 16:21, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> > >
...
> > >
> > > The UEFI spec says "UEFI firmware must neither trust the
> > > values of tp and gp nor make an assumption of owning the write access to
> > > these register in any circumstances". It's kind of vague what "UEFI
> > > firmware" means here, but I think it's reasonable to assume that the
> > > kernel (and thus the EFI stub) is not included there.
> > >
> > > So under that interpretation, the kernel (including the EFI stub) would
> > > be allowed to overwrite GP with whatever it wants.
> > >
...
After some more consideration, I concluded that using GP in code that
executes in the context of EFI is never safe.
Taking the typical Linux/EFI boot sequence as an example, where GRUB
is loaded by the system firmware, and the Linux EFI stub is loaded by
GRUB, it is easy to spot the problem: GRUB exposes the initial ramdisk
via the LoadFile2 protocol, which is essentially a callback interface.
If we assume that EFI apps can use GP as they like, we cannot safely
call this callback interface without restoring GP to its original
value, since we have no idea whether GRUB (or some other loader) is
relying on its value. And in addition to synchronous callbacks, there
may be EFI event callbacks registered by GRUB that are signaled by the
system firmware asynchronously, which means GRUB code could be called
behind our backs. Without any guidance in the UEFI spec on how GP
needs to be managed across such boundaries, we should assume that GP
needs to be restored to the old value in each of those cases, but this
is impossible for async event handlers.
We might conclude then that using GP is only safe for EFI apps if they
don't install protocols or register for EFI events, but that would
still imply that we should restore GP to its old value upon exit.
So without a strong argument why GP relaxations need to be supported
in the EFI stub, I don't think we should consider these changes. If
there are any issues in the current code that result in inadvertent GP
relaxations, we should address those instead.
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [External] Re: [PATCH 3/3] efistub: fix missed the initialization of gp
@ 2024-03-06 17:11 ` Ard Biesheuvel
0 siblings, 0 replies; 48+ messages in thread
From: Ard Biesheuvel @ 2024-03-06 17:11 UTC (permalink / raw)
To: Palmer Dabbelt
Cc: cuiyunhui, Paul Walmsley, aou, xuzhipeng.1973, alexghiti,
samitolvanen, bp, xiao.w.wang, jan.kiszka, kirill.shutemov,
nathan, linux-riscv, linux-kernel, linux-efi, Conor Dooley,
Heinrich Schuchardt, Pedro Falcato
On Wed, 6 Mar 2024 at 17:15, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 6 Mar 2024 at 16:44, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Wed, 6 Mar 2024 at 16:21, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> > >
...
> > >
> > > The UEFI spec says "UEFI firmware must neither trust the
> > > values of tp and gp nor make an assumption of owning the write access to
> > > these register in any circumstances". It's kind of vague what "UEFI
> > > firmware" means here, but I think it's reasonable to assume that the
> > > kernel (and thus the EFI stub) is not included there.
> > >
> > > So under that interpretation, the kernel (including the EFI stub) would
> > > be allowed to overwrite GP with whatever it wants.
> > >
...
After some more consideration, I concluded that using GP in code that
executes in the context of EFI is never safe.
Taking the typical Linux/EFI boot sequence as an example, where GRUB
is loaded by the system firmware, and the Linux EFI stub is loaded by
GRUB, it is easy to spot the problem: GRUB exposes the initial ramdisk
via the LoadFile2 protocol, which is essentially a callback interface.
If we assume that EFI apps can use GP as they like, we cannot safely
call this callback interface without restoring GP to its original
value, since we have no idea whether GRUB (or some other loader) is
relying on its value. And in addition to synchronous callbacks, there
may be EFI event callbacks registered by GRUB that are signaled by the
system firmware asynchronously, which means GRUB code could be called
behind our backs. Without any guidance in the UEFI spec on how GP
needs to be managed across such boundaries, we should assume that GP
needs to be restored to the old value in each of those cases, but this
is impossible for async event handlers.
We might conclude then that using GP is only safe for EFI apps if they
don't install protocols or register for EFI events, but that would
still imply that we should restore GP to its old value upon exit.
So without a strong argument why GP relaxations need to be supported
in the EFI stub, I don't think we should consider these changes. If
there are any issues in the current code that result in inadvertent GP
relaxations, we should address those instead.
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [External] Re: [PATCH 3/3] efistub: fix missed the initialization of gp
2024-03-06 16:15 ` Ard Biesheuvel
@ 2024-03-07 3:19 ` yunhui cui
-1 siblings, 0 replies; 48+ messages in thread
From: yunhui cui @ 2024-03-07 3:19 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Palmer Dabbelt, Paul Walmsley, aou, xuzhipeng.1973, alexghiti,
samitolvanen, bp, xiao.w.wang, jan.kiszka, kirill.shutemov,
nathan, linux-riscv, linux-kernel, linux-efi, Conor Dooley,
Heinrich Schuchardt
Hi Ard,
On Thu, Mar 7, 2024 at 12:15 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 6 Mar 2024 at 16:44, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Wed, 6 Mar 2024 at 16:21, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> > >
> > > On Wed, 06 Mar 2024 05:09:07 PST (-0800), Ard Biesheuvel wrote:
> > > > On Wed, 6 Mar 2024 at 14:02, Ard Biesheuvel <ardb@kernel.org> wrote:
> > > >>
> > > >> On Wed, 6 Mar 2024 at 13:34, yunhui cui <cuiyunhui@bytedance.com> wrote:
> > > >> >
> > > >> > Hi Ard,
> > > >> >
> > > >> > On Wed, Mar 6, 2024 at 5:36 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> > > >> > >
> > > >> > > On Wed, 6 Mar 2024 at 09:56, Yunhui Cui <cuiyunhui@bytedance.com> wrote:
> > > >> > > >
> > > >> > > > Compared with gcc version 12, gcc version 13 uses the gp
> > > >> > > > register for compilation optimization, but the efistub module
> > > >> > > > does not initialize gp.
> > > >> > > >
> > > >> > > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> > > >> > > > Co-Developed-by: Zhipeng Xu <xuzhipeng.1973@bytedance.com>
> > > >> > >
> > > >> > > This needs a sign-off, and your signoff needs to come after.
> > > >> > >
> > > >> > > > ---
> > > >> > > > arch/riscv/kernel/efi-header.S | 11 ++++++++++-
> > > >> > > > 1 file changed, 10 insertions(+), 1 deletion(-)
> > > >> > > >
> > > >> > > > diff --git a/arch/riscv/kernel/efi-header.S b/arch/riscv/kernel/efi-header.S
> > > >> > > > index 515b2dfbca75..fa17c08c092a 100644
> > > >> > > > --- a/arch/riscv/kernel/efi-header.S
> > > >> > > > +++ b/arch/riscv/kernel/efi-header.S
> > > >> > > > @@ -40,7 +40,7 @@ optional_header:
> > > >> > > > .long __pecoff_data_virt_end - __pecoff_text_end // SizeOfInitializedData
> > > >> > > > #endif
> > > >> > > > .long 0 // SizeOfUninitializedData
> > > >> > > > - .long __efistub_efi_pe_entry - _start // AddressOfEntryPoint
> > > >> > > > + .long _efistub_entry - _start // AddressOfEntryPoint
> > > >> > > > .long efi_header_end - _start // BaseOfCode
> > > >> > > > #ifdef CONFIG_32BIT
> > > >> > > > .long __pecoff_text_end - _start // BaseOfData
> > > >> > > > @@ -121,4 +121,13 @@ section_table:
> > > >> > > >
> > > >> > > > .balign 0x1000
> > > >> > > > efi_header_end:
> > > >> > > > +
> > > >> > > > + .global _efistub_entry
> > > >> > > > +_efistub_entry:
> > > >> > >
> > > >> > > This should go into .text or .init.text, not the header.
> > > >> > >
> > > >> > > > + /* Reload the global pointer */
> > > >> > > > + load_global_pointer
> > > >> > > > +
> > > >> > >
> > > >> > > What is supposed to happen here if CONFIG_SHADOW_CALL_STACK=y? The EFI
> > > >> > > stub Makefile removes the SCS CFLAGS, so the stub will be built
> > > >> > > without shadow call stack support, which I guess means that it might
> > > >> > > use GP as a global pointer as usual?
> > > >> > >
> > > >> > > > + call __efistub_efi_pe_entry
> > > >> > > > + ret
> > > >> > > > +
> > > >> > >
> > > >> > > You are returning to the firmware here, but after modifying the GP
> > > >> > > register. Shouldn't you restore it to its old value?
> > > >> > There is no need to restore the value of the gp register. Where gp is
> > > >> > needed, the gp register must first be initialized. And here is the
> > > >> > entry.
> > > >> >
> > > >>
> > > >> But how should the firmware know that GP was corrupted after calling
> > > >> the kernel's EFI entrypoint? The EFI stub can return to the firmware
> > > >> if it encounters any errors while still running in the EFI boot
> > > >> services.
> > > >>
> > > >
> > > > Actually, I wonder if GP can be modified at all before
> > > > ExitBootServices(). The EFI timer interrupt is still live at this
> > > > point, and so the firmware is being called behind your back, and might
> > > > rely on GP retaining its original value.
> > >
> > > [A few of us are talking on IRC as I'm writing this...]
> > >
> > > The UEFI spec says "UEFI firmware must neither trust the
> > > values of tp and gp nor make an assumption of owning the write access to
> > > these register in any circumstances". It's kind of vague what "UEFI
> > > firmware" means here, but I think it's reasonable to assume that the
> > > kernel (and thus the EFI stub) is not included there.
> > >
> > > So under that interpretation, the kernel (including the EFI stub) would
> > > be allowed to overwrite GP with whatever it wants.
> > >
> >
> > OK, so even if the UEFI spec seems to suggest that using GP in EFI
> > applications such as the Linux EFI stub should be safe, I'd still like
> > to understand why this change is necessary. The patches you are
> > reverting are supposed to ensure that a) the compiler does not
> > generate references that can be relaxed to GP based ones, and b) no
> > R_RISCV_RELAX relocations are present in any of the code that runs in
> > the context of the EFI firmware.
> >
> > Are you still seeing GP based symbol references? Is there C code that
> > gets pulled into the EFI stub that uses GP based relocations perhaps?
> > (see list below). If any of those are implemented in C, they should
> > not be used by the EFI stub directly unless they are guaranteed to be
> > uninstrumented and callable at arbitrary offsets other than the one
> > they were linked to run at.
> >
> >
> > __efistub_memcmp = memcmp;
> > __efistub_memchr = memchr;
> > __efistub_memcpy = memcpy;
> > __efistub_memmove = memmove;
> > __efistub_memset = memset;
> > __efistub_strlen = strlen;
> > __efistub_strnlen = strnlen;
> > __efistub_strcmp = strcmp;
> > __efistub_strncmp = strncmp;
> > __efistub_strrchr = strrchr;
> > __efistub___memcpy = memcpy;
> > __efistub___memmove = memmove;
> > __efistub___memset = memset;
> > __efistub__start = _start;
> > __efistub__start_kernel = _start_kernel;
> >
> > (from arch/riscv/kernel/image-vars.h)
>
> Uhm never mind - these are all gone now, I was looking at a v6.1
> kernel source tree.
>
> So that means that, as far as I can tell, the only kernel C code that
> executes in the context of the EFI firmware is built with -mno-relax
> and is checked for the absence of R_RISCV_RELAX relocations. So I fail
> to see why these changes are needed.
>
> Yunhui, could you please explain the reason for this series?
From the logic of binutils, if "__global_pointer$" exists, it is
possible to use GP for optimization. For RISC-V, "__global_pointer$"
was introduced in commit "fbe934d69eb7e". Therefore, for the system as
a whole, we should keep using GP uniformly. The root cause of this
problem is that GP is not loaded, rather than "On RISC-V, we also
avoid GP based relocations..." as commit "d2baf8cc82c17" said. We need
to address problems head-on, rather than avoid them.
Thanks,
Yunhui
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [External] Re: [PATCH 3/3] efistub: fix missed the initialization of gp
@ 2024-03-07 3:19 ` yunhui cui
0 siblings, 0 replies; 48+ messages in thread
From: yunhui cui @ 2024-03-07 3:19 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Palmer Dabbelt, Paul Walmsley, aou, xuzhipeng.1973, alexghiti,
samitolvanen, bp, xiao.w.wang, jan.kiszka, kirill.shutemov,
nathan, linux-riscv, linux-kernel, linux-efi, Conor Dooley,
Heinrich Schuchardt
Hi Ard,
On Thu, Mar 7, 2024 at 12:15 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Wed, 6 Mar 2024 at 16:44, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Wed, 6 Mar 2024 at 16:21, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> > >
> > > On Wed, 06 Mar 2024 05:09:07 PST (-0800), Ard Biesheuvel wrote:
> > > > On Wed, 6 Mar 2024 at 14:02, Ard Biesheuvel <ardb@kernel.org> wrote:
> > > >>
> > > >> On Wed, 6 Mar 2024 at 13:34, yunhui cui <cuiyunhui@bytedance.com> wrote:
> > > >> >
> > > >> > Hi Ard,
> > > >> >
> > > >> > On Wed, Mar 6, 2024 at 5:36 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> > > >> > >
> > > >> > > On Wed, 6 Mar 2024 at 09:56, Yunhui Cui <cuiyunhui@bytedance.com> wrote:
> > > >> > > >
> > > >> > > > Compared with gcc version 12, gcc version 13 uses the gp
> > > >> > > > register for compilation optimization, but the efistub module
> > > >> > > > does not initialize gp.
> > > >> > > >
> > > >> > > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> > > >> > > > Co-Developed-by: Zhipeng Xu <xuzhipeng.1973@bytedance.com>
> > > >> > >
> > > >> > > This needs a sign-off, and your signoff needs to come after.
> > > >> > >
> > > >> > > > ---
> > > >> > > > arch/riscv/kernel/efi-header.S | 11 ++++++++++-
> > > >> > > > 1 file changed, 10 insertions(+), 1 deletion(-)
> > > >> > > >
> > > >> > > > diff --git a/arch/riscv/kernel/efi-header.S b/arch/riscv/kernel/efi-header.S
> > > >> > > > index 515b2dfbca75..fa17c08c092a 100644
> > > >> > > > --- a/arch/riscv/kernel/efi-header.S
> > > >> > > > +++ b/arch/riscv/kernel/efi-header.S
> > > >> > > > @@ -40,7 +40,7 @@ optional_header:
> > > >> > > > .long __pecoff_data_virt_end - __pecoff_text_end // SizeOfInitializedData
> > > >> > > > #endif
> > > >> > > > .long 0 // SizeOfUninitializedData
> > > >> > > > - .long __efistub_efi_pe_entry - _start // AddressOfEntryPoint
> > > >> > > > + .long _efistub_entry - _start // AddressOfEntryPoint
> > > >> > > > .long efi_header_end - _start // BaseOfCode
> > > >> > > > #ifdef CONFIG_32BIT
> > > >> > > > .long __pecoff_text_end - _start // BaseOfData
> > > >> > > > @@ -121,4 +121,13 @@ section_table:
> > > >> > > >
> > > >> > > > .balign 0x1000
> > > >> > > > efi_header_end:
> > > >> > > > +
> > > >> > > > + .global _efistub_entry
> > > >> > > > +_efistub_entry:
> > > >> > >
> > > >> > > This should go into .text or .init.text, not the header.
> > > >> > >
> > > >> > > > + /* Reload the global pointer */
> > > >> > > > + load_global_pointer
> > > >> > > > +
> > > >> > >
> > > >> > > What is supposed to happen here if CONFIG_SHADOW_CALL_STACK=y? The EFI
> > > >> > > stub Makefile removes the SCS CFLAGS, so the stub will be built
> > > >> > > without shadow call stack support, which I guess means that it might
> > > >> > > use GP as a global pointer as usual?
> > > >> > >
> > > >> > > > + call __efistub_efi_pe_entry
> > > >> > > > + ret
> > > >> > > > +
> > > >> > >
> > > >> > > You are returning to the firmware here, but after modifying the GP
> > > >> > > register. Shouldn't you restore it to its old value?
> > > >> > There is no need to restore the value of the gp register. Where gp is
> > > >> > needed, the gp register must first be initialized. And here is the
> > > >> > entry.
> > > >> >
> > > >>
> > > >> But how should the firmware know that GP was corrupted after calling
> > > >> the kernel's EFI entrypoint? The EFI stub can return to the firmware
> > > >> if it encounters any errors while still running in the EFI boot
> > > >> services.
> > > >>
> > > >
> > > > Actually, I wonder if GP can be modified at all before
> > > > ExitBootServices(). The EFI timer interrupt is still live at this
> > > > point, and so the firmware is being called behind your back, and might
> > > > rely on GP retaining its original value.
> > >
> > > [A few of us are talking on IRC as I'm writing this...]
> > >
> > > The UEFI spec says "UEFI firmware must neither trust the
> > > values of tp and gp nor make an assumption of owning the write access to
> > > these register in any circumstances". It's kind of vague what "UEFI
> > > firmware" means here, but I think it's reasonable to assume that the
> > > kernel (and thus the EFI stub) is not included there.
> > >
> > > So under that interpretation, the kernel (including the EFI stub) would
> > > be allowed to overwrite GP with whatever it wants.
> > >
> >
> > OK, so even if the UEFI spec seems to suggest that using GP in EFI
> > applications such as the Linux EFI stub should be safe, I'd still like
> > to understand why this change is necessary. The patches you are
> > reverting are supposed to ensure that a) the compiler does not
> > generate references that can be relaxed to GP based ones, and b) no
> > R_RISCV_RELAX relocations are present in any of the code that runs in
> > the context of the EFI firmware.
> >
> > Are you still seeing GP based symbol references? Is there C code that
> > gets pulled into the EFI stub that uses GP based relocations perhaps?
> > (see list below). If any of those are implemented in C, they should
> > not be used by the EFI stub directly unless they are guaranteed to be
> > uninstrumented and callable at arbitrary offsets other than the one
> > they were linked to run at.
> >
> >
> > __efistub_memcmp = memcmp;
> > __efistub_memchr = memchr;
> > __efistub_memcpy = memcpy;
> > __efistub_memmove = memmove;
> > __efistub_memset = memset;
> > __efistub_strlen = strlen;
> > __efistub_strnlen = strnlen;
> > __efistub_strcmp = strcmp;
> > __efistub_strncmp = strncmp;
> > __efistub_strrchr = strrchr;
> > __efistub___memcpy = memcpy;
> > __efistub___memmove = memmove;
> > __efistub___memset = memset;
> > __efistub__start = _start;
> > __efistub__start_kernel = _start_kernel;
> >
> > (from arch/riscv/kernel/image-vars.h)
>
> Uhm never mind - these are all gone now, I was looking at a v6.1
> kernel source tree.
>
> So that means that, as far as I can tell, the only kernel C code that
> executes in the context of the EFI firmware is built with -mno-relax
> and is checked for the absence of R_RISCV_RELAX relocations. So I fail
> to see why these changes are needed.
>
> Yunhui, could you please explain the reason for this series?
From the logic of binutils, if "__global_pointer$" exists, it is
possible to use GP for optimization. For RISC-V, "__global_pointer$"
was introduced in commit "fbe934d69eb7e". Therefore, for the system as
a whole, we should keep using GP uniformly. The root cause of this
problem is that GP is not loaded, rather than "On RISC-V, we also
avoid GP based relocations..." as commit "d2baf8cc82c17" said. We need
to address problems head-on, rather than avoid them.
Thanks,
Yunhui
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [External] Re: [PATCH 3/3] efistub: fix missed the initialization of gp
2024-03-07 3:19 ` yunhui cui
@ 2024-03-07 16:48 ` Ard Biesheuvel
-1 siblings, 0 replies; 48+ messages in thread
From: Ard Biesheuvel @ 2024-03-07 16:48 UTC (permalink / raw)
To: yunhui cui
Cc: Palmer Dabbelt, Paul Walmsley, aou, xuzhipeng.1973, alexghiti,
samitolvanen, bp, xiao.w.wang, jan.kiszka, kirill.shutemov,
nathan, linux-riscv, linux-kernel, linux-efi, Conor Dooley,
Heinrich Schuchardt
On Thu, 7 Mar 2024 at 04:19, yunhui cui <cuiyunhui@bytedance.com> wrote:
>
> Hi Ard,
>
> On Thu, Mar 7, 2024 at 12:15 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Wed, 6 Mar 2024 at 16:44, Ard Biesheuvel <ardb@kernel.org> wrote:
> > >
> > > On Wed, 6 Mar 2024 at 16:21, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> > > >
> > > > On Wed, 06 Mar 2024 05:09:07 PST (-0800), Ard Biesheuvel wrote:
> > > > > On Wed, 6 Mar 2024 at 14:02, Ard Biesheuvel <ardb@kernel.org> wrote:
> > > > >>
> > > > >> On Wed, 6 Mar 2024 at 13:34, yunhui cui <cuiyunhui@bytedance.com> wrote:
> > > > >> >
> > > > >> > Hi Ard,
> > > > >> >
> > > > >> > On Wed, Mar 6, 2024 at 5:36 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> > > > >> > >
> > > > >> > > On Wed, 6 Mar 2024 at 09:56, Yunhui Cui <cuiyunhui@bytedance.com> wrote:
> > > > >> > > >
> > > > >> > > > Compared with gcc version 12, gcc version 13 uses the gp
> > > > >> > > > register for compilation optimization, but the efistub module
> > > > >> > > > does not initialize gp.
> > > > >> > > >
> > > > >> > > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> > > > >> > > > Co-Developed-by: Zhipeng Xu <xuzhipeng.1973@bytedance.com>
> > > > >> > >
> > > > >> > > This needs a sign-off, and your signoff needs to come after.
> > > > >> > >
> > > > >> > > > ---
> > > > >> > > > arch/riscv/kernel/efi-header.S | 11 ++++++++++-
> > > > >> > > > 1 file changed, 10 insertions(+), 1 deletion(-)
> > > > >> > > >
> > > > >> > > > diff --git a/arch/riscv/kernel/efi-header.S b/arch/riscv/kernel/efi-header.S
> > > > >> > > > index 515b2dfbca75..fa17c08c092a 100644
> > > > >> > > > --- a/arch/riscv/kernel/efi-header.S
> > > > >> > > > +++ b/arch/riscv/kernel/efi-header.S
> > > > >> > > > @@ -40,7 +40,7 @@ optional_header:
> > > > >> > > > .long __pecoff_data_virt_end - __pecoff_text_end // SizeOfInitializedData
> > > > >> > > > #endif
> > > > >> > > > .long 0 // SizeOfUninitializedData
> > > > >> > > > - .long __efistub_efi_pe_entry - _start // AddressOfEntryPoint
> > > > >> > > > + .long _efistub_entry - _start // AddressOfEntryPoint
> > > > >> > > > .long efi_header_end - _start // BaseOfCode
> > > > >> > > > #ifdef CONFIG_32BIT
> > > > >> > > > .long __pecoff_text_end - _start // BaseOfData
> > > > >> > > > @@ -121,4 +121,13 @@ section_table:
> > > > >> > > >
> > > > >> > > > .balign 0x1000
> > > > >> > > > efi_header_end:
> > > > >> > > > +
> > > > >> > > > + .global _efistub_entry
> > > > >> > > > +_efistub_entry:
> > > > >> > >
> > > > >> > > This should go into .text or .init.text, not the header.
> > > > >> > >
> > > > >> > > > + /* Reload the global pointer */
> > > > >> > > > + load_global_pointer
> > > > >> > > > +
> > > > >> > >
> > > > >> > > What is supposed to happen here if CONFIG_SHADOW_CALL_STACK=y? The EFI
> > > > >> > > stub Makefile removes the SCS CFLAGS, so the stub will be built
> > > > >> > > without shadow call stack support, which I guess means that it might
> > > > >> > > use GP as a global pointer as usual?
> > > > >> > >
> > > > >> > > > + call __efistub_efi_pe_entry
> > > > >> > > > + ret
> > > > >> > > > +
> > > > >> > >
> > > > >> > > You are returning to the firmware here, but after modifying the GP
> > > > >> > > register. Shouldn't you restore it to its old value?
> > > > >> > There is no need to restore the value of the gp register. Where gp is
> > > > >> > needed, the gp register must first be initialized. And here is the
> > > > >> > entry.
> > > > >> >
> > > > >>
> > > > >> But how should the firmware know that GP was corrupted after calling
> > > > >> the kernel's EFI entrypoint? The EFI stub can return to the firmware
> > > > >> if it encounters any errors while still running in the EFI boot
> > > > >> services.
> > > > >>
> > > > >
> > > > > Actually, I wonder if GP can be modified at all before
> > > > > ExitBootServices(). The EFI timer interrupt is still live at this
> > > > > point, and so the firmware is being called behind your back, and might
> > > > > rely on GP retaining its original value.
> > > >
> > > > [A few of us are talking on IRC as I'm writing this...]
> > > >
> > > > The UEFI spec says "UEFI firmware must neither trust the
> > > > values of tp and gp nor make an assumption of owning the write access to
> > > > these register in any circumstances". It's kind of vague what "UEFI
> > > > firmware" means here, but I think it's reasonable to assume that the
> > > > kernel (and thus the EFI stub) is not included there.
> > > >
> > > > So under that interpretation, the kernel (including the EFI stub) would
> > > > be allowed to overwrite GP with whatever it wants.
> > > >
> > >
> > > OK, so even if the UEFI spec seems to suggest that using GP in EFI
> > > applications such as the Linux EFI stub should be safe, I'd still like
> > > to understand why this change is necessary. The patches you are
> > > reverting are supposed to ensure that a) the compiler does not
> > > generate references that can be relaxed to GP based ones, and b) no
> > > R_RISCV_RELAX relocations are present in any of the code that runs in
> > > the context of the EFI firmware.
> > >
> > > Are you still seeing GP based symbol references? Is there C code that
> > > gets pulled into the EFI stub that uses GP based relocations perhaps?
> > > (see list below). If any of those are implemented in C, they should
> > > not be used by the EFI stub directly unless they are guaranteed to be
> > > uninstrumented and callable at arbitrary offsets other than the one
> > > they were linked to run at.
> > >
> > >
> > > __efistub_memcmp = memcmp;
> > > __efistub_memchr = memchr;
> > > __efistub_memcpy = memcpy;
> > > __efistub_memmove = memmove;
> > > __efistub_memset = memset;
> > > __efistub_strlen = strlen;
> > > __efistub_strnlen = strnlen;
> > > __efistub_strcmp = strcmp;
> > > __efistub_strncmp = strncmp;
> > > __efistub_strrchr = strrchr;
> > > __efistub___memcpy = memcpy;
> > > __efistub___memmove = memmove;
> > > __efistub___memset = memset;
> > > __efistub__start = _start;
> > > __efistub__start_kernel = _start_kernel;
> > >
> > > (from arch/riscv/kernel/image-vars.h)
> >
> > Uhm never mind - these are all gone now, I was looking at a v6.1
> > kernel source tree.
> >
> > So that means that, as far as I can tell, the only kernel C code that
> > executes in the context of the EFI firmware is built with -mno-relax
> > and is checked for the absence of R_RISCV_RELAX relocations. So I fail
> > to see why these changes are needed.
> >
> > Yunhui, could you please explain the reason for this series?
>
> From the logic of binutils, if "__global_pointer$" exists, it is
> possible to use GP for optimization. For RISC-V, "__global_pointer$"
> was introduced in commit "fbe934d69eb7e". Therefore, for the system as
> a whole, we should keep using GP uniformly.
There is no 'system as a whole' that can use GP 'uniformly'
The EFI stub is a separate executable that runs from a different
mapping of memory, in an execution context managed by the firmware. It
happens to be linked into the same executable as the vmlinux kernel.
> The root cause of this
> problem is that GP is not loaded, rather than "On RISC-V, we also
> avoid GP based relocations..." as commit "d2baf8cc82c17" said.
GP is not loaded because in the EFI firmware context, there is no safe
way to rely on it.
> We need
> to address problems head-on, rather than avoid them.
>
So what solution are you proposing for the potential GP conflicts
between the boot loader, the Linux EFI stub and the firmware?
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [External] Re: [PATCH 3/3] efistub: fix missed the initialization of gp
@ 2024-03-07 16:48 ` Ard Biesheuvel
0 siblings, 0 replies; 48+ messages in thread
From: Ard Biesheuvel @ 2024-03-07 16:48 UTC (permalink / raw)
To: yunhui cui
Cc: Palmer Dabbelt, Paul Walmsley, aou, xuzhipeng.1973, alexghiti,
samitolvanen, bp, xiao.w.wang, jan.kiszka, kirill.shutemov,
nathan, linux-riscv, linux-kernel, linux-efi, Conor Dooley,
Heinrich Schuchardt
On Thu, 7 Mar 2024 at 04:19, yunhui cui <cuiyunhui@bytedance.com> wrote:
>
> Hi Ard,
>
> On Thu, Mar 7, 2024 at 12:15 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Wed, 6 Mar 2024 at 16:44, Ard Biesheuvel <ardb@kernel.org> wrote:
> > >
> > > On Wed, 6 Mar 2024 at 16:21, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> > > >
> > > > On Wed, 06 Mar 2024 05:09:07 PST (-0800), Ard Biesheuvel wrote:
> > > > > On Wed, 6 Mar 2024 at 14:02, Ard Biesheuvel <ardb@kernel.org> wrote:
> > > > >>
> > > > >> On Wed, 6 Mar 2024 at 13:34, yunhui cui <cuiyunhui@bytedance.com> wrote:
> > > > >> >
> > > > >> > Hi Ard,
> > > > >> >
> > > > >> > On Wed, Mar 6, 2024 at 5:36 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> > > > >> > >
> > > > >> > > On Wed, 6 Mar 2024 at 09:56, Yunhui Cui <cuiyunhui@bytedance.com> wrote:
> > > > >> > > >
> > > > >> > > > Compared with gcc version 12, gcc version 13 uses the gp
> > > > >> > > > register for compilation optimization, but the efistub module
> > > > >> > > > does not initialize gp.
> > > > >> > > >
> > > > >> > > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> > > > >> > > > Co-Developed-by: Zhipeng Xu <xuzhipeng.1973@bytedance.com>
> > > > >> > >
> > > > >> > > This needs a sign-off, and your signoff needs to come after.
> > > > >> > >
> > > > >> > > > ---
> > > > >> > > > arch/riscv/kernel/efi-header.S | 11 ++++++++++-
> > > > >> > > > 1 file changed, 10 insertions(+), 1 deletion(-)
> > > > >> > > >
> > > > >> > > > diff --git a/arch/riscv/kernel/efi-header.S b/arch/riscv/kernel/efi-header.S
> > > > >> > > > index 515b2dfbca75..fa17c08c092a 100644
> > > > >> > > > --- a/arch/riscv/kernel/efi-header.S
> > > > >> > > > +++ b/arch/riscv/kernel/efi-header.S
> > > > >> > > > @@ -40,7 +40,7 @@ optional_header:
> > > > >> > > > .long __pecoff_data_virt_end - __pecoff_text_end // SizeOfInitializedData
> > > > >> > > > #endif
> > > > >> > > > .long 0 // SizeOfUninitializedData
> > > > >> > > > - .long __efistub_efi_pe_entry - _start // AddressOfEntryPoint
> > > > >> > > > + .long _efistub_entry - _start // AddressOfEntryPoint
> > > > >> > > > .long efi_header_end - _start // BaseOfCode
> > > > >> > > > #ifdef CONFIG_32BIT
> > > > >> > > > .long __pecoff_text_end - _start // BaseOfData
> > > > >> > > > @@ -121,4 +121,13 @@ section_table:
> > > > >> > > >
> > > > >> > > > .balign 0x1000
> > > > >> > > > efi_header_end:
> > > > >> > > > +
> > > > >> > > > + .global _efistub_entry
> > > > >> > > > +_efistub_entry:
> > > > >> > >
> > > > >> > > This should go into .text or .init.text, not the header.
> > > > >> > >
> > > > >> > > > + /* Reload the global pointer */
> > > > >> > > > + load_global_pointer
> > > > >> > > > +
> > > > >> > >
> > > > >> > > What is supposed to happen here if CONFIG_SHADOW_CALL_STACK=y? The EFI
> > > > >> > > stub Makefile removes the SCS CFLAGS, so the stub will be built
> > > > >> > > without shadow call stack support, which I guess means that it might
> > > > >> > > use GP as a global pointer as usual?
> > > > >> > >
> > > > >> > > > + call __efistub_efi_pe_entry
> > > > >> > > > + ret
> > > > >> > > > +
> > > > >> > >
> > > > >> > > You are returning to the firmware here, but after modifying the GP
> > > > >> > > register. Shouldn't you restore it to its old value?
> > > > >> > There is no need to restore the value of the gp register. Where gp is
> > > > >> > needed, the gp register must first be initialized. And here is the
> > > > >> > entry.
> > > > >> >
> > > > >>
> > > > >> But how should the firmware know that GP was corrupted after calling
> > > > >> the kernel's EFI entrypoint? The EFI stub can return to the firmware
> > > > >> if it encounters any errors while still running in the EFI boot
> > > > >> services.
> > > > >>
> > > > >
> > > > > Actually, I wonder if GP can be modified at all before
> > > > > ExitBootServices(). The EFI timer interrupt is still live at this
> > > > > point, and so the firmware is being called behind your back, and might
> > > > > rely on GP retaining its original value.
> > > >
> > > > [A few of us are talking on IRC as I'm writing this...]
> > > >
> > > > The UEFI spec says "UEFI firmware must neither trust the
> > > > values of tp and gp nor make an assumption of owning the write access to
> > > > these register in any circumstances". It's kind of vague what "UEFI
> > > > firmware" means here, but I think it's reasonable to assume that the
> > > > kernel (and thus the EFI stub) is not included there.
> > > >
> > > > So under that interpretation, the kernel (including the EFI stub) would
> > > > be allowed to overwrite GP with whatever it wants.
> > > >
> > >
> > > OK, so even if the UEFI spec seems to suggest that using GP in EFI
> > > applications such as the Linux EFI stub should be safe, I'd still like
> > > to understand why this change is necessary. The patches you are
> > > reverting are supposed to ensure that a) the compiler does not
> > > generate references that can be relaxed to GP based ones, and b) no
> > > R_RISCV_RELAX relocations are present in any of the code that runs in
> > > the context of the EFI firmware.
> > >
> > > Are you still seeing GP based symbol references? Is there C code that
> > > gets pulled into the EFI stub that uses GP based relocations perhaps?
> > > (see list below). If any of those are implemented in C, they should
> > > not be used by the EFI stub directly unless they are guaranteed to be
> > > uninstrumented and callable at arbitrary offsets other than the one
> > > they were linked to run at.
> > >
> > >
> > > __efistub_memcmp = memcmp;
> > > __efistub_memchr = memchr;
> > > __efistub_memcpy = memcpy;
> > > __efistub_memmove = memmove;
> > > __efistub_memset = memset;
> > > __efistub_strlen = strlen;
> > > __efistub_strnlen = strnlen;
> > > __efistub_strcmp = strcmp;
> > > __efistub_strncmp = strncmp;
> > > __efistub_strrchr = strrchr;
> > > __efistub___memcpy = memcpy;
> > > __efistub___memmove = memmove;
> > > __efistub___memset = memset;
> > > __efistub__start = _start;
> > > __efistub__start_kernel = _start_kernel;
> > >
> > > (from arch/riscv/kernel/image-vars.h)
> >
> > Uhm never mind - these are all gone now, I was looking at a v6.1
> > kernel source tree.
> >
> > So that means that, as far as I can tell, the only kernel C code that
> > executes in the context of the EFI firmware is built with -mno-relax
> > and is checked for the absence of R_RISCV_RELAX relocations. So I fail
> > to see why these changes are needed.
> >
> > Yunhui, could you please explain the reason for this series?
>
> From the logic of binutils, if "__global_pointer$" exists, it is
> possible to use GP for optimization. For RISC-V, "__global_pointer$"
> was introduced in commit "fbe934d69eb7e". Therefore, for the system as
> a whole, we should keep using GP uniformly.
There is no 'system as a whole' that can use GP 'uniformly'
The EFI stub is a separate executable that runs from a different
mapping of memory, in an execution context managed by the firmware. It
happens to be linked into the same executable as the vmlinux kernel.
> The root cause of this
> problem is that GP is not loaded, rather than "On RISC-V, we also
> avoid GP based relocations..." as commit "d2baf8cc82c17" said.
GP is not loaded because in the EFI firmware context, there is no safe
way to rely on it.
> We need
> to address problems head-on, rather than avoid them.
>
So what solution are you proposing for the potential GP conflicts
between the boot loader, the Linux EFI stub and the firmware?
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [External] Re: [PATCH 3/3] efistub: fix missed the initialization of gp
2024-03-07 16:48 ` Ard Biesheuvel
@ 2024-03-08 7:09 ` yunhui cui
-1 siblings, 0 replies; 48+ messages in thread
From: yunhui cui @ 2024-03-08 7:09 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Palmer Dabbelt, Paul Walmsley, aou, xuzhipeng.1973, alexghiti,
samitolvanen, bp, xiao.w.wang, jan.kiszka, kirill.shutemov,
nathan, linux-riscv, linux-kernel, linux-efi, Conor Dooley,
Heinrich Schuchardt
Hi Ard,
On Fri, Mar 8, 2024 at 12:49 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Thu, 7 Mar 2024 at 04:19, yunhui cui <cuiyunhui@bytedance.com> wrote:
> >
> > Hi Ard,
> >
> > On Thu, Mar 7, 2024 at 12:15 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> > >
> > > On Wed, 6 Mar 2024 at 16:44, Ard Biesheuvel <ardb@kernel.org> wrote:
> > > >
> > > > On Wed, 6 Mar 2024 at 16:21, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> > > > >
> > > > > On Wed, 06 Mar 2024 05:09:07 PST (-0800), Ard Biesheuvel wrote:
> > > > > > On Wed, 6 Mar 2024 at 14:02, Ard Biesheuvel <ardb@kernel.org> wrote:
> > > > > >>
> > > > > >> On Wed, 6 Mar 2024 at 13:34, yunhui cui <cuiyunhui@bytedance.com> wrote:
> > > > > >> >
> > > > > >> > Hi Ard,
> > > > > >> >
> > > > > >> > On Wed, Mar 6, 2024 at 5:36 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> > > > > >> > >
> > > > > >> > > On Wed, 6 Mar 2024 at 09:56, Yunhui Cui <cuiyunhui@bytedance.com> wrote:
> > > > > >> > > >
> > > > > >> > > > Compared with gcc version 12, gcc version 13 uses the gp
> > > > > >> > > > register for compilation optimization, but the efistub module
> > > > > >> > > > does not initialize gp.
> > > > > >> > > >
> > > > > >> > > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> > > > > >> > > > Co-Developed-by: Zhipeng Xu <xuzhipeng.1973@bytedance.com>
> > > > > >> > >
> > > > > >> > > This needs a sign-off, and your signoff needs to come after.
> > > > > >> > >
> > > > > >> > > > ---
> > > > > >> > > > arch/riscv/kernel/efi-header.S | 11 ++++++++++-
> > > > > >> > > > 1 file changed, 10 insertions(+), 1 deletion(-)
> > > > > >> > > >
> > > > > >> > > > diff --git a/arch/riscv/kernel/efi-header.S b/arch/riscv/kernel/efi-header.S
> > > > > >> > > > index 515b2dfbca75..fa17c08c092a 100644
> > > > > >> > > > --- a/arch/riscv/kernel/efi-header.S
> > > > > >> > > > +++ b/arch/riscv/kernel/efi-header.S
> > > > > >> > > > @@ -40,7 +40,7 @@ optional_header:
> > > > > >> > > > .long __pecoff_data_virt_end - __pecoff_text_end // SizeOfInitializedData
> > > > > >> > > > #endif
> > > > > >> > > > .long 0 // SizeOfUninitializedData
> > > > > >> > > > - .long __efistub_efi_pe_entry - _start // AddressOfEntryPoint
> > > > > >> > > > + .long _efistub_entry - _start // AddressOfEntryPoint
> > > > > >> > > > .long efi_header_end - _start // BaseOfCode
> > > > > >> > > > #ifdef CONFIG_32BIT
> > > > > >> > > > .long __pecoff_text_end - _start // BaseOfData
> > > > > >> > > > @@ -121,4 +121,13 @@ section_table:
> > > > > >> > > >
> > > > > >> > > > .balign 0x1000
> > > > > >> > > > efi_header_end:
> > > > > >> > > > +
> > > > > >> > > > + .global _efistub_entry
> > > > > >> > > > +_efistub_entry:
> > > > > >> > >
> > > > > >> > > This should go into .text or .init.text, not the header.
> > > > > >> > >
> > > > > >> > > > + /* Reload the global pointer */
> > > > > >> > > > + load_global_pointer
> > > > > >> > > > +
> > > > > >> > >
> > > > > >> > > What is supposed to happen here if CONFIG_SHADOW_CALL_STACK=y? The EFI
> > > > > >> > > stub Makefile removes the SCS CFLAGS, so the stub will be built
> > > > > >> > > without shadow call stack support, which I guess means that it might
> > > > > >> > > use GP as a global pointer as usual?
> > > > > >> > >
> > > > > >> > > > + call __efistub_efi_pe_entry
> > > > > >> > > > + ret
> > > > > >> > > > +
> > > > > >> > >
> > > > > >> > > You are returning to the firmware here, but after modifying the GP
> > > > > >> > > register. Shouldn't you restore it to its old value?
> > > > > >> > There is no need to restore the value of the gp register. Where gp is
> > > > > >> > needed, the gp register must first be initialized. And here is the
> > > > > >> > entry.
> > > > > >> >
> > > > > >>
> > > > > >> But how should the firmware know that GP was corrupted after calling
> > > > > >> the kernel's EFI entrypoint? The EFI stub can return to the firmware
> > > > > >> if it encounters any errors while still running in the EFI boot
> > > > > >> services.
> > > > > >>
> > > > > >
> > > > > > Actually, I wonder if GP can be modified at all before
> > > > > > ExitBootServices(). The EFI timer interrupt is still live at this
> > > > > > point, and so the firmware is being called behind your back, and might
> > > > > > rely on GP retaining its original value.
> > > > >
> > > > > [A few of us are talking on IRC as I'm writing this...]
> > > > >
> > > > > The UEFI spec says "UEFI firmware must neither trust the
> > > > > values of tp and gp nor make an assumption of owning the write access to
> > > > > these register in any circumstances". It's kind of vague what "UEFI
> > > > > firmware" means here, but I think it's reasonable to assume that the
> > > > > kernel (and thus the EFI stub) is not included there.
> > > > >
> > > > > So under that interpretation, the kernel (including the EFI stub) would
> > > > > be allowed to overwrite GP with whatever it wants.
> > > > >
> > > >
> > > > OK, so even if the UEFI spec seems to suggest that using GP in EFI
> > > > applications such as the Linux EFI stub should be safe, I'd still like
> > > > to understand why this change is necessary. The patches you are
> > > > reverting are supposed to ensure that a) the compiler does not
> > > > generate references that can be relaxed to GP based ones, and b) no
> > > > R_RISCV_RELAX relocations are present in any of the code that runs in
> > > > the context of the EFI firmware.
> > > >
> > > > Are you still seeing GP based symbol references? Is there C code that
> > > > gets pulled into the EFI stub that uses GP based relocations perhaps?
> > > > (see list below). If any of those are implemented in C, they should
> > > > not be used by the EFI stub directly unless they are guaranteed to be
> > > > uninstrumented and callable at arbitrary offsets other than the one
> > > > they were linked to run at.
> > > >
> > > >
> > > > __efistub_memcmp = memcmp;
> > > > __efistub_memchr = memchr;
> > > > __efistub_memcpy = memcpy;
> > > > __efistub_memmove = memmove;
> > > > __efistub_memset = memset;
> > > > __efistub_strlen = strlen;
> > > > __efistub_strnlen = strnlen;
> > > > __efistub_strcmp = strcmp;
> > > > __efistub_strncmp = strncmp;
> > > > __efistub_strrchr = strrchr;
> > > > __efistub___memcpy = memcpy;
> > > > __efistub___memmove = memmove;
> > > > __efistub___memset = memset;
> > > > __efistub__start = _start;
> > > > __efistub__start_kernel = _start_kernel;
> > > >
> > > > (from arch/riscv/kernel/image-vars.h)
> > >
> > > Uhm never mind - these are all gone now, I was looking at a v6.1
> > > kernel source tree.
> > >
> > > So that means that, as far as I can tell, the only kernel C code that
> > > executes in the context of the EFI firmware is built with -mno-relax
> > > and is checked for the absence of R_RISCV_RELAX relocations. So I fail
> > > to see why these changes are needed.
> > >
> > > Yunhui, could you please explain the reason for this series?
> >
> > From the logic of binutils, if "__global_pointer$" exists, it is
> > possible to use GP for optimization. For RISC-V, "__global_pointer$"
> > was introduced in commit "fbe934d69eb7e". Therefore, for the system as
> > a whole, we should keep using GP uniformly.
>
> There is no 'system as a whole' that can use GP 'uniformly'
>
> The EFI stub is a separate executable that runs from a different
> mapping of memory, in an execution context managed by the firmware. It
> happens to be linked into the same executable as the vmlinux kernel.
>
> > The root cause of this
> > problem is that GP is not loaded, rather than "On RISC-V, we also
> > avoid GP based relocations..." as commit "d2baf8cc82c17" said.
>
> GP is not loaded because in the EFI firmware context, there is no safe
> way to rely on it.
>
> > We need
> > to address problems head-on, rather than avoid them.
> >
>
> So what solution are you proposing for the potential GP conflicts
> between the boot loader, the Linux EFI stub and the firmware?
The GP register values are now loaded in the arch/riscv/kernel/head.S
and arch/riscv/kernel/suspend_entry.S files.
Let's think about EFI runtimeservice. If the EFI firmware code uses GP
registers but the compiler does not avoid GP, and kernel uses the
callback function provided by EFI, is there a problem? Is it possible
to solve the problem only by making the firmware code not use GP at
all and compiling options to avoid using GP?
The same goes for efistub.
So the way to solve this problem is that the firmware does not use GP
optimization. Does this allow efistub to load the GP register?
Thanks,
Yunhui
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [External] Re: [PATCH 3/3] efistub: fix missed the initialization of gp
@ 2024-03-08 7:09 ` yunhui cui
0 siblings, 0 replies; 48+ messages in thread
From: yunhui cui @ 2024-03-08 7:09 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Palmer Dabbelt, Paul Walmsley, aou, xuzhipeng.1973, alexghiti,
samitolvanen, bp, xiao.w.wang, jan.kiszka, kirill.shutemov,
nathan, linux-riscv, linux-kernel, linux-efi, Conor Dooley,
Heinrich Schuchardt
Hi Ard,
On Fri, Mar 8, 2024 at 12:49 AM Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Thu, 7 Mar 2024 at 04:19, yunhui cui <cuiyunhui@bytedance.com> wrote:
> >
> > Hi Ard,
> >
> > On Thu, Mar 7, 2024 at 12:15 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> > >
> > > On Wed, 6 Mar 2024 at 16:44, Ard Biesheuvel <ardb@kernel.org> wrote:
> > > >
> > > > On Wed, 6 Mar 2024 at 16:21, Palmer Dabbelt <palmer@dabbelt.com> wrote:
> > > > >
> > > > > On Wed, 06 Mar 2024 05:09:07 PST (-0800), Ard Biesheuvel wrote:
> > > > > > On Wed, 6 Mar 2024 at 14:02, Ard Biesheuvel <ardb@kernel.org> wrote:
> > > > > >>
> > > > > >> On Wed, 6 Mar 2024 at 13:34, yunhui cui <cuiyunhui@bytedance.com> wrote:
> > > > > >> >
> > > > > >> > Hi Ard,
> > > > > >> >
> > > > > >> > On Wed, Mar 6, 2024 at 5:36 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> > > > > >> > >
> > > > > >> > > On Wed, 6 Mar 2024 at 09:56, Yunhui Cui <cuiyunhui@bytedance.com> wrote:
> > > > > >> > > >
> > > > > >> > > > Compared with gcc version 12, gcc version 13 uses the gp
> > > > > >> > > > register for compilation optimization, but the efistub module
> > > > > >> > > > does not initialize gp.
> > > > > >> > > >
> > > > > >> > > > Signed-off-by: Yunhui Cui <cuiyunhui@bytedance.com>
> > > > > >> > > > Co-Developed-by: Zhipeng Xu <xuzhipeng.1973@bytedance.com>
> > > > > >> > >
> > > > > >> > > This needs a sign-off, and your signoff needs to come after.
> > > > > >> > >
> > > > > >> > > > ---
> > > > > >> > > > arch/riscv/kernel/efi-header.S | 11 ++++++++++-
> > > > > >> > > > 1 file changed, 10 insertions(+), 1 deletion(-)
> > > > > >> > > >
> > > > > >> > > > diff --git a/arch/riscv/kernel/efi-header.S b/arch/riscv/kernel/efi-header.S
> > > > > >> > > > index 515b2dfbca75..fa17c08c092a 100644
> > > > > >> > > > --- a/arch/riscv/kernel/efi-header.S
> > > > > >> > > > +++ b/arch/riscv/kernel/efi-header.S
> > > > > >> > > > @@ -40,7 +40,7 @@ optional_header:
> > > > > >> > > > .long __pecoff_data_virt_end - __pecoff_text_end // SizeOfInitializedData
> > > > > >> > > > #endif
> > > > > >> > > > .long 0 // SizeOfUninitializedData
> > > > > >> > > > - .long __efistub_efi_pe_entry - _start // AddressOfEntryPoint
> > > > > >> > > > + .long _efistub_entry - _start // AddressOfEntryPoint
> > > > > >> > > > .long efi_header_end - _start // BaseOfCode
> > > > > >> > > > #ifdef CONFIG_32BIT
> > > > > >> > > > .long __pecoff_text_end - _start // BaseOfData
> > > > > >> > > > @@ -121,4 +121,13 @@ section_table:
> > > > > >> > > >
> > > > > >> > > > .balign 0x1000
> > > > > >> > > > efi_header_end:
> > > > > >> > > > +
> > > > > >> > > > + .global _efistub_entry
> > > > > >> > > > +_efistub_entry:
> > > > > >> > >
> > > > > >> > > This should go into .text or .init.text, not the header.
> > > > > >> > >
> > > > > >> > > > + /* Reload the global pointer */
> > > > > >> > > > + load_global_pointer
> > > > > >> > > > +
> > > > > >> > >
> > > > > >> > > What is supposed to happen here if CONFIG_SHADOW_CALL_STACK=y? The EFI
> > > > > >> > > stub Makefile removes the SCS CFLAGS, so the stub will be built
> > > > > >> > > without shadow call stack support, which I guess means that it might
> > > > > >> > > use GP as a global pointer as usual?
> > > > > >> > >
> > > > > >> > > > + call __efistub_efi_pe_entry
> > > > > >> > > > + ret
> > > > > >> > > > +
> > > > > >> > >
> > > > > >> > > You are returning to the firmware here, but after modifying the GP
> > > > > >> > > register. Shouldn't you restore it to its old value?
> > > > > >> > There is no need to restore the value of the gp register. Where gp is
> > > > > >> > needed, the gp register must first be initialized. And here is the
> > > > > >> > entry.
> > > > > >> >
> > > > > >>
> > > > > >> But how should the firmware know that GP was corrupted after calling
> > > > > >> the kernel's EFI entrypoint? The EFI stub can return to the firmware
> > > > > >> if it encounters any errors while still running in the EFI boot
> > > > > >> services.
> > > > > >>
> > > > > >
> > > > > > Actually, I wonder if GP can be modified at all before
> > > > > > ExitBootServices(). The EFI timer interrupt is still live at this
> > > > > > point, and so the firmware is being called behind your back, and might
> > > > > > rely on GP retaining its original value.
> > > > >
> > > > > [A few of us are talking on IRC as I'm writing this...]
> > > > >
> > > > > The UEFI spec says "UEFI firmware must neither trust the
> > > > > values of tp and gp nor make an assumption of owning the write access to
> > > > > these register in any circumstances". It's kind of vague what "UEFI
> > > > > firmware" means here, but I think it's reasonable to assume that the
> > > > > kernel (and thus the EFI stub) is not included there.
> > > > >
> > > > > So under that interpretation, the kernel (including the EFI stub) would
> > > > > be allowed to overwrite GP with whatever it wants.
> > > > >
> > > >
> > > > OK, so even if the UEFI spec seems to suggest that using GP in EFI
> > > > applications such as the Linux EFI stub should be safe, I'd still like
> > > > to understand why this change is necessary. The patches you are
> > > > reverting are supposed to ensure that a) the compiler does not
> > > > generate references that can be relaxed to GP based ones, and b) no
> > > > R_RISCV_RELAX relocations are present in any of the code that runs in
> > > > the context of the EFI firmware.
> > > >
> > > > Are you still seeing GP based symbol references? Is there C code that
> > > > gets pulled into the EFI stub that uses GP based relocations perhaps?
> > > > (see list below). If any of those are implemented in C, they should
> > > > not be used by the EFI stub directly unless they are guaranteed to be
> > > > uninstrumented and callable at arbitrary offsets other than the one
> > > > they were linked to run at.
> > > >
> > > >
> > > > __efistub_memcmp = memcmp;
> > > > __efistub_memchr = memchr;
> > > > __efistub_memcpy = memcpy;
> > > > __efistub_memmove = memmove;
> > > > __efistub_memset = memset;
> > > > __efistub_strlen = strlen;
> > > > __efistub_strnlen = strnlen;
> > > > __efistub_strcmp = strcmp;
> > > > __efistub_strncmp = strncmp;
> > > > __efistub_strrchr = strrchr;
> > > > __efistub___memcpy = memcpy;
> > > > __efistub___memmove = memmove;
> > > > __efistub___memset = memset;
> > > > __efistub__start = _start;
> > > > __efistub__start_kernel = _start_kernel;
> > > >
> > > > (from arch/riscv/kernel/image-vars.h)
> > >
> > > Uhm never mind - these are all gone now, I was looking at a v6.1
> > > kernel source tree.
> > >
> > > So that means that, as far as I can tell, the only kernel C code that
> > > executes in the context of the EFI firmware is built with -mno-relax
> > > and is checked for the absence of R_RISCV_RELAX relocations. So I fail
> > > to see why these changes are needed.
> > >
> > > Yunhui, could you please explain the reason for this series?
> >
> > From the logic of binutils, if "__global_pointer$" exists, it is
> > possible to use GP for optimization. For RISC-V, "__global_pointer$"
> > was introduced in commit "fbe934d69eb7e". Therefore, for the system as
> > a whole, we should keep using GP uniformly.
>
> There is no 'system as a whole' that can use GP 'uniformly'
>
> The EFI stub is a separate executable that runs from a different
> mapping of memory, in an execution context managed by the firmware. It
> happens to be linked into the same executable as the vmlinux kernel.
>
> > The root cause of this
> > problem is that GP is not loaded, rather than "On RISC-V, we also
> > avoid GP based relocations..." as commit "d2baf8cc82c17" said.
>
> GP is not loaded because in the EFI firmware context, there is no safe
> way to rely on it.
>
> > We need
> > to address problems head-on, rather than avoid them.
> >
>
> So what solution are you proposing for the potential GP conflicts
> between the boot loader, the Linux EFI stub and the firmware?
The GP register values are now loaded in the arch/riscv/kernel/head.S
and arch/riscv/kernel/suspend_entry.S files.
Let's think about EFI runtimeservice. If the EFI firmware code uses GP
registers but the compiler does not avoid GP, and kernel uses the
callback function provided by EFI, is there a problem? Is it possible
to solve the problem only by making the firmware code not use GP at
all and compiling options to avoid using GP?
The same goes for efistub.
So the way to solve this problem is that the firmware does not use GP
optimization. Does this allow efistub to load the GP register?
Thanks,
Yunhui
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [External] Re: [PATCH 3/3] efistub: fix missed the initialization of gp
2024-03-08 7:09 ` yunhui cui
@ 2024-03-08 8:16 ` Ard Biesheuvel
-1 siblings, 0 replies; 48+ messages in thread
From: Ard Biesheuvel @ 2024-03-08 8:16 UTC (permalink / raw)
To: yunhui cui
Cc: Palmer Dabbelt, Paul Walmsley, aou, xuzhipeng.1973, alexghiti,
samitolvanen, bp, xiao.w.wang, jan.kiszka, kirill.shutemov,
nathan, linux-riscv, linux-kernel, linux-efi, Conor Dooley,
Heinrich Schuchardt
On Fri, 8 Mar 2024 at 08:10, yunhui cui <cuiyunhui@bytedance.com> wrote:
>
> Hi Ard,
>
> On Fri, Mar 8, 2024 at 12:49 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Thu, 7 Mar 2024 at 04:19, yunhui cui <cuiyunhui@bytedance.com> wrote:
> > >
...
> > >
> > > From the logic of binutils, if "__global_pointer$" exists, it is
> > > possible to use GP for optimization. For RISC-V, "__global_pointer$"
> > > was introduced in commit "fbe934d69eb7e". Therefore, for the system as
> > > a whole, we should keep using GP uniformly.
> >
> > There is no 'system as a whole' that can use GP 'uniformly'
> >
> > The EFI stub is a separate executable that runs from a different
> > mapping of memory, in an execution context managed by the firmware. It
> > happens to be linked into the same executable as the vmlinux kernel.
> >
> > > The root cause of this
> > > problem is that GP is not loaded, rather than "On RISC-V, we also
> > > avoid GP based relocations..." as commit "d2baf8cc82c17" said.
> >
> > GP is not loaded because in the EFI firmware context, there is no safe
> > way to rely on it.
> >
> > > We need
> > > to address problems head-on, rather than avoid them.
> > >
> >
> > So what solution are you proposing for the potential GP conflicts
> > between the boot loader, the Linux EFI stub and the firmware?
>
>
> The GP register values are now loaded in the arch/riscv/kernel/head.S
> and arch/riscv/kernel/suspend_entry.S files.
>
> Let's think about EFI runtimeservice. If the EFI firmware code uses GP
> registers but the compiler does not avoid GP, and kernel uses the
> callback function provided by EFI, is there a problem? Is it possible
> to solve the problem only by making the firmware code not use GP at
> all and compiling options to avoid using GP?
>
EFI runtime services do not use callbacks, and execute in a context
that is entirely owned by the OS. So this is one place where EFI
firmware cannot use GP at all even if the UEFI spec permitted it.
> The same goes for efistub.
>
Not really. The UEFI spec seems to suggest that *system* firmware
should not touch GP or make any assumptions about its value, but it
doesn't say anything about EFI applications such as the EFI stub or
GRUB.
> So the way to solve this problem is that the firmware does not use GP
> optimization. Does this allow efistub to load the GP register?
>
What about GRUB or other bootloaders that are loaded before the
kernel, but are still active while the EFI stub is executing? Who gets
to own GP in this scenario?
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [External] Re: [PATCH 3/3] efistub: fix missed the initialization of gp
@ 2024-03-08 8:16 ` Ard Biesheuvel
0 siblings, 0 replies; 48+ messages in thread
From: Ard Biesheuvel @ 2024-03-08 8:16 UTC (permalink / raw)
To: yunhui cui
Cc: Palmer Dabbelt, Paul Walmsley, aou, xuzhipeng.1973, alexghiti,
samitolvanen, bp, xiao.w.wang, jan.kiszka, kirill.shutemov,
nathan, linux-riscv, linux-kernel, linux-efi, Conor Dooley,
Heinrich Schuchardt
On Fri, 8 Mar 2024 at 08:10, yunhui cui <cuiyunhui@bytedance.com> wrote:
>
> Hi Ard,
>
> On Fri, Mar 8, 2024 at 12:49 AM Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Thu, 7 Mar 2024 at 04:19, yunhui cui <cuiyunhui@bytedance.com> wrote:
> > >
...
> > >
> > > From the logic of binutils, if "__global_pointer$" exists, it is
> > > possible to use GP for optimization. For RISC-V, "__global_pointer$"
> > > was introduced in commit "fbe934d69eb7e". Therefore, for the system as
> > > a whole, we should keep using GP uniformly.
> >
> > There is no 'system as a whole' that can use GP 'uniformly'
> >
> > The EFI stub is a separate executable that runs from a different
> > mapping of memory, in an execution context managed by the firmware. It
> > happens to be linked into the same executable as the vmlinux kernel.
> >
> > > The root cause of this
> > > problem is that GP is not loaded, rather than "On RISC-V, we also
> > > avoid GP based relocations..." as commit "d2baf8cc82c17" said.
> >
> > GP is not loaded because in the EFI firmware context, there is no safe
> > way to rely on it.
> >
> > > We need
> > > to address problems head-on, rather than avoid them.
> > >
> >
> > So what solution are you proposing for the potential GP conflicts
> > between the boot loader, the Linux EFI stub and the firmware?
>
>
> The GP register values are now loaded in the arch/riscv/kernel/head.S
> and arch/riscv/kernel/suspend_entry.S files.
>
> Let's think about EFI runtimeservice. If the EFI firmware code uses GP
> registers but the compiler does not avoid GP, and kernel uses the
> callback function provided by EFI, is there a problem? Is it possible
> to solve the problem only by making the firmware code not use GP at
> all and compiling options to avoid using GP?
>
EFI runtime services do not use callbacks, and execute in a context
that is entirely owned by the OS. So this is one place where EFI
firmware cannot use GP at all even if the UEFI spec permitted it.
> The same goes for efistub.
>
Not really. The UEFI spec seems to suggest that *system* firmware
should not touch GP or make any assumptions about its value, but it
doesn't say anything about EFI applications such as the EFI stub or
GRUB.
> So the way to solve this problem is that the firmware does not use GP
> optimization. Does this allow efistub to load the GP register?
>
What about GRUB or other bootloaders that are loaded before the
kernel, but are still active while the EFI stub is executing? Who gets
to own GP in this scenario?
_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv
^ permalink raw reply [flat|nested] 48+ messages in thread
end of thread, other threads:[~2024-03-08 8:16 UTC | newest]
Thread overview: 48+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-03-06 8:56 [PATCH 1/3] Revert "riscv/efistub: Ensure GP-relative addressing is not used" Yunhui Cui
2024-03-06 8:56 ` Yunhui Cui
2024-03-06 8:56 ` [PATCH 2/3] Revert "riscv/efistub: Tighten ELF relocation check" Yunhui Cui
2024-03-06 8:56 ` Yunhui Cui
2024-03-06 8:56 ` [PATCH 3/3] efistub: fix missed the initialization of gp Yunhui Cui
2024-03-06 8:56 ` Yunhui Cui
2024-03-06 9:36 ` Ard Biesheuvel
2024-03-06 9:36 ` Ard Biesheuvel
2024-03-06 12:34 ` [External] " yunhui cui
2024-03-06 12:34 ` yunhui cui
2024-03-06 13:02 ` Ard Biesheuvel
2024-03-06 13:02 ` Ard Biesheuvel
2024-03-06 13:09 ` Ard Biesheuvel
2024-03-06 13:09 ` Ard Biesheuvel
2024-03-06 13:30 ` yunhui cui
2024-03-06 13:30 ` yunhui cui
2024-03-06 15:21 ` Palmer Dabbelt
2024-03-06 15:21 ` Palmer Dabbelt
2024-03-06 15:44 ` Ard Biesheuvel
2024-03-06 15:44 ` Ard Biesheuvel
2024-03-06 16:15 ` Ard Biesheuvel
2024-03-06 16:15 ` Ard Biesheuvel
2024-03-06 17:11 ` Ard Biesheuvel
2024-03-06 17:11 ` Ard Biesheuvel
2024-03-07 3:19 ` yunhui cui
2024-03-07 3:19 ` yunhui cui
2024-03-07 16:48 ` Ard Biesheuvel
2024-03-07 16:48 ` Ard Biesheuvel
2024-03-08 7:09 ` yunhui cui
2024-03-08 7:09 ` yunhui cui
2024-03-08 8:16 ` Ard Biesheuvel
2024-03-08 8:16 ` Ard Biesheuvel
2024-03-06 9:02 ` [PATCH 1/3] Revert "riscv/efistub: Ensure GP-relative addressing is not used" Conor Dooley
2024-03-06 9:02 ` Conor Dooley
2024-03-06 9:38 ` Ard Biesheuvel
2024-03-06 9:38 ` Ard Biesheuvel
2024-03-06 12:37 ` [External] " yunhui cui
2024-03-06 12:37 ` yunhui cui
2024-03-06 12:52 ` Jan Kiszka
2024-03-06 12:52 ` Jan Kiszka
2024-03-06 13:07 ` [External] " yunhui cui
2024-03-06 13:07 ` yunhui cui
2024-03-06 13:11 ` Ard Biesheuvel
2024-03-06 13:11 ` Ard Biesheuvel
2024-03-06 13:26 ` yunhui cui
2024-03-06 13:26 ` yunhui cui
2024-03-06 13:46 ` Ard Biesheuvel
2024-03-06 13:46 ` Ard Biesheuvel
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.