On Thu, Feb 20, 2020 at 10:22:23PM +0100, Ard Biesheuvel wrote: > On Thu, 20 Feb 2020 at 20:52, Arvind Sankar <nivedita@xxxxxxxxxxxx> wrote: > > > > On Thu, Feb 20, 2020 at 06:32:39PM +0100, Ard Biesheuvel wrote: > > > On Thu, 20 Feb 2020 at 18:28, Arvind Sankar <nivedita@xxxxxxxxxxxx> wrote: > > > > > > > > The .text section contains data as well. Shouldn't it be marked > > > > IMAGE_SCN_MEM_WRITE also? > > > > > > > > > > Another good point. For ARM and arm64, I actually changed this into > > > split R-X and RW- sections, in case the loader decides to honour these > > > attributes (which it should imo) > > > > > > So yes, we should either add IMAGE_SCN_MEM_WRITE, or add a .data > > > section (although we probably don't have the space for that). > > > > > > Another thing I wondered was whether we really need the .reloc > > > section. We don't have one on ARM, and it works fine with the existing > > > EDK2 loader. > > > Peter: any idea whether the issue with .reloc you pointed out to me > > > the other day still exists in EDK2 today? > > > > Is there any reason we couldn't combine .setup + .text as R-X, then > > .reloc, then .data + .bss as RW-? That would leave room for the new > > .compat section then even without removing .reloc. > > > > Although if the loader actually honored the non-writable setting, > > efi_relocate_kernel has to be done unconditionally. It currently happens > > to be unconditional if you come in via PE loader, since we don't set > > ImageBase to tell it our preferred address, and even if we did, the > > existence of .setup means startup32 isn't at that address. > > Shouldn't matter, given that we don't use the same page tables to run > the startup code. Ooh right. And for the 32-bit case we turn off paging altogether.
