| Message ID | 20171011120310.26383-1-gregory.herrero@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 11.10.17 at 14:03, <gregory.herrero@oracle.com> wrote: > When filling __xen_guest section of a guest, user may define > HYPERCALL_PAGE earlier than VIRT_BASE in the section leading to an > incorrect hypercall page address since an undefined virt_base could be > used to compute hypercall page address. > If there is no VIRT_BASE entry in __xen_guest section, default value of > 0 is used for virt_base. Thus, setting hypercall page address to > HYPERCALL_PAGE value is correct in this case too. Do you have an example of a guest kernel that is doing this? I ask because the __xen_guest section has been deprecated for many years - everyone should be using the notes section instead nowadays. > --- a/xen/common/libelf/libelf-dominfo.c > +++ b/xen/common/libelf/libelf-dominfo.c > @@ -330,14 +330,21 @@ elf_errorstatus elf_xen_parse_guest_info(struct elf_binary *elf, > > /* longs */ > if ( !strcmp(name, "VIRT_BASE") ) > + { > parms->virt_base = strtoull(value, NULL, 0); > + if ( parms->virt_hypercall != UNSET_ADDR ) > + params->virt_hypercall += params->virt_base; > + } > if ( !strcmp(name, "VIRT_ENTRY") ) > parms->virt_entry = strtoull(value, NULL, 0); > if ( !strcmp(name, "ELF_PADDR_OFFSET") ) > parms->elf_paddr_offset = strtoull(value, NULL, 0); > if ( !strcmp(name, "HYPERCALL_PAGE") ) > - parms->virt_hypercall = (strtoull(value, NULL, 0) << 12) + > - parms->virt_base; > + { > + parms->virt_hypercall = (strtoull(value, NULL, 0) << 12); > + if ( parms->virt_base != UNSET_ADDR ) > + params->virt_hypercall += params->virt_base; > + } I think you rather want to do this once after the loop. That'll then also take care of there being multiple VIRT_BASE entries. Jan
On Wed, Oct 11, 2017 at 06:40:14AM -0600, Jan Beulich wrote: > >>> On 11.10.17 at 14:03, <gregory.herrero@oracle.com> wrote: > > When filling __xen_guest section of a guest, user may define > > HYPERCALL_PAGE earlier than VIRT_BASE in the section leading to an > > incorrect hypercall page address since an undefined virt_base could be > > used to compute hypercall page address. > > If there is no VIRT_BASE entry in __xen_guest section, default value of > > 0 is used for virt_base. Thus, setting hypercall page address to > > HYPERCALL_PAGE value is correct in this case too. > > Do you have an example of a guest kernel that is doing this? I > ask because the __xen_guest section has been deprecated for > many years - everyone should be using the notes section > instead nowadays. > There is no public example AFAIK, I faced this issue while creating a guest locally for testing purpose. > > --- a/xen/common/libelf/libelf-dominfo.c > > +++ b/xen/common/libelf/libelf-dominfo.c > > @@ -330,14 +330,21 @@ elf_errorstatus elf_xen_parse_guest_info(struct elf_binary *elf, > > > > /* longs */ > > if ( !strcmp(name, "VIRT_BASE") ) > > + { > > parms->virt_base = strtoull(value, NULL, 0); > > + if ( parms->virt_hypercall != UNSET_ADDR ) > > + params->virt_hypercall += params->virt_base; > > + } > > if ( !strcmp(name, "VIRT_ENTRY") ) > > parms->virt_entry = strtoull(value, NULL, 0); > > if ( !strcmp(name, "ELF_PADDR_OFFSET") ) > > parms->elf_paddr_offset = strtoull(value, NULL, 0); > > if ( !strcmp(name, "HYPERCALL_PAGE") ) > > - parms->virt_hypercall = (strtoull(value, NULL, 0) << 12) + > > - parms->virt_base; > > + { > > + parms->virt_hypercall = (strtoull(value, NULL, 0) << 12); > > + if ( parms->virt_base != UNSET_ADDR ) > > + params->virt_hypercall += params->virt_base; > > + } > > I think you rather want to do this once after the loop. That'll > then also take care of there being multiple VIRT_BASE entries. > I will send an updated patch with your suggestion. Feel free to ignore it if usage of __xen_guest section is deprecated. Thanks, Gregory
diff --git a/xen/common/libelf/libelf-dominfo.c b/xen/common/libelf/libelf-dominfo.c index a52900c00cd..b1255acc059 100644 --- a/xen/common/libelf/libelf-dominfo.c +++ b/xen/common/libelf/libelf-dominfo.c @@ -330,14 +330,21 @@ elf_errorstatus elf_xen_parse_guest_info(struct elf_binary *elf, /* longs */ if ( !strcmp(name, "VIRT_BASE") ) + { parms->virt_base = strtoull(value, NULL, 0); + if ( parms->virt_hypercall != UNSET_ADDR ) + params->virt_hypercall += params->virt_base; + } if ( !strcmp(name, "VIRT_ENTRY") ) parms->virt_entry = strtoull(value, NULL, 0); if ( !strcmp(name, "ELF_PADDR_OFFSET") ) parms->elf_paddr_offset = strtoull(value, NULL, 0); if ( !strcmp(name, "HYPERCALL_PAGE") ) - parms->virt_hypercall = (strtoull(value, NULL, 0) << 12) + - parms->virt_base; + { + parms->virt_hypercall = (strtoull(value, NULL, 0) << 12); + if ( parms->virt_base != UNSET_ADDR ) + params->virt_hypercall += params->virt_base; + } /* other */ if ( !strcmp(name, "FEATURES") )