| Message ID | 1405415402-3427-1-git-send-email-ard.biesheuvel@linaro.org |
|---|---|
| State | New |
| Headers | show |
Hi Ard, On Tue, Jul 15, 2014 at 10:10:02AM +0100, Ard Biesheuvel wrote: > After the EFI stub has done its business, it jumps into the kernel by branching > to offset #0 of the loaded Image, which is where it expects to find the header > containing a 'branch to stext' instruction. > However, the header is not covered by any PE/COFF section, so the header may > not actually be loaded at the expected offset. So instead, jump to 'stext' > directly, which is at the base of the PE/COFF .text section. It would be nice to point out in the commit message that the other changes in the patch are just cleanup to use stext_offset rather than open-coding it. > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> > --- > arch/arm64/kernel/efi-entry.S | 2 +- > arch/arm64/kernel/head.S | 10 ++++++---- > 2 files changed, 7 insertions(+), 5 deletions(-) > > diff --git a/arch/arm64/kernel/efi-entry.S b/arch/arm64/kernel/efi-entry.S > index 619b1dd7bcde..6ef541731d9e 100644 > --- a/arch/arm64/kernel/efi-entry.S > +++ b/arch/arm64/kernel/efi-entry.S > @@ -61,7 +61,7 @@ ENTRY(efi_stub_entry) > */ > mov x20, x0 // DTB address > ldr x0, [sp, #16] // relocated _text address > - mov x21, x0 > + add x21, x0, #:lo12:stext_offset I think we can drop the :lo12: here, which will allow us to have a warning if stext_offset is unexpectedly large (I believe this will currently silently mask bits were that to happen?). Other than that, this looks like a sensible thing to do given that we cannot rely on the header being present. Cheers, Mark. > > /* > * Flush dcache covering current runtime addresses > diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S > index a2c1195abb7f..78ddae28b090 100644 > --- a/arch/arm64/kernel/head.S > +++ b/arch/arm64/kernel/head.S > @@ -137,6 +137,8 @@ efi_head: > #endif > > #ifdef CONFIG_EFI > + .globl stext_offset > + .set stext_offset, stext - efi_head > .align 3 > pe_header: > .ascii "PE" > @@ -160,7 +162,7 @@ optional_header: > .long 0 // SizeOfInitializedData > .long 0 // SizeOfUninitializedData > .long efi_stub_entry - efi_head // AddressOfEntryPoint > - .long stext - efi_head // BaseOfCode > + .long stext_offset // BaseOfCode > > extra_header_fields: > .quad 0 // ImageBase > @@ -177,7 +179,7 @@ extra_header_fields: > .long _edata - efi_head // SizeOfImage > > // Everything before the kernel image is considered part of the header > - .long stext - efi_head // SizeOfHeaders > + .long stext_offset // SizeOfHeaders > .long 0 // CheckSum > .short 0xa // Subsystem (EFI application) > .short 0 // DllCharacteristics > @@ -222,9 +224,9 @@ section_table: > .byte 0 > .byte 0 // end of 0 padding of section name > .long _edata - stext // VirtualSize > - .long stext - efi_head // VirtualAddress > + .long stext_offset // VirtualAddress > .long _edata - stext // SizeOfRawData > - .long stext - efi_head // PointerToRawData > + .long stext_offset // PointerToRawData > > .long 0 // PointerToRelocations (0 for executables) > .long 0 // PointerToLineNumbers (0 for executables) > -- > 1.8.3.2 > >
On 15 July 2014 11:57, Mark Rutland <mark.rutland@arm.com> wrote: > Hi Ard, > > On Tue, Jul 15, 2014 at 10:10:02AM +0100, Ard Biesheuvel wrote: >> After the EFI stub has done its business, it jumps into the kernel by branching >> to offset #0 of the loaded Image, which is where it expects to find the header >> containing a 'branch to stext' instruction. >> However, the header is not covered by any PE/COFF section, so the header may >> not actually be loaded at the expected offset. So instead, jump to 'stext' >> directly, which is at the base of the PE/COFF .text section. > > It would be nice to point out in the commit message that the other > changes in the patch are just cleanup to use stext_offset rather than > open-coding it. > OK >> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> >> --- >> arch/arm64/kernel/efi-entry.S | 2 +- >> arch/arm64/kernel/head.S | 10 ++++++---- >> 2 files changed, 7 insertions(+), 5 deletions(-) >> >> diff --git a/arch/arm64/kernel/efi-entry.S b/arch/arm64/kernel/efi-entry.S >> index 619b1dd7bcde..6ef541731d9e 100644 >> --- a/arch/arm64/kernel/efi-entry.S >> +++ b/arch/arm64/kernel/efi-entry.S >> @@ -61,7 +61,7 @@ ENTRY(efi_stub_entry) >> */ >> mov x20, x0 // DTB address >> ldr x0, [sp, #16] // relocated _text address >> - mov x21, x0 >> + add x21, x0, #:lo12:stext_offset > > I think we can drop the :lo12: here, which will allow us to have a > warning if stext_offset is unexpectedly large (I believe this will > currently silently mask bits were that to happen?). > There is no alternative lo12 relocation that errors out when the value does not fit, so it would have to use a literal instead. > Other than that, this looks like a sensible thing to do given that we > cannot rely on the header being present. > Cheers, Ard. >> >> /* >> * Flush dcache covering current runtime addresses >> diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S >> index a2c1195abb7f..78ddae28b090 100644 >> --- a/arch/arm64/kernel/head.S >> +++ b/arch/arm64/kernel/head.S >> @@ -137,6 +137,8 @@ efi_head: >> #endif >> >> #ifdef CONFIG_EFI >> + .globl stext_offset >> + .set stext_offset, stext - efi_head >> .align 3 >> pe_header: >> .ascii "PE" >> @@ -160,7 +162,7 @@ optional_header: >> .long 0 // SizeOfInitializedData >> .long 0 // SizeOfUninitializedData >> .long efi_stub_entry - efi_head // AddressOfEntryPoint >> - .long stext - efi_head // BaseOfCode >> + .long stext_offset // BaseOfCode >> >> extra_header_fields: >> .quad 0 // ImageBase >> @@ -177,7 +179,7 @@ extra_header_fields: >> .long _edata - efi_head // SizeOfImage >> >> // Everything before the kernel image is considered part of the header >> - .long stext - efi_head // SizeOfHeaders >> + .long stext_offset // SizeOfHeaders >> .long 0 // CheckSum >> .short 0xa // Subsystem (EFI application) >> .short 0 // DllCharacteristics >> @@ -222,9 +224,9 @@ section_table: >> .byte 0 >> .byte 0 // end of 0 padding of section name >> .long _edata - stext // VirtualSize >> - .long stext - efi_head // VirtualAddress >> + .long stext_offset // VirtualAddress >> .long _edata - stext // SizeOfRawData >> - .long stext - efi_head // PointerToRawData >> + .long stext_offset // PointerToRawData >> >> .long 0 // PointerToRelocations (0 for executables) >> .long 0 // PointerToLineNumbers (0 for executables) >> -- >> 1.8.3.2 >> >>
> >> diff --git a/arch/arm64/kernel/efi-entry.S b/arch/arm64/kernel/efi-entry.S > >> index 619b1dd7bcde..6ef541731d9e 100644 > >> --- a/arch/arm64/kernel/efi-entry.S > >> +++ b/arch/arm64/kernel/efi-entry.S > >> @@ -61,7 +61,7 @@ ENTRY(efi_stub_entry) > >> */ > >> mov x20, x0 // DTB address > >> ldr x0, [sp, #16] // relocated _text address > >> - mov x21, x0 > >> + add x21, x0, #:lo12:stext_offset > > > > I think we can drop the :lo12: here, which will allow us to have a > > warning if stext_offset is unexpectedly large (I believe this will > > currently silently mask bits were that to happen?). > > > > There is no alternative lo12 relocation that errors out when the value > does not fit, so it would have to use a literal instead. Ah, that's a shame. What happens when the value doesn't fit if the linker / assembler don't error out? That sounds like a toolchain bug if they're silently doing the wrong thing. Cheers, Mark.
On 15 July 2014 13:31, Mark Rutland <mark.rutland@arm.com> wrote: >> >> diff --git a/arch/arm64/kernel/efi-entry.S b/arch/arm64/kernel/efi-entry.S >> >> index 619b1dd7bcde..6ef541731d9e 100644 >> >> --- a/arch/arm64/kernel/efi-entry.S >> >> +++ b/arch/arm64/kernel/efi-entry.S >> >> @@ -61,7 +61,7 @@ ENTRY(efi_stub_entry) >> >> */ >> >> mov x20, x0 // DTB address >> >> ldr x0, [sp, #16] // relocated _text address >> >> - mov x21, x0 >> >> + add x21, x0, #:lo12:stext_offset >> > >> > I think we can drop the :lo12: here, which will allow us to have a >> > warning if stext_offset is unexpectedly large (I believe this will >> > currently silently mask bits were that to happen?). >> > >> >> There is no alternative lo12 relocation that errors out when the value >> does not fit, so it would have to use a literal instead. > > Ah, that's a shame. What happens when the value doesn't fit if the > linker / assembler don't error out? > Obviously, it will jump into the wrong place if stext ever gets pushed beyond a 4k boundary, which is not that likely (current value is 0x160, we may want to up it to 0x200 at some point if we need to increase the PE/COFF section alignment, which some versions of the (poor) PE/COFF documentation say should be 0x200 at the minimum) However, doing ldr x21, =stext_offset works fine as well > That sounds like a toolchain bug if they're silently doing the wrong > thing. > Meh, don't think so, really. You get what you pay for, and :lo12: just isn't supposed to care. (It's mainly used with adrp to get a +/- 4gb PC relative reach)
On Tue, Jul 15, 2014 at 12:49:07PM +0100, Ard Biesheuvel wrote: > On 15 July 2014 13:31, Mark Rutland <mark.rutland@arm.com> wrote: > >> >> diff --git a/arch/arm64/kernel/efi-entry.S b/arch/arm64/kernel/efi-entry.S > >> >> index 619b1dd7bcde..6ef541731d9e 100644 > >> >> --- a/arch/arm64/kernel/efi-entry.S > >> >> +++ b/arch/arm64/kernel/efi-entry.S > >> >> @@ -61,7 +61,7 @@ ENTRY(efi_stub_entry) > >> >> */ > >> >> mov x20, x0 // DTB address > >> >> ldr x0, [sp, #16] // relocated _text address > >> >> - mov x21, x0 > >> >> + add x21, x0, #:lo12:stext_offset > >> > > >> > I think we can drop the :lo12: here, which will allow us to have a > >> > warning if stext_offset is unexpectedly large (I believe this will > >> > currently silently mask bits were that to happen?). > >> > > >> > >> There is no alternative lo12 relocation that errors out when the value > >> does not fit, so it would have to use a literal instead. > > > > Ah, that's a shame. What happens when the value doesn't fit if the > > linker / assembler don't error out? > > > > Obviously, it will jump into the wrong place if stext ever gets pushed > beyond a 4k boundary, which is not that likely (current value is > 0x160, we may want to up it to 0x200 at some point if we need to > increase the PE/COFF section alignment, which some versions of the > (poor) PE/COFF documentation say should be 0x200 at the minimum) > > However, doing ldr x21, =stext_offset works fine as well > > > That sounds like a toolchain bug if they're silently doing the wrong > > thing. > > > > Meh, don't think so, really. You get what you pay for, and :lo12: just > isn't supposed to care. (It's mainly used with adrp to get a +/- 4gb > PC relative reach) We appear to have a misunderstanding. I was suggesting we use: add x21, x0, #stext_offset ... and I thought you meant that wouldn't produce an error if the immediate was out of range. I understand that :lo12: cuts the top bits off, and for its intended use that makes sense. From testing I see see that gas isn't happy to accept an undefined symbol as an immediate without :lo12:, and I can see why that makes sense. My suggestion was bad, sorry. Thanks, Mark.
diff --git a/arch/arm64/kernel/efi-entry.S b/arch/arm64/kernel/efi-entry.S index 619b1dd7bcde..6ef541731d9e 100644 --- a/arch/arm64/kernel/efi-entry.S +++ b/arch/arm64/kernel/efi-entry.S @@ -61,7 +61,7 @@ ENTRY(efi_stub_entry) */ mov x20, x0 // DTB address ldr x0, [sp, #16] // relocated _text address - mov x21, x0 + add x21, x0, #:lo12:stext_offset /* * Flush dcache covering current runtime addresses diff --git a/arch/arm64/kernel/head.S b/arch/arm64/kernel/head.S index a2c1195abb7f..78ddae28b090 100644 --- a/arch/arm64/kernel/head.S +++ b/arch/arm64/kernel/head.S @@ -137,6 +137,8 @@ efi_head: #endif #ifdef CONFIG_EFI + .globl stext_offset + .set stext_offset, stext - efi_head .align 3 pe_header: .ascii "PE" @@ -160,7 +162,7 @@ optional_header: .long 0 // SizeOfInitializedData .long 0 // SizeOfUninitializedData .long efi_stub_entry - efi_head // AddressOfEntryPoint - .long stext - efi_head // BaseOfCode + .long stext_offset // BaseOfCode extra_header_fields: .quad 0 // ImageBase @@ -177,7 +179,7 @@ extra_header_fields: .long _edata - efi_head // SizeOfImage // Everything before the kernel image is considered part of the header - .long stext - efi_head // SizeOfHeaders + .long stext_offset // SizeOfHeaders .long 0 // CheckSum .short 0xa // Subsystem (EFI application) .short 0 // DllCharacteristics @@ -222,9 +224,9 @@ section_table: .byte 0 .byte 0 // end of 0 padding of section name .long _edata - stext // VirtualSize - .long stext - efi_head // VirtualAddress + .long stext_offset // VirtualAddress .long _edata - stext // SizeOfRawData - .long stext - efi_head // PointerToRawData + .long stext_offset // PointerToRawData .long 0 // PointerToRelocations (0 for executables) .long 0 // PointerToLineNumbers (0 for executables)
After the EFI stub has done its business, it jumps into the kernel by branching to offset #0 of the loaded Image, which is where it expects to find the header containing a 'branch to stext' instruction. However, the header is not covered by any PE/COFF section, so the header may not actually be loaded at the expected offset. So instead, jump to 'stext' directly, which is at the base of the PE/COFF .text section. Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- arch/arm64/kernel/efi-entry.S | 2 +- arch/arm64/kernel/head.S | 10 ++++++---- 2 files changed, 7 insertions(+), 5 deletions(-)