* [PATCH] xen: look for Xen notes in section headers too
@ 2019-03-14 1:57 Marek Marczykowski-Górecki
2019-03-14 12:39 ` Daniel Kiper
0 siblings, 1 reply; 4+ messages in thread
From: Marek Marczykowski-Górecki @ 2019-03-14 1:57 UTC (permalink / raw)
To: grub-devel; +Cc: Marek Marczykowski-Górecki
Mirror behaviour of elf loader in libxc: first look for Xen notes in
PT_NOTE segment, then in SHT_NOTE section and only then fallback to
a section with __xen_guest name.
This fixes loading PV kernels that Xen note have outside of PT_NOTE.
While this may be result of a buggy linker script, loading such kernel
directly works fine, so make it work with grub too.
Specifically, this applies to binaries built from Unikraft.
Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
---
grub-core/loader/i386/xen_fileXX.c | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/grub-core/loader/i386/xen_fileXX.c b/grub-core/loader/i386/xen_fileXX.c
index fb66e66fe..6af6cc0da 100644
--- a/grub-core/loader/i386/xen_fileXX.c
+++ b/grub-core/loader/i386/xen_fileXX.c
@@ -344,6 +344,21 @@ grub_xen_get_infoXX (grub_elf_t elf, struct grub_xen_file_info *xi)
s = (Elf_Shdr *) ((char *) s0 + elf->ehdr.ehdrXX.e_shstrndx * shentsize);
stroff = s->sh_offset;
+ for (s = s0; s < (Elf_Shdr *) ((char *) s0 + shnum * shentsize);
+ s = (Elf_Shdr *) ((char *) s + shentsize))
+ {
+ if (s->sh_type == SHT_NOTE) {
+ err = parse_note (elf, xi, s->sh_offset, s->sh_size);
+ if (err)
+ goto cleanup;
+ }
+ }
+
+ if (xi->has_note) {
+ err = GRUB_ERR_NONE;
+ goto cleanup;
+ }
+
for (s = s0; s < (Elf_Shdr *) ((char *) s0 + shnum * shentsize);
s = (Elf_Shdr *) ((char *) s + shentsize))
{
--
2.17.2
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] xen: look for Xen notes in section headers too
2019-03-14 1:57 [PATCH] xen: look for Xen notes in section headers too Marek Marczykowski-Górecki
@ 2019-03-14 12:39 ` Daniel Kiper
2019-03-14 19:52 ` Marek Marczykowski-Górecki
0 siblings, 1 reply; 4+ messages in thread
From: Daniel Kiper @ 2019-03-14 12:39 UTC (permalink / raw)
To: Marek Marczykowski-Górecki; +Cc: grub-devel
On Thu, Mar 14, 2019 at 02:57:08AM +0100, Marek Marczykowski-Górecki wrote:
> Mirror behaviour of elf loader in libxc: first look for Xen notes in
> PT_NOTE segment, then in SHT_NOTE section and only then fallback to
> a section with __xen_guest name.
> This fixes loading PV kernels that Xen note have outside of PT_NOTE.
> While this may be result of a buggy linker script, loading such kernel
> directly works fine, so make it work with grub too.
> Specifically, this applies to binaries built from Unikraft.
>
> Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
I treat this as a fix, so, Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
However, ...
> ---
> grub-core/loader/i386/xen_fileXX.c | 15 +++++++++++++++
> 1 file changed, 15 insertions(+)
>
> diff --git a/grub-core/loader/i386/xen_fileXX.c b/grub-core/loader/i386/xen_fileXX.c
> index fb66e66fe..6af6cc0da 100644
> --- a/grub-core/loader/i386/xen_fileXX.c
> +++ b/grub-core/loader/i386/xen_fileXX.c
> @@ -344,6 +344,21 @@ grub_xen_get_infoXX (grub_elf_t elf, struct grub_xen_file_info *xi)
> s = (Elf_Shdr *) ((char *) s0 + elf->ehdr.ehdrXX.e_shstrndx * shentsize);
> stroff = s->sh_offset;
>
> + for (s = s0; s < (Elf_Shdr *) ((char *) s0 + shnum * shentsize);
> + s = (Elf_Shdr *) ((char *) s + shentsize))
> + {
> + if (s->sh_type == SHT_NOTE) {
Curly bracket is in wrong place...
> + err = parse_note (elf, xi, s->sh_offset, s->sh_size);
> + if (err)
> + goto cleanup;
> + }
... and this is also in wrong place.
> + }
> +
> + if (xi->has_note) {
> + err = GRUB_ERR_NONE;
> + goto cleanup;
> + }
Ditto. Next time please double check that you have correct formatting.
Now I will fix it before the push. If there are no objections then it
will happen at the beginning of next week.
Daniel
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] xen: look for Xen notes in section headers too
2019-03-14 12:39 ` Daniel Kiper
@ 2019-03-14 19:52 ` Marek Marczykowski-Górecki
2019-03-15 14:40 ` Daniel Kiper
0 siblings, 1 reply; 4+ messages in thread
From: Marek Marczykowski-Górecki @ 2019-03-14 19:52 UTC (permalink / raw)
To: Daniel Kiper; +Cc: grub-devel
[-- Attachment #1: Type: text/plain, Size: 2463 bytes --]
On Thu, Mar 14, 2019 at 01:39:43PM +0100, Daniel Kiper wrote:
> On Thu, Mar 14, 2019 at 02:57:08AM +0100, Marek Marczykowski-Górecki wrote:
> > Mirror behaviour of elf loader in libxc: first look for Xen notes in
> > PT_NOTE segment, then in SHT_NOTE section and only then fallback to
> > a section with __xen_guest name.
> > This fixes loading PV kernels that Xen note have outside of PT_NOTE.
> > While this may be result of a buggy linker script, loading such kernel
> > directly works fine, so make it work with grub too.
> > Specifically, this applies to binaries built from Unikraft.
> >
> > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
>
> I treat this as a fix, so, Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
>
> However, ...
>
> > ---
> > grub-core/loader/i386/xen_fileXX.c | 15 +++++++++++++++
> > 1 file changed, 15 insertions(+)
> >
> > diff --git a/grub-core/loader/i386/xen_fileXX.c b/grub-core/loader/i386/xen_fileXX.c
> > index fb66e66fe..6af6cc0da 100644
> > --- a/grub-core/loader/i386/xen_fileXX.c
> > +++ b/grub-core/loader/i386/xen_fileXX.c
> > @@ -344,6 +344,21 @@ grub_xen_get_infoXX (grub_elf_t elf, struct grub_xen_file_info *xi)
> > s = (Elf_Shdr *) ((char *) s0 + elf->ehdr.ehdrXX.e_shstrndx * shentsize);
> > stroff = s->sh_offset;
> >
> > + for (s = s0; s < (Elf_Shdr *) ((char *) s0 + shnum * shentsize);
> > + s = (Elf_Shdr *) ((char *) s + shentsize))
> > + {
> > + if (s->sh_type == SHT_NOTE) {
>
> Curly bracket is in wrong place...
>
> > + err = parse_note (elf, xi, s->sh_offset, s->sh_size);
> > + if (err)
> > + goto cleanup;
> > + }
>
> ... and this is also in wrong place.
>
> > + }
> > +
> > + if (xi->has_note) {
> > + err = GRUB_ERR_NONE;
> > + goto cleanup;
> > + }
>
> Ditto. Next time please double check that you have correct formatting.
> Now I will fix it before the push. If there are no objections then it
> will happen at the beginning of next week.
Thanks. This code style is very different than any other project I
contribute to...
Anyway, there is a comment earlier in this function:
/* FIXME: check note. */
Is it about this very issue and now can be removed?
--
Best Regards,
Marek Marczykowski-Górecki
Invisible Things Lab
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 484 bytes --]
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] xen: look for Xen notes in section headers too
2019-03-14 19:52 ` Marek Marczykowski-Górecki
@ 2019-03-15 14:40 ` Daniel Kiper
0 siblings, 0 replies; 4+ messages in thread
From: Daniel Kiper @ 2019-03-15 14:40 UTC (permalink / raw)
To: Marek Marczykowski-Górecki; +Cc: grub-devel
On Thu, Mar 14, 2019 at 08:52:45PM +0100, Marek Marczykowski-Górecki wrote:
> On Thu, Mar 14, 2019 at 01:39:43PM +0100, Daniel Kiper wrote:
> > On Thu, Mar 14, 2019 at 02:57:08AM +0100, Marek Marczykowski-Górecki wrote:
> > > Mirror behaviour of elf loader in libxc: first look for Xen notes in
> > > PT_NOTE segment, then in SHT_NOTE section and only then fallback to
> > > a section with __xen_guest name.
> > > This fixes loading PV kernels that Xen note have outside of PT_NOTE.
> > > While this may be result of a buggy linker script, loading such kernel
> > > directly works fine, so make it work with grub too.
> > > Specifically, this applies to binaries built from Unikraft.
> > >
> > > Signed-off-by: Marek Marczykowski-Górecki <marmarek@invisiblethingslab.com>
> >
> > I treat this as a fix, so, Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
> >
> > However, ...
> >
> > > ---
> > > grub-core/loader/i386/xen_fileXX.c | 15 +++++++++++++++
> > > 1 file changed, 15 insertions(+)
> > >
> > > diff --git a/grub-core/loader/i386/xen_fileXX.c b/grub-core/loader/i386/xen_fileXX.c
> > > index fb66e66fe..6af6cc0da 100644
> > > --- a/grub-core/loader/i386/xen_fileXX.c
> > > +++ b/grub-core/loader/i386/xen_fileXX.c
> > > @@ -344,6 +344,21 @@ grub_xen_get_infoXX (grub_elf_t elf, struct grub_xen_file_info *xi)
> > > s = (Elf_Shdr *) ((char *) s0 + elf->ehdr.ehdrXX.e_shstrndx * shentsize);
> > > stroff = s->sh_offset;
> > >
> > > + for (s = s0; s < (Elf_Shdr *) ((char *) s0 + shnum * shentsize);
> > > + s = (Elf_Shdr *) ((char *) s + shentsize))
> > > + {
> > > + if (s->sh_type == SHT_NOTE) {
> >
> > Curly bracket is in wrong place...
> >
> > > + err = parse_note (elf, xi, s->sh_offset, s->sh_size);
> > > + if (err)
> > > + goto cleanup;
> > > + }
> >
> > ... and this is also in wrong place.
> >
> > > + }
> > > +
> > > + if (xi->has_note) {
> > > + err = GRUB_ERR_NONE;
> > > + goto cleanup;
> > > + }
> >
> > Ditto. Next time please double check that you have correct formatting.
> > Now I will fix it before the push. If there are no objections then it
> > will happen at the beginning of next week.
>
> Thanks. This code style is very different than any other project I
> contribute to...
Yeah, I know how it works.
> Anyway, there is a comment earlier in this function:
>
> /* FIXME: check note. */
>
> Is it about this very issue and now can be removed?
Hmmm... I am not sure. I am not the author of this comment.
I think that I would leave it as is if we are not sure.
Daniel
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2019-03-15 14:40 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-03-14 1:57 [PATCH] xen: look for Xen notes in section headers too Marek Marczykowski-Górecki
2019-03-14 12:39 ` Daniel Kiper
2019-03-14 19:52 ` Marek Marczykowski-Górecki
2019-03-15 14:40 ` Daniel Kiper
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.