| Message ID | 20201025134941.4805-6-ard.biesheuvel@arm.com |
|---|---|
| State | New |
| Headers | show |
| Series | linux: implement LoadFile2 initrd loading | expand |
On 10/25/20 2:49 PM, Ard Biesheuvel wrote: > The way we load the Linux and PE/COFF image headers depends on a fixed > placement of the COFF header at offset 0x40 into the file. This is a > reasonable default, given that this is where Linux emits it today. > However, in order to comply with the PE/COFF spec, which permits this > header to appear anywhere in the file, let's ensure that we read the > header from where it actually appears in the file if it is not located > at offset 0x40. > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com> > --- > grub-core/loader/arm64/linux.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c > index 915b6ad7292d..28ff8584a3b5 100644 > --- a/grub-core/loader/arm64/linux.c > +++ b/grub-core/loader/arm64/linux.c > @@ -66,6 +66,21 @@ grub_arch_efi_linux_load_image_header (grub_file_t file, > grub_dprintf ("linux", "UEFI stub kernel:\n"); > grub_dprintf ("linux", "PE/COFF header @ %08x\n", lh->hdr_offset); > > + /* > + * The PE/COFF spec permits the COFF header to appear anywhere in the file, so > + * we need to double check whether it was where we expected it, and if not, we > + * must load it from the correct offset into the coff_image_header field of > + * struct linux_arch_kernel_header. > + */ > + if ((grub_uint8_t *) lh + lh->hdr_offset != (grub_uint8_t *) &lh->coff_image_header) > + { > + grub_file_seek (file, lh->hdr_offset); Isn't this overly complicated? Why don't we first read the whole file into memory and then analyze it instead of using multiple accesses which only slows down the process? Best regards Heinrich > + > + if (grub_file_read (file, &lh->coff_image_header, sizeof(struct grub_coff_image_header)) > + != sizeof(struct grub_coff_image_header)) > + return grub_error(GRUB_ERR_FILE_READ_ERROR, "failed to read COFF image header"); > + } > + > return GRUB_ERR_NONE; > } > >
On Thu, 8 Apr 2021 at 20:57, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 10/25/20 2:49 PM, Ard Biesheuvel wrote: > > The way we load the Linux and PE/COFF image headers depends on a fixed > > placement of the COFF header at offset 0x40 into the file. This is a > > reasonable default, given that this is where Linux emits it today. > > However, in order to comply with the PE/COFF spec, which permits this > > header to appear anywhere in the file, let's ensure that we read the > > header from where it actually appears in the file if it is not located > > at offset 0x40. > > > > Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com> > > --- > > grub-core/loader/arm64/linux.c | 15 +++++++++++++++ > > 1 file changed, 15 insertions(+) > > > > diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c > > index 915b6ad7292d..28ff8584a3b5 100644 > > --- a/grub-core/loader/arm64/linux.c > > +++ b/grub-core/loader/arm64/linux.c > > @@ -66,6 +66,21 @@ grub_arch_efi_linux_load_image_header (grub_file_t file, > > grub_dprintf ("linux", "UEFI stub kernel:\n"); > > grub_dprintf ("linux", "PE/COFF header @ %08x\n", lh->hdr_offset); > > > > + /* > > + * The PE/COFF spec permits the COFF header to appear anywhere in the file, so > > + * we need to double check whether it was where we expected it, and if not, we > > + * must load it from the correct offset into the coff_image_header field of > > + * struct linux_arch_kernel_header. > > + */ > > + if ((grub_uint8_t *) lh + lh->hdr_offset != (grub_uint8_t *) &lh->coff_image_header) > > + { > > + grub_file_seek (file, lh->hdr_offset); > > Isn't this overly complicated? Why don't we first read the whole file > into memory and then analyze it instead of using multiple accesses which > only slows down the process? > Given that the condition will never hold in practice, as the offset is always going to be 0x40, this change is not expected to affect performance at all. Doing a complete overhaul of the PE image loading logic for this seems unwise to me.
On 4/9/21 8:12 AM, Ard Biesheuvel wrote: > On Thu, 8 Apr 2021 at 20:57, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: >> >> On 10/25/20 2:49 PM, Ard Biesheuvel wrote: >>> The way we load the Linux and PE/COFF image headers depends on a fixed >>> placement of the COFF header at offset 0x40 into the file. This is a >>> reasonable default, given that this is where Linux emits it today. >>> However, in order to comply with the PE/COFF spec, which permits this >>> header to appear anywhere in the file, let's ensure that we read the >>> header from where it actually appears in the file if it is not located >>> at offset 0x40. >>> >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com> >>> --- >>> grub-core/loader/arm64/linux.c | 15 +++++++++++++++ >>> 1 file changed, 15 insertions(+) >>> >>> diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c >>> index 915b6ad7292d..28ff8584a3b5 100644 >>> --- a/grub-core/loader/arm64/linux.c >>> +++ b/grub-core/loader/arm64/linux.c >>> @@ -66,6 +66,21 @@ grub_arch_efi_linux_load_image_header (grub_file_t file, >>> grub_dprintf ("linux", "UEFI stub kernel:\n"); >>> grub_dprintf ("linux", "PE/COFF header @ %08x\n", lh->hdr_offset); >>> >>> + /* >>> + * The PE/COFF spec permits the COFF header to appear anywhere in the file, so >>> + * we need to double check whether it was where we expected it, and if not, we >>> + * must load it from the correct offset into the coff_image_header field of >>> + * struct linux_arch_kernel_header. >>> + */ >>> + if ((grub_uint8_t *) lh + lh->hdr_offset != (grub_uint8_t *) &lh->coff_image_header) >>> + { >>> + grub_file_seek (file, lh->hdr_offset); >> >> Isn't this overly complicated? Why don't we first read the whole file >> into memory and then analyze it instead of using multiple accesses which >> only slows down the process? >> > > Given that the condition will never hold in practice, as the offset is > always going to be 0x40, this change is not expected to affect > performance at all. The PE COFF specification let's you specify any value. The linux command can be used to launch arbitrary EFI binaries if they have the Linux magic 'ARM\x64' in the right place. What I never understood is why the linux command is checking this Linux magic field at all instead of running any EFI binary thrown at it. Best regards Heinrich > > Doing a complete overhaul of the PE image loading logic for this seems > unwise to me. >
On Fri, 9 Apr 2021 at 08:40, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > > On 4/9/21 8:12 AM, Ard Biesheuvel wrote: > > On Thu, 8 Apr 2021 at 20:57, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > >> > >> On 10/25/20 2:49 PM, Ard Biesheuvel wrote: > >>> The way we load the Linux and PE/COFF image headers depends on a fixed > >>> placement of the COFF header at offset 0x40 into the file. This is a > >>> reasonable default, given that this is where Linux emits it today. > >>> However, in order to comply with the PE/COFF spec, which permits this > >>> header to appear anywhere in the file, let's ensure that we read the > >>> header from where it actually appears in the file if it is not located > >>> at offset 0x40. > >>> > >>> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com> > >>> --- > >>> grub-core/loader/arm64/linux.c | 15 +++++++++++++++ > >>> 1 file changed, 15 insertions(+) > >>> > >>> diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c > >>> index 915b6ad7292d..28ff8584a3b5 100644 > >>> --- a/grub-core/loader/arm64/linux.c > >>> +++ b/grub-core/loader/arm64/linux.c > >>> @@ -66,6 +66,21 @@ grub_arch_efi_linux_load_image_header (grub_file_t file, > >>> grub_dprintf ("linux", "UEFI stub kernel:\n"); > >>> grub_dprintf ("linux", "PE/COFF header @ %08x\n", lh->hdr_offset); > >>> > >>> + /* > >>> + * The PE/COFF spec permits the COFF header to appear anywhere in the file, so > >>> + * we need to double check whether it was where we expected it, and if not, we > >>> + * must load it from the correct offset into the coff_image_header field of > >>> + * struct linux_arch_kernel_header. > >>> + */ > >>> + if ((grub_uint8_t *) lh + lh->hdr_offset != (grub_uint8_t *) &lh->coff_image_header) > >>> + { > >>> + grub_file_seek (file, lh->hdr_offset); > >> > >> Isn't this overly complicated? Why don't we first read the whole file > >> into memory and then analyze it instead of using multiple accesses which > >> only slows down the process? > >> > > > > Given that the condition will never hold in practice, as the offset is > > always going to be 0x40, this change is not expected to affect > > performance at all. > > The PE COFF specification let's you specify any value. The linux command > can be used to launch arbitrary EFI binaries if they have the Linux > magic 'ARM\x64' in the right place. > > What I never understood is why the linux command is checking this Linux > magic field at all instead of running any EFI binary thrown at it. > I don't disagree with you on that. The question is whether it should be in scope for this change to fix all of that.
diff --git a/grub-core/loader/arm64/linux.c b/grub-core/loader/arm64/linux.c index 915b6ad7292d..28ff8584a3b5 100644 --- a/grub-core/loader/arm64/linux.c +++ b/grub-core/loader/arm64/linux.c @@ -66,6 +66,21 @@ grub_arch_efi_linux_load_image_header (grub_file_t file, grub_dprintf ("linux", "UEFI stub kernel:\n"); grub_dprintf ("linux", "PE/COFF header @ %08x\n", lh->hdr_offset); + /* + * The PE/COFF spec permits the COFF header to appear anywhere in the file, so + * we need to double check whether it was where we expected it, and if not, we + * must load it from the correct offset into the coff_image_header field of + * struct linux_arch_kernel_header. + */ + if ((grub_uint8_t *) lh + lh->hdr_offset != (grub_uint8_t *) &lh->coff_image_header) + { + grub_file_seek (file, lh->hdr_offset); + + if (grub_file_read (file, &lh->coff_image_header, sizeof(struct grub_coff_image_header)) + != sizeof(struct grub_coff_image_header)) + return grub_error(GRUB_ERR_FILE_READ_ERROR, "failed to read COFF image header"); + } + return GRUB_ERR_NONE; }
The way we load the Linux and PE/COFF image headers depends on a fixed placement of the COFF header at offset 0x40 into the file. This is a reasonable default, given that this is where Linux emits it today. However, in order to comply with the PE/COFF spec, which permits this header to appear anywhere in the file, let's ensure that we read the header from where it actually appears in the file if it is not located at offset 0x40. Signed-off-by: Ard Biesheuvel <ard.biesheuvel@arm.com> --- grub-core/loader/arm64/linux.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)