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.
