* Re: [Xen-devel] [PATCH] libxc: elf_kernel loader: Remove check for shstrtab
2019-05-15 12:07 ` Andrew Cooper
@ 2019-05-15 12:07 ` Andrew Cooper
2019-05-15 12:09 ` Wei Liu
2019-05-15 12:55 ` Anthony PERARD
2 siblings, 0 replies; 16+ messages in thread
From: Andrew Cooper @ 2019-05-15 12:07 UTC (permalink / raw)
To: Anthony PERARD, xen-devel; +Cc: Ian Jackson, Wei Liu
On 15/05/2019 12:40, Anthony PERARD wrote:
> This was probably useful to load ELF Note, but now ELF notes
> "should live in a PT_NOTE segment" (elfnote.h).
>
> With notes living in segment, there are no need for sections, so there
> is nothing to be stored in the shstrtab.
>
> This patch would allow to write a simpler ELF header for an OVMF blob
> (which isn't an ELF) and allow it to be loaded as a PVH kernel. The
> header only needs to declare two program segments:
> - one to tell an ELF loader where to put the blob,
> - one for a Xen ELFNOTE.
>
> The ELFNOTE is to comply to the pvh design which wants the
> XEN_ELFNOTE_PHYS32_ENTRY to declare a blob as compaptible with the PVH
> boot ABI.
>
> Note that without the ELFNOTE, libxc will load an ELF but with
> the plain ELF loader, which doesn't check for shstrtab.
>
> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> ---
> tools/libxc/xc_dom_elfloader.c | 9 ---------
> 1 file changed, 9 deletions(-)
>
> diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c
> index 82b5f2ee79..b327db219d 100644
> --- a/tools/libxc/xc_dom_elfloader.c
> +++ b/tools/libxc/xc_dom_elfloader.c
> @@ -165,15 +165,6 @@ static elf_negerrnoval xc_dom_parse_elf_kernel(struct xc_dom_image *dom)
> return rc;
> }
>
> - /* Find the section-header strings table. */
> - if ( ELF_PTRVAL_INVALID(elf->sec_strtab) )
> - {
> - xc_dom_panic(dom->xch, XC_INVALID_KERNEL, "%s: ELF image"
> - " has no shstrtab", __FUNCTION__);
> - rc = -EINVAL;
> - goto out;
> - }
This might be fine for newer binaries, but you'll break older ones.
Instead, you should skip searching for strtab if we've already located
the Xen notes.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] libxc: elf_kernel loader: Remove check for shstrtab
2019-05-15 12:07 ` Andrew Cooper
2019-05-15 12:07 ` [Xen-devel] " Andrew Cooper
@ 2019-05-15 12:09 ` Wei Liu
2019-05-15 12:09 ` [Xen-devel] " Wei Liu
2019-05-15 12:55 ` Anthony PERARD
2 siblings, 1 reply; 16+ messages in thread
From: Wei Liu @ 2019-05-15 12:09 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Anthony PERARD, xen-devel, Wei Liu, Ian Jackson
On Wed, May 15, 2019 at 01:07:03PM +0100, Andrew Cooper wrote:
> On 15/05/2019 12:40, Anthony PERARD wrote:
> > This was probably useful to load ELF Note, but now ELF notes
> > "should live in a PT_NOTE segment" (elfnote.h).
> >
> > With notes living in segment, there are no need for sections, so there
> > is nothing to be stored in the shstrtab.
> >
> > This patch would allow to write a simpler ELF header for an OVMF blob
> > (which isn't an ELF) and allow it to be loaded as a PVH kernel. The
> > header only needs to declare two program segments:
> > - one to tell an ELF loader where to put the blob,
> > - one for a Xen ELFNOTE.
> >
> > The ELFNOTE is to comply to the pvh design which wants the
> > XEN_ELFNOTE_PHYS32_ENTRY to declare a blob as compaptible with the PVH
> > boot ABI.
> >
> > Note that without the ELFNOTE, libxc will load an ELF but with
> > the plain ELF loader, which doesn't check for shstrtab.
> >
> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> > ---
> > tools/libxc/xc_dom_elfloader.c | 9 ---------
> > 1 file changed, 9 deletions(-)
> >
> > diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c
> > index 82b5f2ee79..b327db219d 100644
> > --- a/tools/libxc/xc_dom_elfloader.c
> > +++ b/tools/libxc/xc_dom_elfloader.c
> > @@ -165,15 +165,6 @@ static elf_negerrnoval xc_dom_parse_elf_kernel(struct xc_dom_image *dom)
> > return rc;
> > }
> >
> > - /* Find the section-header strings table. */
> > - if ( ELF_PTRVAL_INVALID(elf->sec_strtab) )
> > - {
> > - xc_dom_panic(dom->xch, XC_INVALID_KERNEL, "%s: ELF image"
> > - " has no shstrtab", __FUNCTION__);
> > - rc = -EINVAL;
> > - goto out;
> > - }
>
> This might be fine for newer binaries, but you'll break older ones.
>
> Instead, you should skip searching for strtab if we've already located
> the Xen notes.
AIUI old binaries always have shstrtab while it isn't always true for
new ones.
Unfortunately my attempt to figure out the history of this piece of code
is futile.
Wei.
>
> ~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Xen-devel] [PATCH] libxc: elf_kernel loader: Remove check for shstrtab
2019-05-15 12:09 ` Wei Liu
@ 2019-05-15 12:09 ` Wei Liu
0 siblings, 0 replies; 16+ messages in thread
From: Wei Liu @ 2019-05-15 12:09 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Anthony PERARD, xen-devel, Wei Liu, Ian Jackson
On Wed, May 15, 2019 at 01:07:03PM +0100, Andrew Cooper wrote:
> On 15/05/2019 12:40, Anthony PERARD wrote:
> > This was probably useful to load ELF Note, but now ELF notes
> > "should live in a PT_NOTE segment" (elfnote.h).
> >
> > With notes living in segment, there are no need for sections, so there
> > is nothing to be stored in the shstrtab.
> >
> > This patch would allow to write a simpler ELF header for an OVMF blob
> > (which isn't an ELF) and allow it to be loaded as a PVH kernel. The
> > header only needs to declare two program segments:
> > - one to tell an ELF loader where to put the blob,
> > - one for a Xen ELFNOTE.
> >
> > The ELFNOTE is to comply to the pvh design which wants the
> > XEN_ELFNOTE_PHYS32_ENTRY to declare a blob as compaptible with the PVH
> > boot ABI.
> >
> > Note that without the ELFNOTE, libxc will load an ELF but with
> > the plain ELF loader, which doesn't check for shstrtab.
> >
> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> > ---
> > tools/libxc/xc_dom_elfloader.c | 9 ---------
> > 1 file changed, 9 deletions(-)
> >
> > diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c
> > index 82b5f2ee79..b327db219d 100644
> > --- a/tools/libxc/xc_dom_elfloader.c
> > +++ b/tools/libxc/xc_dom_elfloader.c
> > @@ -165,15 +165,6 @@ static elf_negerrnoval xc_dom_parse_elf_kernel(struct xc_dom_image *dom)
> > return rc;
> > }
> >
> > - /* Find the section-header strings table. */
> > - if ( ELF_PTRVAL_INVALID(elf->sec_strtab) )
> > - {
> > - xc_dom_panic(dom->xch, XC_INVALID_KERNEL, "%s: ELF image"
> > - " has no shstrtab", __FUNCTION__);
> > - rc = -EINVAL;
> > - goto out;
> > - }
>
> This might be fine for newer binaries, but you'll break older ones.
>
> Instead, you should skip searching for strtab if we've already located
> the Xen notes.
AIUI old binaries always have shstrtab while it isn't always true for
new ones.
Unfortunately my attempt to figure out the history of this piece of code
is futile.
Wei.
>
> ~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH] libxc: elf_kernel loader: Remove check for shstrtab
2019-05-15 12:07 ` Andrew Cooper
2019-05-15 12:07 ` [Xen-devel] " Andrew Cooper
2019-05-15 12:09 ` Wei Liu
@ 2019-05-15 12:55 ` Anthony PERARD
2019-05-15 12:55 ` [Xen-devel] " Anthony PERARD
2019-05-16 13:23 ` Wei Liu
2 siblings, 2 replies; 16+ messages in thread
From: Anthony PERARD @ 2019-05-15 12:55 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Wei Liu, Ian Jackson
On Wed, May 15, 2019 at 01:07:03PM +0100, Andrew Cooper wrote:
> On 15/05/2019 12:40, Anthony PERARD wrote:
> > This was probably useful to load ELF Note, but now ELF notes
> > "should live in a PT_NOTE segment" (elfnote.h).
> >
> > With notes living in segment, there are no need for sections, so there
> > is nothing to be stored in the shstrtab.
> >
> > This patch would allow to write a simpler ELF header for an OVMF blob
> > (which isn't an ELF) and allow it to be loaded as a PVH kernel. The
> > header only needs to declare two program segments:
> > - one to tell an ELF loader where to put the blob,
> > - one for a Xen ELFNOTE.
> >
> > The ELFNOTE is to comply to the pvh design which wants the
> > XEN_ELFNOTE_PHYS32_ENTRY to declare a blob as compaptible with the PVH
> > boot ABI.
> >
> > Note that without the ELFNOTE, libxc will load an ELF but with
> > the plain ELF loader, which doesn't check for shstrtab.
> >
> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> > ---
> > tools/libxc/xc_dom_elfloader.c | 9 ---------
> > 1 file changed, 9 deletions(-)
> >
> > diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c
> > index 82b5f2ee79..b327db219d 100644
> > --- a/tools/libxc/xc_dom_elfloader.c
> > +++ b/tools/libxc/xc_dom_elfloader.c
> > @@ -165,15 +165,6 @@ static elf_negerrnoval xc_dom_parse_elf_kernel(struct xc_dom_image *dom)
> > return rc;
> > }
> >
> > - /* Find the section-header strings table. */
> > - if ( ELF_PTRVAL_INVALID(elf->sec_strtab) )
> > - {
> > - xc_dom_panic(dom->xch, XC_INVALID_KERNEL, "%s: ELF image"
> > - " has no shstrtab", __FUNCTION__);
> > - rc = -EINVAL;
> > - goto out;
> > - }
>
> This might be fine for newer binaries, but you'll break older ones.
>
> Instead, you should skip searching for strtab if we've already located
> the Xen notes.
:-(, maybe I should have gone futher on explaining why this check is
useless (and probably at the wrong place, at least now).
The next thing that's done after that check is:
elf_parse_binary()
elf_xen_parse()
Those are located in "xen/common/libelf", and those are the functions
that actually takes care of extracting data from the elf.
elf_xen_parse() first look for Xen ELFNOTE in the program segments
(phdr, PT_NOTE) and skip reading section and strtab if found.
So, libelf already does what you asked for ;-).
The shstrtab are only used to look for legacy __xen_guest section names.
Since ELFNOTEs was used, the name of section aren't looked at.
I hope that help.
Thanks,
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Xen-devel] [PATCH] libxc: elf_kernel loader: Remove check for shstrtab
2019-05-15 12:55 ` Anthony PERARD
@ 2019-05-15 12:55 ` Anthony PERARD
2019-05-16 13:23 ` Wei Liu
1 sibling, 0 replies; 16+ messages in thread
From: Anthony PERARD @ 2019-05-15 12:55 UTC (permalink / raw)
To: Andrew Cooper; +Cc: xen-devel, Wei Liu, Ian Jackson
On Wed, May 15, 2019 at 01:07:03PM +0100, Andrew Cooper wrote:
> On 15/05/2019 12:40, Anthony PERARD wrote:
> > This was probably useful to load ELF Note, but now ELF notes
> > "should live in a PT_NOTE segment" (elfnote.h).
> >
> > With notes living in segment, there are no need for sections, so there
> > is nothing to be stored in the shstrtab.
> >
> > This patch would allow to write a simpler ELF header for an OVMF blob
> > (which isn't an ELF) and allow it to be loaded as a PVH kernel. The
> > header only needs to declare two program segments:
> > - one to tell an ELF loader where to put the blob,
> > - one for a Xen ELFNOTE.
> >
> > The ELFNOTE is to comply to the pvh design which wants the
> > XEN_ELFNOTE_PHYS32_ENTRY to declare a blob as compaptible with the PVH
> > boot ABI.
> >
> > Note that without the ELFNOTE, libxc will load an ELF but with
> > the plain ELF loader, which doesn't check for shstrtab.
> >
> > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> > ---
> > tools/libxc/xc_dom_elfloader.c | 9 ---------
> > 1 file changed, 9 deletions(-)
> >
> > diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c
> > index 82b5f2ee79..b327db219d 100644
> > --- a/tools/libxc/xc_dom_elfloader.c
> > +++ b/tools/libxc/xc_dom_elfloader.c
> > @@ -165,15 +165,6 @@ static elf_negerrnoval xc_dom_parse_elf_kernel(struct xc_dom_image *dom)
> > return rc;
> > }
> >
> > - /* Find the section-header strings table. */
> > - if ( ELF_PTRVAL_INVALID(elf->sec_strtab) )
> > - {
> > - xc_dom_panic(dom->xch, XC_INVALID_KERNEL, "%s: ELF image"
> > - " has no shstrtab", __FUNCTION__);
> > - rc = -EINVAL;
> > - goto out;
> > - }
>
> This might be fine for newer binaries, but you'll break older ones.
>
> Instead, you should skip searching for strtab if we've already located
> the Xen notes.
:-(, maybe I should have gone futher on explaining why this check is
useless (and probably at the wrong place, at least now).
The next thing that's done after that check is:
elf_parse_binary()
elf_xen_parse()
Those are located in "xen/common/libelf", and those are the functions
that actually takes care of extracting data from the elf.
elf_xen_parse() first look for Xen ELFNOTE in the program segments
(phdr, PT_NOTE) and skip reading section and strtab if found.
So, libelf already does what you asked for ;-).
The shstrtab are only used to look for legacy __xen_guest section names.
Since ELFNOTEs was used, the name of section aren't looked at.
I hope that help.
Thanks,
--
Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] libxc: elf_kernel loader: Remove check for shstrtab
2019-05-15 12:55 ` Anthony PERARD
2019-05-15 12:55 ` [Xen-devel] " Anthony PERARD
@ 2019-05-16 13:23 ` Wei Liu
2019-05-16 13:23 ` [Xen-devel] " Wei Liu
2019-05-16 13:38 ` Andrew Cooper
1 sibling, 2 replies; 16+ messages in thread
From: Wei Liu @ 2019-05-16 13:23 UTC (permalink / raw)
To: Anthony PERARD; +Cc: Andrew Cooper, Wei Liu, Ian Jackson, xen-devel
On Wed, May 15, 2019 at 01:55:30PM +0100, Anthony PERARD wrote:
> On Wed, May 15, 2019 at 01:07:03PM +0100, Andrew Cooper wrote:
> > On 15/05/2019 12:40, Anthony PERARD wrote:
> > > This was probably useful to load ELF Note, but now ELF notes
> > > "should live in a PT_NOTE segment" (elfnote.h).
> > >
> > > With notes living in segment, there are no need for sections, so there
> > > is nothing to be stored in the shstrtab.
> > >
> > > This patch would allow to write a simpler ELF header for an OVMF blob
> > > (which isn't an ELF) and allow it to be loaded as a PVH kernel. The
> > > header only needs to declare two program segments:
> > > - one to tell an ELF loader where to put the blob,
> > > - one for a Xen ELFNOTE.
> > >
> > > The ELFNOTE is to comply to the pvh design which wants the
> > > XEN_ELFNOTE_PHYS32_ENTRY to declare a blob as compaptible with the PVH
> > > boot ABI.
> > >
> > > Note that without the ELFNOTE, libxc will load an ELF but with
> > > the plain ELF loader, which doesn't check for shstrtab.
> > >
> > > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> > > ---
> > > tools/libxc/xc_dom_elfloader.c | 9 ---------
> > > 1 file changed, 9 deletions(-)
> > >
> > > diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c
> > > index 82b5f2ee79..b327db219d 100644
> > > --- a/tools/libxc/xc_dom_elfloader.c
> > > +++ b/tools/libxc/xc_dom_elfloader.c
> > > @@ -165,15 +165,6 @@ static elf_negerrnoval xc_dom_parse_elf_kernel(struct xc_dom_image *dom)
> > > return rc;
> > > }
> > >
> > > - /* Find the section-header strings table. */
> > > - if ( ELF_PTRVAL_INVALID(elf->sec_strtab) )
> > > - {
> > > - xc_dom_panic(dom->xch, XC_INVALID_KERNEL, "%s: ELF image"
> > > - " has no shstrtab", __FUNCTION__);
> > > - rc = -EINVAL;
> > > - goto out;
> > > - }
> >
> > This might be fine for newer binaries, but you'll break older ones.
> >
> > Instead, you should skip searching for strtab if we've already located
> > the Xen notes.
>
> :-(, maybe I should have gone futher on explaining why this check is
> useless (and probably at the wrong place, at least now).
>
> The next thing that's done after that check is:
> elf_parse_binary()
> elf_xen_parse()
> Those are located in "xen/common/libelf", and those are the functions
> that actually takes care of extracting data from the elf.
>
> elf_xen_parse() first look for Xen ELFNOTE in the program segments
> (phdr, PT_NOTE) and skip reading section and strtab if found.
>
> So, libelf already does what you asked for ;-).
>
> The shstrtab are only used to look for legacy __xen_guest section names.
> Since ELFNOTEs was used, the name of section aren't looked at.
>
> I hope that help.
>
Andrew, do you still have concern?
Wei.
> Thanks,
>
> --
> Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Xen-devel] [PATCH] libxc: elf_kernel loader: Remove check for shstrtab
2019-05-16 13:23 ` Wei Liu
@ 2019-05-16 13:23 ` Wei Liu
2019-05-16 13:38 ` Andrew Cooper
1 sibling, 0 replies; 16+ messages in thread
From: Wei Liu @ 2019-05-16 13:23 UTC (permalink / raw)
To: Anthony PERARD; +Cc: Andrew Cooper, Wei Liu, Ian Jackson, xen-devel
On Wed, May 15, 2019 at 01:55:30PM +0100, Anthony PERARD wrote:
> On Wed, May 15, 2019 at 01:07:03PM +0100, Andrew Cooper wrote:
> > On 15/05/2019 12:40, Anthony PERARD wrote:
> > > This was probably useful to load ELF Note, but now ELF notes
> > > "should live in a PT_NOTE segment" (elfnote.h).
> > >
> > > With notes living in segment, there are no need for sections, so there
> > > is nothing to be stored in the shstrtab.
> > >
> > > This patch would allow to write a simpler ELF header for an OVMF blob
> > > (which isn't an ELF) and allow it to be loaded as a PVH kernel. The
> > > header only needs to declare two program segments:
> > > - one to tell an ELF loader where to put the blob,
> > > - one for a Xen ELFNOTE.
> > >
> > > The ELFNOTE is to comply to the pvh design which wants the
> > > XEN_ELFNOTE_PHYS32_ENTRY to declare a blob as compaptible with the PVH
> > > boot ABI.
> > >
> > > Note that without the ELFNOTE, libxc will load an ELF but with
> > > the plain ELF loader, which doesn't check for shstrtab.
> > >
> > > Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
> > > ---
> > > tools/libxc/xc_dom_elfloader.c | 9 ---------
> > > 1 file changed, 9 deletions(-)
> > >
> > > diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c
> > > index 82b5f2ee79..b327db219d 100644
> > > --- a/tools/libxc/xc_dom_elfloader.c
> > > +++ b/tools/libxc/xc_dom_elfloader.c
> > > @@ -165,15 +165,6 @@ static elf_negerrnoval xc_dom_parse_elf_kernel(struct xc_dom_image *dom)
> > > return rc;
> > > }
> > >
> > > - /* Find the section-header strings table. */
> > > - if ( ELF_PTRVAL_INVALID(elf->sec_strtab) )
> > > - {
> > > - xc_dom_panic(dom->xch, XC_INVALID_KERNEL, "%s: ELF image"
> > > - " has no shstrtab", __FUNCTION__);
> > > - rc = -EINVAL;
> > > - goto out;
> > > - }
> >
> > This might be fine for newer binaries, but you'll break older ones.
> >
> > Instead, you should skip searching for strtab if we've already located
> > the Xen notes.
>
> :-(, maybe I should have gone futher on explaining why this check is
> useless (and probably at the wrong place, at least now).
>
> The next thing that's done after that check is:
> elf_parse_binary()
> elf_xen_parse()
> Those are located in "xen/common/libelf", and those are the functions
> that actually takes care of extracting data from the elf.
>
> elf_xen_parse() first look for Xen ELFNOTE in the program segments
> (phdr, PT_NOTE) and skip reading section and strtab if found.
>
> So, libelf already does what you asked for ;-).
>
> The shstrtab are only used to look for legacy __xen_guest section names.
> Since ELFNOTEs was used, the name of section aren't looked at.
>
> I hope that help.
>
Andrew, do you still have concern?
Wei.
> Thanks,
>
> --
> Anthony PERARD
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [PATCH] libxc: elf_kernel loader: Remove check for shstrtab
2019-05-16 13:23 ` Wei Liu
2019-05-16 13:23 ` [Xen-devel] " Wei Liu
@ 2019-05-16 13:38 ` Andrew Cooper
2019-05-16 13:38 ` [Xen-devel] " Andrew Cooper
1 sibling, 1 reply; 16+ messages in thread
From: Andrew Cooper @ 2019-05-16 13:38 UTC (permalink / raw)
To: Wei Liu, Anthony PERARD; +Cc: xen-devel, Ian Jackson
On 16/05/2019 14:23, Wei Liu wrote:
> On Wed, May 15, 2019 at 01:55:30PM +0100, Anthony PERARD wrote:
>> On Wed, May 15, 2019 at 01:07:03PM +0100, Andrew Cooper wrote:
>>> On 15/05/2019 12:40, Anthony PERARD wrote:
>>>> This was probably useful to load ELF Note, but now ELF notes
>>>> "should live in a PT_NOTE segment" (elfnote.h).
>>>>
>>>> With notes living in segment, there are no need for sections, so there
>>>> is nothing to be stored in the shstrtab.
>>>>
>>>> This patch would allow to write a simpler ELF header for an OVMF blob
>>>> (which isn't an ELF) and allow it to be loaded as a PVH kernel. The
>>>> header only needs to declare two program segments:
>>>> - one to tell an ELF loader where to put the blob,
>>>> - one for a Xen ELFNOTE.
>>>>
>>>> The ELFNOTE is to comply to the pvh design which wants the
>>>> XEN_ELFNOTE_PHYS32_ENTRY to declare a blob as compaptible with the PVH
>>>> boot ABI.
>>>>
>>>> Note that without the ELFNOTE, libxc will load an ELF but with
>>>> the plain ELF loader, which doesn't check for shstrtab.
>>>>
>>>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>>>> ---
>>>> tools/libxc/xc_dom_elfloader.c | 9 ---------
>>>> 1 file changed, 9 deletions(-)
>>>>
>>>> diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c
>>>> index 82b5f2ee79..b327db219d 100644
>>>> --- a/tools/libxc/xc_dom_elfloader.c
>>>> +++ b/tools/libxc/xc_dom_elfloader.c
>>>> @@ -165,15 +165,6 @@ static elf_negerrnoval xc_dom_parse_elf_kernel(struct xc_dom_image *dom)
>>>> return rc;
>>>> }
>>>>
>>>> - /* Find the section-header strings table. */
>>>> - if ( ELF_PTRVAL_INVALID(elf->sec_strtab) )
>>>> - {
>>>> - xc_dom_panic(dom->xch, XC_INVALID_KERNEL, "%s: ELF image"
>>>> - " has no shstrtab", __FUNCTION__);
>>>> - rc = -EINVAL;
>>>> - goto out;
>>>> - }
>>> This might be fine for newer binaries, but you'll break older ones.
>>>
>>> Instead, you should skip searching for strtab if we've already located
>>> the Xen notes.
>> :-(, maybe I should have gone futher on explaining why this check is
>> useless (and probably at the wrong place, at least now).
>>
>> The next thing that's done after that check is:
>> elf_parse_binary()
>> elf_xen_parse()
>> Those are located in "xen/common/libelf", and those are the functions
>> that actually takes care of extracting data from the elf.
>>
>> elf_xen_parse() first look for Xen ELFNOTE in the program segments
>> (phdr, PT_NOTE) and skip reading section and strtab if found.
>>
>> So, libelf already does what you asked for ;-).
>>
>> The shstrtab are only used to look for legacy __xen_guest section names.
>> Since ELFNOTEs was used, the name of section aren't looked at.
>>
>> I hope that help.
>>
> Andrew, do you still have concern?
If libelf goes on to DTRT then fine, but this full explanation needs to
be in the commit message.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread* Re: [Xen-devel] [PATCH] libxc: elf_kernel loader: Remove check for shstrtab
2019-05-16 13:38 ` Andrew Cooper
@ 2019-05-16 13:38 ` Andrew Cooper
0 siblings, 0 replies; 16+ messages in thread
From: Andrew Cooper @ 2019-05-16 13:38 UTC (permalink / raw)
To: Wei Liu, Anthony PERARD; +Cc: xen-devel, Ian Jackson
On 16/05/2019 14:23, Wei Liu wrote:
> On Wed, May 15, 2019 at 01:55:30PM +0100, Anthony PERARD wrote:
>> On Wed, May 15, 2019 at 01:07:03PM +0100, Andrew Cooper wrote:
>>> On 15/05/2019 12:40, Anthony PERARD wrote:
>>>> This was probably useful to load ELF Note, but now ELF notes
>>>> "should live in a PT_NOTE segment" (elfnote.h).
>>>>
>>>> With notes living in segment, there are no need for sections, so there
>>>> is nothing to be stored in the shstrtab.
>>>>
>>>> This patch would allow to write a simpler ELF header for an OVMF blob
>>>> (which isn't an ELF) and allow it to be loaded as a PVH kernel. The
>>>> header only needs to declare two program segments:
>>>> - one to tell an ELF loader where to put the blob,
>>>> - one for a Xen ELFNOTE.
>>>>
>>>> The ELFNOTE is to comply to the pvh design which wants the
>>>> XEN_ELFNOTE_PHYS32_ENTRY to declare a blob as compaptible with the PVH
>>>> boot ABI.
>>>>
>>>> Note that without the ELFNOTE, libxc will load an ELF but with
>>>> the plain ELF loader, which doesn't check for shstrtab.
>>>>
>>>> Signed-off-by: Anthony PERARD <anthony.perard@citrix.com>
>>>> ---
>>>> tools/libxc/xc_dom_elfloader.c | 9 ---------
>>>> 1 file changed, 9 deletions(-)
>>>>
>>>> diff --git a/tools/libxc/xc_dom_elfloader.c b/tools/libxc/xc_dom_elfloader.c
>>>> index 82b5f2ee79..b327db219d 100644
>>>> --- a/tools/libxc/xc_dom_elfloader.c
>>>> +++ b/tools/libxc/xc_dom_elfloader.c
>>>> @@ -165,15 +165,6 @@ static elf_negerrnoval xc_dom_parse_elf_kernel(struct xc_dom_image *dom)
>>>> return rc;
>>>> }
>>>>
>>>> - /* Find the section-header strings table. */
>>>> - if ( ELF_PTRVAL_INVALID(elf->sec_strtab) )
>>>> - {
>>>> - xc_dom_panic(dom->xch, XC_INVALID_KERNEL, "%s: ELF image"
>>>> - " has no shstrtab", __FUNCTION__);
>>>> - rc = -EINVAL;
>>>> - goto out;
>>>> - }
>>> This might be fine for newer binaries, but you'll break older ones.
>>>
>>> Instead, you should skip searching for strtab if we've already located
>>> the Xen notes.
>> :-(, maybe I should have gone futher on explaining why this check is
>> useless (and probably at the wrong place, at least now).
>>
>> The next thing that's done after that check is:
>> elf_parse_binary()
>> elf_xen_parse()
>> Those are located in "xen/common/libelf", and those are the functions
>> that actually takes care of extracting data from the elf.
>>
>> elf_xen_parse() first look for Xen ELFNOTE in the program segments
>> (phdr, PT_NOTE) and skip reading section and strtab if found.
>>
>> So, libelf already does what you asked for ;-).
>>
>> The shstrtab are only used to look for legacy __xen_guest section names.
>> Since ELFNOTEs was used, the name of section aren't looked at.
>>
>> I hope that help.
>>
> Andrew, do you still have concern?
If libelf goes on to DTRT then fine, but this full explanation needs to
be in the commit message.
~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply [flat|nested] 16+ messages in thread