[PATCH] libelf: Handle PVH kernels lacking ENTRY elfnote

Jason Andryuk posted 1 patch 2 weeks ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/xen tags/patchew/20201014153150.83875-1-jandryuk@gmail.com
Maintainers: Stefano Stabellini <sstabellini@kernel.org>, Wei Liu <wl@xen.org>, Ian Jackson <iwj@xenproject.org>, Jan Beulich <jbeulich@suse.com>, George Dunlap <george.dunlap@citrix.com>, Andrew Cooper <andrew.cooper3@citrix.com>, Julien Grall <julien@xen.org>
xen/common/libelf/libelf-dominfo.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)

[PATCH] libelf: Handle PVH kernels lacking ENTRY elfnote

Posted by Jason Andryuk 2 weeks ago
Linux kernels only have an ENTRY elfnote when built with CONFIG_PV.  A
kernel build CONFIG_PVH=y CONFIG_PV=n lacks the note.  In this case,
virt_entry will be UNSET_ADDR, overwritten by the ELF header e_entry,
and fail the check against the virt address range.

Change the code to only check virt_entry against the virtual address
range if it was set upon entry to the function.

Signed-off-by: Jason Andryuk <jandryuk@gmail.com>

---
Maybe the overwriting of virt_entry could be removed, but I don't know
if there would be unintended consequences where (old?) kernels don't
have an elfnote, but do have an in-range e_entry?  The failing kernel I
just looked at has an e_entry of 0x1000000.

Oh, it looks like Mini-OS doesn't set the entry ELFNOTE and relies on
e_entry (of 0) to pass these checks.

---
 xen/common/libelf/libelf-dominfo.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c
index 508f08db42..1ecf35166b 100644
--- a/xen/common/libelf/libelf-dominfo.c
+++ b/xen/common/libelf/libelf-dominfo.c
@@ -416,6 +416,7 @@ static elf_errorstatus elf_xen_note_check(struct elf_binary *elf,
 static elf_errorstatus elf_xen_addr_calc_check(struct elf_binary *elf,
                                    struct elf_dom_parms *parms)
 {
+    bool check_virt_entry = true;
     uint64_t virt_offset;
 
     if ( (parms->elf_paddr_offset != UNSET_ADDR) &&
@@ -456,8 +457,10 @@ static elf_errorstatus elf_xen_addr_calc_check(struct elf_binary *elf,
     parms->virt_kstart = elf->pstart + virt_offset;
     parms->virt_kend   = elf->pend   + virt_offset;
 
-    if ( parms->virt_entry == UNSET_ADDR )
+    if ( parms->virt_entry == UNSET_ADDR ) {
         parms->virt_entry = elf_uval(elf, elf->ehdr, e_entry);
+        check_virt_entry = false;
+    }
 
     if ( parms->bsd_symtab )
     {
@@ -476,11 +479,17 @@ static elf_errorstatus elf_xen_addr_calc_check(struct elf_binary *elf,
     elf_msg(elf, "    p2m_base         = 0x%" PRIx64 "\n", parms->p2m_base);
 
     if ( (parms->virt_kstart > parms->virt_kend) ||
-         (parms->virt_entry < parms->virt_kstart) ||
-         (parms->virt_entry > parms->virt_kend) ||
          (parms->virt_base > parms->virt_kstart) )
     {
-        elf_err(elf, "ERROR: ELF start or entries are out of bounds\n");
+        elf_err(elf, "ERROR: ELF start is out of bounds\n");
+        return -1;
+    }
+
+    if ( check_virt_entry &&
+         ( (parms->virt_entry < parms->virt_kstart) ||
+           (parms->virt_entry > parms->virt_kend) ) )
+    {
+        elf_err(elf, "ERROR: ELF entry is out of bounds\n");
         return -1;
     }
 
-- 
2.26.2


Re: [PATCH] libelf: Handle PVH kernels lacking ENTRY elfnote

Posted by Jürgen Groß 2 weeks ago
On 14.10.20 17:31, Jason Andryuk wrote:
> Linux kernels only have an ENTRY elfnote when built with CONFIG_PV.  A

This wrong. Have a look into arch/x86/platform/pvh/head.S


Juergen

Re: [PATCH] libelf: Handle PVH kernels lacking ENTRY elfnote

Posted by Jason Andryuk 2 weeks ago
On Wed, Oct 14, 2020 at 12:12 PM Jürgen Groß <jgross@suse.com> wrote:
>
> On 14.10.20 17:31, Jason Andryuk wrote:
> > Linux kernels only have an ENTRY elfnote when built with CONFIG_PV.  A
>
> This wrong. Have a look into arch/x86/platform/pvh/head.S

That is XEN_ELFNOTE_PHYS32_ENTRY, which is different from
XEN_ELFNOTE_ENTRY in arch/x86/xen/xen-head.S:
#ifdef CONFIG_XEN_PV
        ELFNOTE(Xen, XEN_ELFNOTE_ENTRY,          _ASM_PTR startup_xen)
#endif

Regards,
Jason

Re: [PATCH] libelf: Handle PVH kernels lacking ENTRY elfnote

Posted by Jürgen Groß 2 weeks ago
On 14.10.20 18:27, Jason Andryuk wrote:
> On Wed, Oct 14, 2020 at 12:12 PM Jürgen Groß <jgross@suse.com> wrote:
>>
>> On 14.10.20 17:31, Jason Andryuk wrote:
>>> Linux kernels only have an ENTRY elfnote when built with CONFIG_PV.  A
>>
>> This wrong. Have a look into arch/x86/platform/pvh/head.S
> 
> That is XEN_ELFNOTE_PHYS32_ENTRY, which is different from
> XEN_ELFNOTE_ENTRY in arch/x86/xen/xen-head.S:
> #ifdef CONFIG_XEN_PV
>          ELFNOTE(Xen, XEN_ELFNOTE_ENTRY,          _ASM_PTR startup_xen)
> #endif

Oh, sorry, I shouldn't have answered when being in a hurry.

I misunderstood the purpose of the patch.


Sorry for the noise,

Juergen


Re: [PATCH] libelf: Handle PVH kernels lacking ENTRY elfnote

Posted by Jan Beulich 2 weeks ago
On 14.10.2020 17:31, Jason Andryuk wrote:
> Linux kernels only have an ENTRY elfnote when built with CONFIG_PV.  A
> kernel build CONFIG_PVH=y CONFIG_PV=n lacks the note.  In this case,
> virt_entry will be UNSET_ADDR, overwritten by the ELF header e_entry,
> and fail the check against the virt address range.
> 
> Change the code to only check virt_entry against the virtual address
> range if it was set upon entry to the function.

Not checking at all seems wrong to me. The ELF spec anyway says
"virtual address", so an out of bounds value is at least suspicious.

> Maybe the overwriting of virt_entry could be removed, but I don't know
> if there would be unintended consequences where (old?) kernels don't
> have an elfnote, but do have an in-range e_entry?  The failing kernel I
> just looked at has an e_entry of 0x1000000.

And if you dropped the overwriting, what entry point would we use
in the absence of an ELF note?

I'd rather put up the option of adjusting the entry (or the check),
if it looks like a valid physical address.

Jan

Re: [PATCH] libelf: Handle PVH kernels lacking ENTRY elfnote

Posted by Jason Andryuk 2 weeks ago
On Wed, Oct 14, 2020 at 12:02 PM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 14.10.2020 17:31, Jason Andryuk wrote:
> > Linux kernels only have an ENTRY elfnote when built with CONFIG_PV.  A
> > kernel build CONFIG_PVH=y CONFIG_PV=n lacks the note.  In this case,
> > virt_entry will be UNSET_ADDR, overwritten by the ELF header e_entry,
> > and fail the check against the virt address range.

Oh, these should be CONFIG_XEN_PVH=y and CONFIG_XEN_PV=n

> > Change the code to only check virt_entry against the virtual address
> > range if it was set upon entry to the function.
>
> Not checking at all seems wrong to me. The ELF spec anyway says
> "virtual address", so an out of bounds value is at least suspicious.
>
> > Maybe the overwriting of virt_entry could be removed, but I don't know
> > if there would be unintended consequences where (old?) kernels don't
> > have an elfnote, but do have an in-range e_entry?  The failing kernel I
> > just looked at has an e_entry of 0x1000000.
>
> And if you dropped the overwriting, what entry point would we use
> in the absence of an ELF note?

elf_xen_note_check currently has:

    /* PVH only requires one ELF note to be set */
    if ( parms->phys_entry != UNSET_ADDR32 )
    {
        elf_msg(elf, "ELF: Found PVH image\n");
        return 0;
    }

> I'd rather put up the option of adjusting the entry (or the check),
> if it looks like a valid physical address.

The function doesn't know if the image will be booted PV or PVH, so I
guess we do all the checks, but use 'parms->phys_entry != UNSET_ADDR32
&& parms->virt_entry == UNSET_ADDR' to conditionally skip checking
virt?

Regards,
Jason

Re: [PATCH] libelf: Handle PVH kernels lacking ENTRY elfnote

Posted by Jan Beulich 2 weeks ago
On 14.10.2020 18:27, Jason Andryuk wrote:
> On Wed, Oct 14, 2020 at 12:02 PM Jan Beulich <jbeulich@suse.com> wrote:
>>
>> On 14.10.2020 17:31, Jason Andryuk wrote:
>>> Linux kernels only have an ENTRY elfnote when built with CONFIG_PV.  A
>>> kernel build CONFIG_PVH=y CONFIG_PV=n lacks the note.  In this case,
>>> virt_entry will be UNSET_ADDR, overwritten by the ELF header e_entry,
>>> and fail the check against the virt address range.
> 
> Oh, these should be CONFIG_XEN_PVH=y and CONFIG_XEN_PV=n
> 
>>> Change the code to only check virt_entry against the virtual address
>>> range if it was set upon entry to the function.
>>
>> Not checking at all seems wrong to me. The ELF spec anyway says
>> "virtual address", so an out of bounds value is at least suspicious.
>>
>>> Maybe the overwriting of virt_entry could be removed, but I don't know
>>> if there would be unintended consequences where (old?) kernels don't
>>> have an elfnote, but do have an in-range e_entry?  The failing kernel I
>>> just looked at has an e_entry of 0x1000000.
>>
>> And if you dropped the overwriting, what entry point would we use
>> in the absence of an ELF note?
> 
> elf_xen_note_check currently has:
> 
>     /* PVH only requires one ELF note to be set */
>     if ( parms->phys_entry != UNSET_ADDR32 )
>     {
>         elf_msg(elf, "ELF: Found PVH image\n");
>         return 0;
>     }
> 
>> I'd rather put up the option of adjusting the entry (or the check),
>> if it looks like a valid physical address.
> 
> The function doesn't know if the image will be booted PV or PVH, so I
> guess we do all the checks, but use 'parms->phys_entry != UNSET_ADDR32
> && parms->virt_entry == UNSET_ADDR' to conditionally skip checking
> virt?

Like Jürgen, the purpose of the patch hadn't become clear to me
from reading the description. As I understand it now, we're currently
refusing to boot such a kernel for no reason. If that's correct,
perhaps you could say so in the description in a more direct way?

As far as actual code adjustments go - how much of
elf_xen_addr_calc_check() is actually applicable when booting PVH?

And why is there no bounds check of ->phys_entry paralleling the
->virt_entry one?

On the whole, as long as we don't know what mode we're planning to
boot in, we can't skip any checks, as the mere presence of
XEN_ELFNOTE_PHYS32_ENTRY doesn't mean that's also what gets used.
Therefore simply bypassing any of the checks is not an option. In
particular what you suggest would lead to failure to check
e_entry-derived ->virt_entry when the PVH-specific note is
present but we're booting in PV mode. For now I don't see how to
address this without making the function aware of the intended
booting mode.

Jan

Re: [PATCH] libelf: Handle PVH kernels lacking ENTRY elfnote

Posted by Roger Pau Monné 2 weeks ago
On Thu, Oct 15, 2020 at 09:00:09AM +0200, Jan Beulich wrote:
> On 14.10.2020 18:27, Jason Andryuk wrote:
> > On Wed, Oct 14, 2020 at 12:02 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 14.10.2020 17:31, Jason Andryuk wrote:
> >>> Linux kernels only have an ENTRY elfnote when built with CONFIG_PV.  A
> >>> kernel build CONFIG_PVH=y CONFIG_PV=n lacks the note.  In this case,
> >>> virt_entry will be UNSET_ADDR, overwritten by the ELF header e_entry,
> >>> and fail the check against the virt address range.
> > 
> > Oh, these should be CONFIG_XEN_PVH=y and CONFIG_XEN_PV=n
> > 
> >>> Change the code to only check virt_entry against the virtual address
> >>> range if it was set upon entry to the function.
> >>
> >> Not checking at all seems wrong to me. The ELF spec anyway says
> >> "virtual address", so an out of bounds value is at least suspicious.
> >>
> >>> Maybe the overwriting of virt_entry could be removed, but I don't know
> >>> if there would be unintended consequences where (old?) kernels don't
> >>> have an elfnote, but do have an in-range e_entry?  The failing kernel I
> >>> just looked at has an e_entry of 0x1000000.
> >>
> >> And if you dropped the overwriting, what entry point would we use
> >> in the absence of an ELF note?
> > 
> > elf_xen_note_check currently has:
> > 
> >     /* PVH only requires one ELF note to be set */
> >     if ( parms->phys_entry != UNSET_ADDR32 )
> >     {
> >         elf_msg(elf, "ELF: Found PVH image\n");
> >         return 0;
> >     }
> > 
> >> I'd rather put up the option of adjusting the entry (or the check),
> >> if it looks like a valid physical address.
> > 
> > The function doesn't know if the image will be booted PV or PVH, so I
> > guess we do all the checks, but use 'parms->phys_entry != UNSET_ADDR32
> > && parms->virt_entry == UNSET_ADDR' to conditionally skip checking
> > virt?
> 
> Like Jürgen, the purpose of the patch hadn't become clear to me
> from reading the description. As I understand it now, we're currently
> refusing to boot such a kernel for no reason. If that's correct,
> perhaps you could say so in the description in a more direct way?
> 
> As far as actual code adjustments go - how much of
> elf_xen_addr_calc_check() is actually applicable when booting PVH?

I think the only relevant check for PVH would be the symtab loading
(XEN_ELFNOTE_BSD_SYMTAB).

> And why is there no bounds check of ->phys_entry paralleling the
> ->virt_entry one?
> 
> On the whole, as long as we don't know what mode we're planning to
> boot in, we can't skip any checks, as the mere presence of
> XEN_ELFNOTE_PHYS32_ENTRY doesn't mean that's also what gets used.
> Therefore simply bypassing any of the checks is not an option. In
> particular what you suggest would lead to failure to check
> e_entry-derived ->virt_entry when the PVH-specific note is
> present but we're booting in PV mode. For now I don't see how to
> address this without making the function aware of the intended
> booting mode.

That seems the only viable approach. Maybe an intended mode field could
be added to elf_dom_parms in order to signal this?

Thanks, Roger.

Re: [PATCH] libelf: Handle PVH kernels lacking ENTRY elfnote

Posted by Jason Andryuk 2 weeks ago
On Thu, Oct 15, 2020 at 3:00 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 14.10.2020 18:27, Jason Andryuk wrote:
> > On Wed, Oct 14, 2020 at 12:02 PM Jan Beulich <jbeulich@suse.com> wrote:
> >>
> >> On 14.10.2020 17:31, Jason Andryuk wrote:
> >>> Linux kernels only have an ENTRY elfnote when built with CONFIG_PV.  A
> >>> kernel build CONFIG_PVH=y CONFIG_PV=n lacks the note.  In this case,
> >>> virt_entry will be UNSET_ADDR, overwritten by the ELF header e_entry,
> >>> and fail the check against the virt address range.
> >
> > Oh, these should be CONFIG_XEN_PVH=y and CONFIG_XEN_PV=n
> >
> >>> Change the code to only check virt_entry against the virtual address
> >>> range if it was set upon entry to the function.
> >>
> >> Not checking at all seems wrong to me. The ELF spec anyway says
> >> "virtual address", so an out of bounds value is at least suspicious.
> >>
> >>> Maybe the overwriting of virt_entry could be removed, but I don't know
> >>> if there would be unintended consequences where (old?) kernels don't
> >>> have an elfnote, but do have an in-range e_entry?  The failing kernel I
> >>> just looked at has an e_entry of 0x1000000.
> >>
> >> And if you dropped the overwriting, what entry point would we use
> >> in the absence of an ELF note?
> >
> > elf_xen_note_check currently has:
> >
> >     /* PVH only requires one ELF note to be set */
> >     if ( parms->phys_entry != UNSET_ADDR32 )
> >     {
> >         elf_msg(elf, "ELF: Found PVH image\n");
> >         return 0;
> >     }
> >
> >> I'd rather put up the option of adjusting the entry (or the check),
> >> if it looks like a valid physical address.
> >
> > The function doesn't know if the image will be booted PV or PVH, so I
> > guess we do all the checks, but use 'parms->phys_entry != UNSET_ADDR32
> > && parms->virt_entry == UNSET_ADDR' to conditionally skip checking
> > virt?
>
> Like Jürgen, the purpose of the patch hadn't become clear to me
> from reading the description. As I understand it now, we're currently
> refusing to boot such a kernel for no reason. If that's correct,
> perhaps you could say so in the description in a more direct way?

Yes, sorry I didn't state it clearly.  You are correct, libxc fails
with "xc_dom_find_loader: no loader found" for a linux kernel with
PHYS32_ENTRY but without ENTRY.

> As far as actual code adjustments go - how much of
> elf_xen_addr_calc_check() is actually applicable when booting PVH?

I don't know...

> And why is there no bounds check of ->phys_entry paralleling the
> ->virt_entry one?

What is the purpose of this checking?  It's sanity checking which is
generally good, but what is the harm from failing the checks?  A
corrupt kernel can crash itself?  Maybe you could start executing
something (the initramfs?) instead of the actual kernel?

> On the whole, as long as we don't know what mode we're planning to
> boot in, we can't skip any checks, as the mere presence of
> XEN_ELFNOTE_PHYS32_ENTRY doesn't mean that's also what gets used.
> Therefore simply bypassing any of the checks is not an option.

elf_xen_note_check() early exits when it finds phys_entry set, so
there is already some bypassing.

> In
> particular what you suggest would lead to failure to check
> e_entry-derived ->virt_entry when the PVH-specific note is
> present but we're booting in PV mode. For now I don't see how to
> address this without making the function aware of the intended
> booting mode.

Yes, the relevant checks depend on the desired booting mode.

The e_entry use seems a little problematic.  You said the ELF
Specification states it should be a virtual address, but Linux seems
to fill it with a physical address.  You could use a heuristic e_entry
< 0 (0xffff...) to compare with the virtual addresses otherwise check
against physical?

Regards,
Jason

Re: [PATCH] libelf: Handle PVH kernels lacking ENTRY elfnote

Posted by Jan Beulich 2 weeks ago
On 15.10.2020 16:50, Jason Andryuk wrote:
> On Thu, Oct 15, 2020 at 3:00 AM Jan Beulich <jbeulich@suse.com> wrote:
>> And why is there no bounds check of ->phys_entry paralleling the
>> ->virt_entry one?
> 
> What is the purpose of this checking?  It's sanity checking which is
> generally good, but what is the harm from failing the checks?  A
> corrupt kernel can crash itself?  Maybe you could start executing
> something (the initramfs?) instead of the actual kernel?

This is at least getting close to a possible security issue.
Booting a hacked up binary can be a problem afaik.

>> On the whole, as long as we don't know what mode we're planning to
>> boot in, we can't skip any checks, as the mere presence of
>> XEN_ELFNOTE_PHYS32_ENTRY doesn't mean that's also what gets used.
>> Therefore simply bypassing any of the checks is not an option.
> 
> elf_xen_note_check() early exits when it finds phys_entry set, so
> there is already some bypassing.
> 
>> In
>> particular what you suggest would lead to failure to check
>> e_entry-derived ->virt_entry when the PVH-specific note is
>> present but we're booting in PV mode. For now I don't see how to
>> address this without making the function aware of the intended
>> booting mode.
> 
> Yes, the relevant checks depend on the desired booting mode.
> 
> The e_entry use seems a little problematic.  You said the ELF
> Specification states it should be a virtual address, but Linux seems
> to fill it with a physical address.  You could use a heuristic e_entry
> < 0 (0xffff...) to compare with the virtual addresses otherwise check
> against physical?

Don't we have a physical range as well? And don't we adjust the
entry point already in certain cases anyway? Checking and adjustment
can (and should) be brought in sync, and else checking the entry
point fits at least one of the two ranges may be better than no
checking at all, I think.

Jan

Re: [PATCH] libelf: Handle PVH kernels lacking ENTRY elfnote

Posted by Jason Andryuk 1 week ago
On Thu, Oct 15, 2020 at 11:14 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 15.10.2020 16:50, Jason Andryuk wrote:
> > On Thu, Oct 15, 2020 at 3:00 AM Jan Beulich <jbeulich@suse.com> wrote:
> >> And why is there no bounds check of ->phys_entry paralleling the
> >> ->virt_entry one?
> >
> > What is the purpose of this checking?  It's sanity checking which is
> > generally good, but what is the harm from failing the checks?  A
> > corrupt kernel can crash itself?  Maybe you could start executing
> > something (the initramfs?) instead of the actual kernel?
>
> This is at least getting close to a possible security issue.
> Booting a hacked up binary can be a problem afaik.

If you are already letting the user provide a kernel, they can give a
well formed kernel that does whatever they want.  Like Andrew wrote,
the concern would be if the binary can subvert the hypervisor/tools.

> >> On the whole, as long as we don't know what mode we're planning to
> >> boot in, we can't skip any checks, as the mere presence of
> >> XEN_ELFNOTE_PHYS32_ENTRY doesn't mean that's also what gets used.
> >> Therefore simply bypassing any of the checks is not an option.
> >
> > elf_xen_note_check() early exits when it finds phys_entry set, so
> > there is already some bypassing.
> >
> >> In
> >> particular what you suggest would lead to failure to check
> >> e_entry-derived ->virt_entry when the PVH-specific note is
> >> present but we're booting in PV mode. For now I don't see how to
> >> address this without making the function aware of the intended
> >> booting mode.
> >
> > Yes, the relevant checks depend on the desired booting mode.
> >
> > The e_entry use seems a little problematic.  You said the ELF
> > Specification states it should be a virtual address, but Linux seems
> > to fill it with a physical address.  You could use a heuristic e_entry
> > < 0 (0xffff...) to compare with the virtual addresses otherwise check
> > against physical?
>
> Don't we have a physical range as well? And don't we adjust the
> entry point already in certain cases anyway? Checking and adjustment
> can (and should) be brought in sync, and else checking the entry
> point fits at least one of the two ranges may be better than no
> checking at all, I think.

Looks like we can pass XC_DOM_PV_CONTAINER/XC_DOM_HVM_CONTAINER down
into elf_xen_parse().  Then we would just validate phys_entry for HVM
and virt_entry for PV.  Does that sound reasonable?

(The use in xc_dom_probe_hvm_kernel() is interesting to disallow
Xen-enabled kernel.)

Regards,
Jason

Re: [PATCH] libelf: Handle PVH kernels lacking ENTRY elfnote

Posted by Jan Beulich 1 week ago
On 16.10.2020 18:28, Jason Andryuk wrote:
> Looks like we can pass XC_DOM_PV_CONTAINER/XC_DOM_HVM_CONTAINER down
> into elf_xen_parse().  Then we would just validate phys_entry for HVM
> and virt_entry for PV.  Does that sound reasonable?

I think so, yes. Assuming of course that you'll convert the XC_DOM_*
into a boolean, so that the hypervisor's use of libelf/ can also be
suitably adjusted.

Jan

Re: [PATCH] libelf: Handle PVH kernels lacking ENTRY elfnote

Posted by Jason Andryuk 1 week ago
On Mon, Oct 19, 2020 at 3:38 AM Jan Beulich <jbeulich@suse.com> wrote:
>
> On 16.10.2020 18:28, Jason Andryuk wrote:
> > Looks like we can pass XC_DOM_PV_CONTAINER/XC_DOM_HVM_CONTAINER down
> > into elf_xen_parse().  Then we would just validate phys_entry for HVM
> > and virt_entry for PV.  Does that sound reasonable?
>
> I think so, yes. Assuming of course that you'll convert the XC_DOM_*
> into a boolean, so that the hypervisor's use of libelf/ can also be
> suitably adjusted.

Are you okay with:
-int elf_xen_parse(struct elf_binary *elf,
-                  struct elf_dom_parms *parms);
+int elf_xen_parse_pvh(struct elf_binary *elf,
+                      struct elf_dom_parms *parms);
+int elf_xen_parse_pv(struct elf_binary *elf,
+                     struct elf_dom_parms *parms);
?

I prefer avoiding boolean arguments since I find it helps readability.
The xen dom0 builders can just call their variant, but the xenguest
elfloader and hvmloader call the appropriate one based on the
container_type.

Regards,
Jason

Re: [PATCH] libelf: Handle PVH kernels lacking ENTRY elfnote

Posted by Jan Beulich 1 week ago
On 19.10.2020 17:26, Jason Andryuk wrote:
> On Mon, Oct 19, 2020 at 3:38 AM Jan Beulich <jbeulich@suse.com> wrote:
>> On 16.10.2020 18:28, Jason Andryuk wrote:
>>> Looks like we can pass XC_DOM_PV_CONTAINER/XC_DOM_HVM_CONTAINER down
>>> into elf_xen_parse().  Then we would just validate phys_entry for HVM
>>> and virt_entry for PV.  Does that sound reasonable?
>>
>> I think so, yes. Assuming of course that you'll convert the XC_DOM_*
>> into a boolean, so that the hypervisor's use of libelf/ can also be
>> suitably adjusted.
> 
> Are you okay with:
> -int elf_xen_parse(struct elf_binary *elf,
> -                  struct elf_dom_parms *parms);
> +int elf_xen_parse_pvh(struct elf_binary *elf,
> +                      struct elf_dom_parms *parms);
> +int elf_xen_parse_pv(struct elf_binary *elf,
> +                     struct elf_dom_parms *parms);
> ?
> 
> I prefer avoiding boolean arguments since I find it helps readability.

And I view things the other way around. If readability is of
concern, how about adding an unsigned int flags parameter
and #define-ing two suitable constants to be passed?

And of course it's not me alone who needs to be okay with
whatever route you/we go.

Jan

Re: [PATCH] libelf: Handle PVH kernels lacking ENTRY elfnote

Posted by Andrew Cooper 2 weeks ago
On 15/10/2020 16:14, Jan Beulich wrote:
> On 15.10.2020 16:50, Jason Andryuk wrote:
>> On Thu, Oct 15, 2020 at 3:00 AM Jan Beulich <jbeulich@suse.com> wrote:
>>> And why is there no bounds check of ->phys_entry paralleling the
>>> ->virt_entry one?
>> What is the purpose of this checking?  It's sanity checking which is
>> generally good, but what is the harm from failing the checks?  A
>> corrupt kernel can crash itself?  Maybe you could start executing
>> something (the initramfs?) instead of the actual kernel?
> This is at least getting close to a possible security issue.
> Booting a hacked up binary can be a problem afaik.

It's only a security issue if the absence of the check is going to cause
a malfunction outside of guest the guest context.  (e.g. in the
toolstack's elf parser)

There are a functionally infinite ways for a guest kernel to crash
itself early on boot - malforming the ELF header such that the state of
the guest once executing doesn't boot isn't interesting from this point
of view.

~Andrew

Re: [PATCH] libelf: Handle PVH kernels lacking ENTRY elfnote

Posted by Wei Liu 2 weeks ago
On Wed, Oct 14, 2020 at 11:31:50AM -0400, Jason Andryuk wrote:
> Linux kernels only have an ENTRY elfnote when built with CONFIG_PV.  A
> kernel build CONFIG_PVH=y CONFIG_PV=n lacks the note.  In this case,
> virt_entry will be UNSET_ADDR, overwritten by the ELF header e_entry,
> and fail the check against the virt address range.
> 
> Change the code to only check virt_entry against the virtual address
> range if it was set upon entry to the function.
> 
> Signed-off-by: Jason Andryuk <jandryuk@gmail.com>
> 
> ---
> Maybe the overwriting of virt_entry could be removed, but I don't know
> if there would be unintended consequences where (old?) kernels don't
> have an elfnote, but do have an in-range e_entry?  The failing kernel I
> just looked at has an e_entry of 0x1000000.
> 
> Oh, it looks like Mini-OS doesn't set the entry ELFNOTE and relies on
> e_entry (of 0) to pass these checks.
> 

I have not looked into the patch, but please don't use mini-os as a source
for truth. ;-)

It is more likely than not we should fix mini-os instead of Xen and
Linux.

Wei.