| Message ID | 20160622171545.5304-9-anthony.perard@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 22.06.16 at 19:15, <anthony.perard@citrix.com> wrote: > --- a/tools/firmware/hvmloader/hvmloader.c > +++ b/tools/firmware/hvmloader/hvmloader.c > @@ -253,10 +253,51 @@ static void acpi_enable_sci(void) > BUG_ON(!(pm1a_cnt_val & ACPI_PM1C_SCI_EN)); > } > > +const struct hvm_modlist_entry *get_module_entry( > + const struct hvm_start_info *info, > + const char *name) > +{ > + const struct hvm_modlist_entry *modlist = > + (struct hvm_modlist_entry *)(uint32_t)info->modlist_paddr; > + unsigned int i; > + > + if ( !modlist || info->modlist_paddr > UINT_MAX) > + return NULL; How about info->modlist_paddr + info->nr_modules * sizeof()? You check for overflow below, but not here. I think you should either consistently rely on there being something right below 4Gb which makes this impossible (and then say so in a comment), or do full checks everywhere. > + for ( i = 0; i < info->nr_modules; i++ ) > + { > + uint32_t module_name = modlist[i].cmdline_paddr; > + > + /* Skip if the module or its cmdline is missing. */ > + if ( !module_name || !modlist[i].paddr ) > + continue; > + > + /* Skip if the cmdline can not be read. */ > + if ( modlist[i].cmdline_paddr > UINT_MAX ) > + continue; Similarly here. > + if ( !strcmp(name, (char*)module_name) ) Stray cast. > + { > + if ( modlist[i].paddr > UINT_MAX || modlist[i].size > UINT_MAX || > + (modlist[i].paddr + modlist[i].size) > UINT_MAX ) I think the last one could be >=. Jan
On Fri, Jun 24, 2016 at 01:33:45AM -0600, Jan Beulich wrote: > >>> On 22.06.16 at 19:15, <anthony.perard@citrix.com> wrote: > > --- a/tools/firmware/hvmloader/hvmloader.c > > +++ b/tools/firmware/hvmloader/hvmloader.c > > @@ -253,10 +253,51 @@ static void acpi_enable_sci(void) > > BUG_ON(!(pm1a_cnt_val & ACPI_PM1C_SCI_EN)); > > } > > > > +const struct hvm_modlist_entry *get_module_entry( > > + const struct hvm_start_info *info, > > + const char *name) > > +{ > > + const struct hvm_modlist_entry *modlist = > > + (struct hvm_modlist_entry *)(uint32_t)info->modlist_paddr; > > + unsigned int i; > > + > > + if ( !modlist || info->modlist_paddr > UINT_MAX) > > + return NULL; > > How about info->modlist_paddr + info->nr_modules * sizeof()? > You check for overflow below, but not here. I think you should > either consistently rely on there being something right below 4Gb > which makes this impossible (and then say so in a comment), or > do full checks everywhere. I'll do the full checks. > > + for ( i = 0; i < info->nr_modules; i++ ) > > + { > > + uint32_t module_name = modlist[i].cmdline_paddr; > > + > > + /* Skip if the module or its cmdline is missing. */ > > + if ( !module_name || !modlist[i].paddr ) > > + continue; > > + > > + /* Skip if the cmdline can not be read. */ > > + if ( modlist[i].cmdline_paddr > UINT_MAX ) > > + continue; > > Similarly here. Here, I don't know the size of the cmdline and I don't think calling an extra strlen() would be usefull. I think that the strcmp() below is going to be enough for the top bondary check. Or I could use the size of name. > > + if ( !strcmp(name, (char*)module_name) ) > > Stray cast. Yes. I'll change the type of module_name and remove the cast here. > > + { > > + if ( modlist[i].paddr > UINT_MAX || modlist[i].size > UINT_MAX || > > + (modlist[i].paddr + modlist[i].size) > UINT_MAX ) > > I think the last one could be >=. I think it's valid if addr+size == UINT_MAX. That would means the last byte of the module would be at 0xFFFFFFFE.
>>> On 24.06.16 at 19:02, <anthony.perard@citrix.com> wrote: > On Fri, Jun 24, 2016 at 01:33:45AM -0600, Jan Beulich wrote: >> >>> On 22.06.16 at 19:15, <anthony.perard@citrix.com> wrote: >> > --- a/tools/firmware/hvmloader/hvmloader.c >> > +++ b/tools/firmware/hvmloader/hvmloader.c >> > @@ -253,10 +253,51 @@ static void acpi_enable_sci(void) >> > BUG_ON(!(pm1a_cnt_val & ACPI_PM1C_SCI_EN)); >> > } >> > >> > +const struct hvm_modlist_entry *get_module_entry( >> > + const struct hvm_start_info *info, >> > + const char *name) >> > +{ >> > + const struct hvm_modlist_entry *modlist = >> > + (struct hvm_modlist_entry *)(uint32_t)info->modlist_paddr; >> > + unsigned int i; >> > + >> > + if ( !modlist || info->modlist_paddr > UINT_MAX) >> > + return NULL; >> >> How about info->modlist_paddr + info->nr_modules * sizeof()? >> You check for overflow below, but not here. I think you should >> either consistently rely on there being something right below 4Gb >> which makes this impossible (and then say so in a comment), or >> do full checks everywhere. > > I'll do the full checks. > >> > + for ( i = 0; i < info->nr_modules; i++ ) >> > + { >> > + uint32_t module_name = modlist[i].cmdline_paddr; >> > + >> > + /* Skip if the module or its cmdline is missing. */ >> > + if ( !module_name || !modlist[i].paddr ) >> > + continue; >> > + >> > + /* Skip if the cmdline can not be read. */ >> > + if ( modlist[i].cmdline_paddr > UINT_MAX ) >> > + continue; >> >> Similarly here. > > Here, I don't know the size of the cmdline and I don't think calling an > extra strlen() would be usefull. I think that the strcmp() below is going to > be enough for the top bondary check. No - once you reach the 4Gb boundary, the compare would continue at address zero. That's not what you want. > Or I could use the size of name. Size of name? >> > + { >> > + if ( modlist[i].paddr > UINT_MAX || modlist[i].size > UINT_MAX || >> > + (modlist[i].paddr + modlist[i].size) > UINT_MAX ) >> >> I think the last one could be >=. > > I think it's valid if addr+size == UINT_MAX. That would means the last > byte of the module would be at 0xFFFFFFFE. It's even valid when addr+size == UINT_MAX+1 (without value wrapping of course), as the last valid byte (i.e. the last one hvmloader can address easily) is at 0xffffffff. Jan
On Mon, Jun 27, 2016 at 01:13:43AM -0600, Jan Beulich wrote: > >>> On 24.06.16 at 19:02, <anthony.perard@citrix.com> wrote: > > On Fri, Jun 24, 2016 at 01:33:45AM -0600, Jan Beulich wrote: > >> >>> On 22.06.16 at 19:15, <anthony.perard@citrix.com> wrote: > >> > --- a/tools/firmware/hvmloader/hvmloader.c > >> > +++ b/tools/firmware/hvmloader/hvmloader.c > >> > @@ -253,10 +253,51 @@ static void acpi_enable_sci(void) > >> > BUG_ON(!(pm1a_cnt_val & ACPI_PM1C_SCI_EN)); > >> > } > >> > > >> > +const struct hvm_modlist_entry *get_module_entry( > >> > + const struct hvm_start_info *info, > >> > + const char *name) > >> > +{ > >> > + const struct hvm_modlist_entry *modlist = > >> > + (struct hvm_modlist_entry *)(uint32_t)info->modlist_paddr; > >> > + unsigned int i; > >> > + > >> > + if ( !modlist || info->modlist_paddr > UINT_MAX) > >> > + return NULL; > >> > >> How about info->modlist_paddr + info->nr_modules * sizeof()? > >> You check for overflow below, but not here. I think you should > >> either consistently rely on there being something right below 4Gb > >> which makes this impossible (and then say so in a comment), or > >> do full checks everywhere. > > > > I'll do the full checks. > > > >> > + for ( i = 0; i < info->nr_modules; i++ ) > >> > + { > >> > + uint32_t module_name = modlist[i].cmdline_paddr; > >> > + > >> > + /* Skip if the module or its cmdline is missing. */ > >> > + if ( !module_name || !modlist[i].paddr ) > >> > + continue; > >> > + > >> > + /* Skip if the cmdline can not be read. */ > >> > + if ( modlist[i].cmdline_paddr > UINT_MAX ) > >> > + continue; > >> > >> Similarly here. > > > > Here, I don't know the size of the cmdline and I don't think calling an > > extra strlen() would be usefull. I think that the strcmp() below is going to > > be enough for the top bondary check. > > No - once you reach the 4Gb boundary, the compare would continue > at address zero. That's not what you want. > > Or I could use the size of name. > > Size of name? The function get_module_entry() takes an argument called "name", I think I was proposing to use that, strlen(name). So, I'm going to add this condition: (cmdline_paddr + strlen(name) > UINTPTR_MAX) name is the string we are going to compare cmdline to. I think that will be enough to do a full check of the module cmdline. > >> > + { > >> > + if ( modlist[i].paddr > UINT_MAX || modlist[i].size > UINT_MAX || > >> > + (modlist[i].paddr + modlist[i].size) > UINT_MAX ) > >> > >> I think the last one could be >=. > > > > I think it's valid if addr+size == UINT_MAX. That would means the last > > byte of the module would be at 0xFFFFFFFE. > > It's even valid when addr+size == UINT_MAX+1 (without value > wrapping of course), as the last valid byte (i.e. the last one > hvmloader can address easily) is at 0xffffffff. I'll do (addr+size-1 > UINTPTR_MAX) which should return true when the module cross the 4GB boundary.
>>> On 30.06.16 at 17:04, <anthony.perard@citrix.com> wrote: > On Mon, Jun 27, 2016 at 01:13:43AM -0600, Jan Beulich wrote: >> >>> On 24.06.16 at 19:02, <anthony.perard@citrix.com> wrote: >> > On Fri, Jun 24, 2016 at 01:33:45AM -0600, Jan Beulich wrote: >> >> >>> On 22.06.16 at 19:15, <anthony.perard@citrix.com> wrote: >> >> > + for ( i = 0; i < info->nr_modules; i++ ) >> >> > + { >> >> > + uint32_t module_name = modlist[i].cmdline_paddr; >> >> > + >> >> > + /* Skip if the module or its cmdline is missing. */ >> >> > + if ( !module_name || !modlist[i].paddr ) >> >> > + continue; >> >> > + >> >> > + /* Skip if the cmdline can not be read. */ >> >> > + if ( modlist[i].cmdline_paddr > UINT_MAX ) >> >> > + continue; >> >> >> >> Similarly here. >> > >> > Here, I don't know the size of the cmdline and I don't think calling an >> > extra strlen() would be usefull. I think that the strcmp() below is going to >> > be enough for the top bondary check. >> >> No - once you reach the 4Gb boundary, the compare would continue >> at address zero. That's not what you want. >> > Or I could use the size of name. >> >> Size of name? > > The function get_module_entry() takes an argument called "name", I think > I was proposing to use that, strlen(name). > > So, I'm going to add this condition: > (cmdline_paddr + strlen(name) > UINTPTR_MAX) > name is the string we are going to compare cmdline to. I think that > will be enough to do a full check of the module cmdline. Makes sense. Jan
diff --git a/tools/firmware/hvmloader/config.h b/tools/firmware/hvmloader/config.h index b838cf9..4c6d8ad 100644 --- a/tools/firmware/hvmloader/config.h +++ b/tools/firmware/hvmloader/config.h @@ -22,7 +22,7 @@ struct bios_config { /* ROMS */ void (*load_roms)(void); - void (*bios_load)(const struct bios_config *config); + void (*bios_load)(const struct bios_config *config, void *addr, uint32_t size); void (*bios_info_setup)(void); void (*bios_info_finish)(void); diff --git a/tools/firmware/hvmloader/hvmloader.c b/tools/firmware/hvmloader/hvmloader.c index c45f367..7fdc847 100644 --- a/tools/firmware/hvmloader/hvmloader.c +++ b/tools/firmware/hvmloader/hvmloader.c @@ -253,10 +253,51 @@ static void acpi_enable_sci(void) BUG_ON(!(pm1a_cnt_val & ACPI_PM1C_SCI_EN)); } +const struct hvm_modlist_entry *get_module_entry( + const struct hvm_start_info *info, + const char *name) +{ + const struct hvm_modlist_entry *modlist = + (struct hvm_modlist_entry *)(uint32_t)info->modlist_paddr; + unsigned int i; + + if ( !modlist || info->modlist_paddr > UINT_MAX) + return NULL; + + for ( i = 0; i < info->nr_modules; i++ ) + { + uint32_t module_name = modlist[i].cmdline_paddr; + + /* Skip if the module or its cmdline is missing. */ + if ( !module_name || !modlist[i].paddr ) + continue; + + /* Skip if the cmdline can not be read. */ + if ( modlist[i].cmdline_paddr > UINT_MAX ) + continue; + + if ( !strcmp(name, (char*)module_name) ) + { + if ( modlist[i].paddr > UINT_MAX || modlist[i].size > UINT_MAX || + (modlist[i].paddr + modlist[i].size) > UINT_MAX ) + { + printf("Can not load \"%s\" from 0x"PRIllx" (0x"PRIllx")\n", + name, PRIllx_arg(modlist[i].paddr), + PRIllx_arg(modlist[i].size)); + BUG(); + } + return &modlist[i]; + } + } + + return NULL; +} + int main(void) { const struct bios_config *bios; int acpi_enabled; + const struct hvm_modlist_entry *bios_module; /* Initialise hypercall stubs with RET, rendering them no-ops. */ memset((void *)HYPERCALL_PHYSICAL_ADDRESS, 0xc3 /* RET */, PAGE_SIZE); @@ -292,8 +333,17 @@ int main(void) } printf("Loading %s ...\n", bios->name); - if ( bios->bios_load ) - bios->bios_load(bios); + bios_module = get_module_entry(hvm_start_info, "firmware"); + if ( bios_module && bios->bios_load ) + { + uint32_t paddr = bios_module->paddr; + + bios->bios_load(bios, (void*)paddr, bios_module->size); + } + else if ( bios->bios_load ) + { + bios->bios_load(bios, NULL, 0); + } else { BUG_ON(bios->bios_address + bios->image_size > diff --git a/tools/firmware/hvmloader/ovmf.c b/tools/firmware/hvmloader/ovmf.c index db9fa7a..858a2d4 100644 --- a/tools/firmware/hvmloader/ovmf.c +++ b/tools/firmware/hvmloader/ovmf.c @@ -93,7 +93,8 @@ static void ovmf_finish_bios_info(void) info->checksum = -checksum; } -static void ovmf_load(const struct bios_config *config) +static void ovmf_load(const struct bios_config *config, + void *bios_addr, uint32_t bios_length) { xen_pfn_t mfn; uint64_t addr = OVMF_BEGIN; diff --git a/tools/firmware/hvmloader/rombios.c b/tools/firmware/hvmloader/rombios.c index 1f15b94..2ded844 100644 --- a/tools/firmware/hvmloader/rombios.c +++ b/tools/firmware/hvmloader/rombios.c @@ -121,7 +121,8 @@ static void rombios_load_roms(void) option_rom_phys_addr + option_rom_sz - 1); } -static void rombios_load(const struct bios_config *config) +static void rombios_load(const struct bios_config *config, + void *unused_addr, uint32_t unused_size) { uint32_t bioshigh; struct rombios_info *info; diff --git a/tools/firmware/hvmloader/util.h b/tools/firmware/hvmloader/util.h index 9808016..003dc71 100644 --- a/tools/firmware/hvmloader/util.h +++ b/tools/firmware/hvmloader/util.h @@ -34,6 +34,8 @@ enum { #undef NULL #define NULL ((void*)0) +#define UINT_MAX (~0U) + void __assert_failed(char *assertion, char *file, int line) __attribute__((noreturn)); #define ASSERT(p) \
The BIOS blob can be found an entry called "firmware" of the modlist of the hvm_start_info struct. The found BIOS blob is not loaded by this patch, but only passed as argument to bios_load() function. Signed-off-by: Anthony PERARD <anthony.perard@citrix.com> --- Changes in V5: - don't BUG() on module's paddr having value 0, and just skip. - fix some coding style - rename module name to "firmware" (was "bios") - less use of BUG_ON in get_module_entry() and skip entries instead. Only BUG() if the module which match name is not accessible. Changes in V4: - add more BUG_ON into get_module_entry(). Check that modules paddr and size are 32bits. Changes in V3: - fix some codying style - use module.cmdline to look for a module name instead of the main cmdline from hvm_start_info. --- tools/firmware/hvmloader/config.h | 2 +- tools/firmware/hvmloader/hvmloader.c | 54 ++++++++++++++++++++++++++++++++++-- tools/firmware/hvmloader/ovmf.c | 3 +- tools/firmware/hvmloader/rombios.c | 3 +- tools/firmware/hvmloader/util.h | 2 ++ 5 files changed, 59 insertions(+), 5 deletions(-)