| Message ID | 20250318173547.59475-6-roger.pau@citrix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86: generate xen.efi image with no write-execute sections | expand |
On 18/03/2025 5:35 pm, Roger Pau Monne wrote: > Relocations are now applied after having moved the trampoline, so there's no > reason to warn about relocations to read-only sections. The logic that > apply the relocations would make sure they are applied against writable > mappings. > > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> > --- > xen/arch/x86/efi/mkreloc.c | 5 ----- > 1 file changed, 5 deletions(-) > > diff --git a/xen/arch/x86/efi/mkreloc.c b/xen/arch/x86/efi/mkreloc.c > index 375cb79d6959..a5a1969f2ee5 100644 > --- a/xen/arch/x86/efi/mkreloc.c > +++ b/xen/arch/x86/efi/mkreloc.c > @@ -216,11 +216,6 @@ static void diff_sections(const unsigned char *ptr1, const unsigned char *ptr2, > exit(3); > } > > - if ( !(sec->flags & IMAGE_SCN_MEM_WRITE) ) > - fprintf(stderr, > - "Warning: relocation to r/o section %.8s:%08" PRIxFAST32 "\n", > - sec->name, i - disp); > - > printf("\t.word (%u << 12) | 0x%03" PRIxFAST32 "\n", > reloc, sec->rva + i - disp - rva); > reloc_size += 2; I'm on the fence about this. Obviously we don't want a warning firing for good cases, but the trampoline is special where the relocation is to a R/O section but we don't apply the relocation in that position. Is it possible to limit this to only trampoline_{start,end} and keep the warning in general? ~Andrew
On 18.03.2025 18:35, Roger Pau Monne wrote:
> Relocations are now applied after having moved the trampoline,
That's two entirely different sets of relocations, isn't it? What we generate
here is what is to be encoded in the PE binary's .reloc section, for the PE
loader to process. And for us to then process again once we move Xen back to
its linked position (by virtue of leaving physical mode). Therefore what
matters here is whether these relocations are still carried out while on the
page tables to boot loader created, or when already on page tables we control.
In the former case any relocation to a non-writable section would be liable
to fault when applied.
Jan
On 19.03.2025 11:32, Jan Beulich wrote: > On 18.03.2025 18:35, Roger Pau Monne wrote: >> Relocations are now applied after having moved the trampoline, > > That's two entirely different sets of relocations, isn't it? What we generate > here is what is to be encoded in the PE binary's .reloc section, for the PE > loader to process. And for us to then process again once we move Xen back to > its linked position (by virtue of leaving physical mode). Therefore what > matters here is whether these relocations are still carried out while on the > page tables to boot loader created, or when already on page tables we control. > In the former case any relocation to a non-writable section would be liable > to fault when applied. And yes - both calls to efi_arch_relocate_image() are ahead of switching page tables. The first call is benign - no writes occur there. The second call would cause #PF though for any relocs applied to .text or .rodata or .init.text or whatever else is non-writable. Jan
On 19.03.2025 11:46, Jan Beulich wrote: > On 19.03.2025 11:32, Jan Beulich wrote: >> On 18.03.2025 18:35, Roger Pau Monne wrote: >>> Relocations are now applied after having moved the trampoline, >> >> That's two entirely different sets of relocations, isn't it? What we generate >> here is what is to be encoded in the PE binary's .reloc section, for the PE >> loader to process. And for us to then process again once we move Xen back to >> its linked position (by virtue of leaving physical mode). Therefore what >> matters here is whether these relocations are still carried out while on the >> page tables to boot loader created, or when already on page tables we control. >> In the former case any relocation to a non-writable section would be liable >> to fault when applied. > > And yes - both calls to efi_arch_relocate_image() are ahead of switching page > tables. The first call is benign - no writes occur there. The second call > would cause #PF though for any relocs applied to .text or .rodata or .init.text > or whatever else is non-writable. Ah, no - .rodata is unaffected, due to it being writable as a result of also containing all .data.ro_after_init contributions. Jan
On Wed, Mar 19, 2025 at 11:46:22AM +0100, Jan Beulich wrote: > On 19.03.2025 11:32, Jan Beulich wrote: > > On 18.03.2025 18:35, Roger Pau Monne wrote: > >> Relocations are now applied after having moved the trampoline, > > > > That's two entirely different sets of relocations, isn't it? Right, this is the plain .reloc, while the trampoline one is .trampoline_{rel,seg} > > What we generate > > here is what is to be encoded in the PE binary's .reloc section, for the PE > > loader to process. And for us to then process again once we move Xen back to > > its linked position (by virtue of leaving physical mode). Therefore what > > matters here is whether these relocations are still carried out while on the > > page tables to boot loader created, or when already on page tables we control. > > In the former case any relocation to a non-writable section would be liable > > to fault when applied. > > And yes - both calls to efi_arch_relocate_image() are ahead of switching page > tables. The first call is benign - no writes occur there. The second call > would cause #PF though for any relocs applied to .text or .rodata or .init.text > or whatever else is non-writable. I wonder how this worked then, as I've tested with the xen.efi smoke test in gitlab CI. Maybe ovmf doesn't acknowledge the RX sections and unconditionally sets all mappings as writable? Thanks, Roger.
On 20.03.2025 09:14, Roger Pau Monné wrote: > On Wed, Mar 19, 2025 at 11:46:22AM +0100, Jan Beulich wrote: >> On 19.03.2025 11:32, Jan Beulich wrote: >>> On 18.03.2025 18:35, Roger Pau Monne wrote: >>>> Relocations are now applied after having moved the trampoline, >>> >>> That's two entirely different sets of relocations, isn't it? > > Right, this is the plain .reloc, while the trampoline one is > .trampoline_{rel,seg} > >>> What we generate >>> here is what is to be encoded in the PE binary's .reloc section, for the PE >>> loader to process. And for us to then process again once we move Xen back to >>> its linked position (by virtue of leaving physical mode). Therefore what >>> matters here is whether these relocations are still carried out while on the >>> page tables to boot loader created, or when already on page tables we control. >>> In the former case any relocation to a non-writable section would be liable >>> to fault when applied. >> >> And yes - both calls to efi_arch_relocate_image() are ahead of switching page >> tables. The first call is benign - no writes occur there. The second call >> would cause #PF though for any relocs applied to .text or .rodata or .init.text >> or whatever else is non-writable. > > I wonder how this worked then, as I've tested with the xen.efi smoke > test in gitlab CI. Maybe ovmf doesn't acknowledge the RX sections and > unconditionally sets all mappings as writable? Possible. And that would be in line with the mode being call "physical mode": There are no permissions to enforce there. It just so happens that x86-64 requires paging to be enabled to be able to run 64-bit code. My experience with OVMF has been that it's hard to find where certain code lives. Perhaps I should try whether I can find respective code there. Then again if I find nothing, there wouldn't be any guarantee that I merely didn't spot the right place. Jan
On 20.03.2025 09:34, Jan Beulich wrote: > On 20.03.2025 09:14, Roger Pau Monné wrote: >> I wonder how this worked then, as I've tested with the xen.efi smoke >> test in gitlab CI. Maybe ovmf doesn't acknowledge the RX sections and >> unconditionally sets all mappings as writable? > > Possible. And that would be in line with the mode being call "physical mode": > There are no permissions to enforce there. It just so happens that x86-64 > requires paging to be enabled to be able to run 64-bit code. > > My experience with OVMF has been that it's hard to find where certain code > lives. Perhaps I should try whether I can find respective code there. Then > again if I find nothing, there wouldn't be any guarantee that I merely didn't > spot the right place. All I can find is BaseTools/Source/C/Common/BasePeCoff.c:PeCoffLoaderLoadImage(), which doesn't look to care about section flags at all. (By implication this would mean they needlessly load all the .debug_* sections as well. Then again we need to be glad they ignore the discard flag, or else .reloc wouldn't be loaded either.) Jan
On 20.03.2025 10:53, Jan Beulich wrote: > On 20.03.2025 09:34, Jan Beulich wrote: >> On 20.03.2025 09:14, Roger Pau Monné wrote: >>> I wonder how this worked then, as I've tested with the xen.efi smoke >>> test in gitlab CI. Maybe ovmf doesn't acknowledge the RX sections and >>> unconditionally sets all mappings as writable? >> >> Possible. And that would be in line with the mode being call "physical mode": >> There are no permissions to enforce there. It just so happens that x86-64 >> requires paging to be enabled to be able to run 64-bit code. >> >> My experience with OVMF has been that it's hard to find where certain code >> lives. Perhaps I should try whether I can find respective code there. Then >> again if I find nothing, there wouldn't be any guarantee that I merely didn't >> spot the right place. > > All I can find is BaseTools/Source/C/Common/BasePeCoff.c:PeCoffLoaderLoadImage(), > which doesn't look to care about section flags at all. And then there is at least one duplicate thereof elsewhere, or something very close to a duplicate. In addition I meanwhile found ProtectUefiImage(), yet it's unclear (to me) under what conditions execution would make it there. Plus it leading to SetUefiImageMemoryAttributes() leaves open where gCpu->SetMemoryAttributes() is implemented. I can spot some Arm and RISC-V code with respective names (albeit in distinct places), and MTRR functionality with names along these lines. None of these can be it. Jan
On 20/03/2025 10:18 am, Jan Beulich wrote: > On 20.03.2025 10:53, Jan Beulich wrote: >> On 20.03.2025 09:34, Jan Beulich wrote: >>> On 20.03.2025 09:14, Roger Pau Monné wrote: >>>> I wonder how this worked then, as I've tested with the xen.efi smoke >>>> test in gitlab CI. Maybe ovmf doesn't acknowledge the RX sections and >>>> unconditionally sets all mappings as writable? >>> Possible. And that would be in line with the mode being call "physical mode": >>> There are no permissions to enforce there. It just so happens that x86-64 >>> requires paging to be enabled to be able to run 64-bit code. >>> >>> My experience with OVMF has been that it's hard to find where certain code >>> lives. Perhaps I should try whether I can find respective code there. Then >>> again if I find nothing, there wouldn't be any guarantee that I merely didn't >>> spot the right place. >> All I can find is BaseTools/Source/C/Common/BasePeCoff.c:PeCoffLoaderLoadImage(), >> which doesn't look to care about section flags at all. > And then there is at least one duplicate thereof elsewhere, or something very > close to a duplicate. In addition I meanwhile found ProtectUefiImage(), yet > it's unclear (to me) under what conditions execution would make it there. Plus > it leading to SetUefiImageMemoryAttributes() leaves open where > gCpu->SetMemoryAttributes() is implemented. I can spot some Arm and RISC-V code > with respective names (albeit in distinct places), and MTRR functionality with > names along these lines. None of these can be it. https://www.kraxel.org/blog/2023/12/uefi-nx-linux-boot/ ~Andrew
On 20.03.2025 12:00, Andrew Cooper wrote: > On 20/03/2025 10:18 am, Jan Beulich wrote: >> On 20.03.2025 10:53, Jan Beulich wrote: >>> On 20.03.2025 09:34, Jan Beulich wrote: >>>> On 20.03.2025 09:14, Roger Pau Monné wrote: >>>>> I wonder how this worked then, as I've tested with the xen.efi smoke >>>>> test in gitlab CI. Maybe ovmf doesn't acknowledge the RX sections and >>>>> unconditionally sets all mappings as writable? >>>> Possible. And that would be in line with the mode being call "physical mode": >>>> There are no permissions to enforce there. It just so happens that x86-64 >>>> requires paging to be enabled to be able to run 64-bit code. >>>> >>>> My experience with OVMF has been that it's hard to find where certain code >>>> lives. Perhaps I should try whether I can find respective code there. Then >>>> again if I find nothing, there wouldn't be any guarantee that I merely didn't >>>> spot the right place. >>> All I can find is BaseTools/Source/C/Common/BasePeCoff.c:PeCoffLoaderLoadImage(), >>> which doesn't look to care about section flags at all. >> And then there is at least one duplicate thereof elsewhere, or something very >> close to a duplicate. In addition I meanwhile found ProtectUefiImage(), yet >> it's unclear (to me) under what conditions execution would make it there. Plus >> it leading to SetUefiImageMemoryAttributes() leaves open where >> gCpu->SetMemoryAttributes() is implemented. I can spot some Arm and RISC-V code >> with respective names (albeit in distinct places), and MTRR functionality with >> names along these lines. None of these can be it. > > https://www.kraxel.org/blog/2023/12/uefi-nx-linux-boot/ Which, besides saying e.g. "When configured to do so ...", means that our copy of ovmf wouldn't have any of that, as it hasn't been updated for quite some time afaict (just tried to pull earlier in the day, with nothing new coming through at all; the last time something new came through was apparently about a year and a half ago, i.e. older than what that article says is needed). Jan
diff --git a/xen/arch/x86/efi/mkreloc.c b/xen/arch/x86/efi/mkreloc.c index 375cb79d6959..a5a1969f2ee5 100644 --- a/xen/arch/x86/efi/mkreloc.c +++ b/xen/arch/x86/efi/mkreloc.c @@ -216,11 +216,6 @@ static void diff_sections(const unsigned char *ptr1, const unsigned char *ptr2, exit(3); } - if ( !(sec->flags & IMAGE_SCN_MEM_WRITE) ) - fprintf(stderr, - "Warning: relocation to r/o section %.8s:%08" PRIxFAST32 "\n", - sec->name, i - disp); - printf("\t.word (%u << 12) | 0x%03" PRIxFAST32 "\n", reloc, sec->rva + i - disp - rva); reloc_size += 2;
Relocations are now applied after having moved the trampoline, so there's no reason to warn about relocations to read-only sections. The logic that apply the relocations would make sure they are applied against writable mappings. Signed-off-by: Roger Pau Monné <roger.pau@citrix.com> --- xen/arch/x86/efi/mkreloc.c | 5 ----- 1 file changed, 5 deletions(-)