Re: [Xen-devel] [PATCH v4 11/14] xen/x86: parse Dom0 kernel for PVHv2

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 30.11.16 at 17:49, <roger.pau@xxxxxxxxxx> wrote:
> @@ -1930,12 +1931,148 @@ static int __init hvm_setup_p2m(struct domain *d)
>  #undef MB1_PAGES
>  }
>  
> +static int __init hvm_copy_to_phys(struct domain *d, paddr_t paddr, void 
> *buf,
> +                                   int size)

I guess you made size plain int because hvm_copy_to_guest_phys()
has it that way, but please let's not spread such bogus things - sizes
can't possibly be negative.

> +{
> +    struct vcpu *saved_current;
> +    int rc;
> +
> +    saved_current = current;
> +    set_current(d->vcpu[0]);
> +    rc = hvm_copy_to_guest_phys(paddr, buf, size);
> +    set_current(saved_current);

I continue to be uncertain about the behavior of this if something
inside hvm_copy_to_guest_phys() goes wrong: Did you either
statically analyze the code or try in practice out whether the
playing with current makes understanding the crash output any
harder?

While there's going to be some work involved with it, I do think
that the use here might be a reason for the whole hvm_copy()
machinery to gain a struct vcpu* parameter.

> +static int __init hvm_load_kernel(struct domain *d, const module_t *image,
> +                                  unsigned long image_headroom,
> +                                  module_t *initrd, char *image_base,
> +                                  char *cmdline, paddr_t *entry,
> +                                  paddr_t *start_info_addr)
> +{
> +    char *image_start = image_base + image_headroom;
> +    unsigned long image_len = image->mod_end;
> +    struct elf_binary elf;
> +    struct elf_dom_parms parms;
> +    paddr_t last_addr;
> +    struct hvm_start_info start_info;
> +    struct hvm_modlist_entry mod;
> +    struct vcpu *saved_current, *v = d->vcpu[0];
> +    int rc;
> +
> +    if ( (rc = bzimage_parse(image_base, &image_start, &image_len)) != 0 )
> +    {
> +        printk("Error trying to detect bz compressed kernel\n");
> +        return rc;
> +    }
> +
> +    if ( (rc = elf_init(&elf, image_start, image_len)) != 0 )
> +    {
> +        printk("Unable to init ELF\n");
> +        return rc;
> +    }
> +#ifdef VERBOSE
> +    elf_set_verbose(&elf);
> +#endif
> +    elf_parse_binary(&elf);
> +    if ( (rc = elf_xen_parse(&elf, &parms)) != 0 )
> +    {
> +        printk("Unable to parse kernel for ELFNOTES\n");
> +        return rc;
> +    }
> +
> +    if ( parms.phys_entry == UNSET_ADDR32 ) {
> +        printk("Unable to find XEN_ELFNOTE_PHYS32_ENTRY address\n");
> +        return -EINVAL;
> +    }
> +
> +    printk("OS: %s version: %s loader: %s bitness: %s\n", parms.guest_os,
> +           parms.guest_ver, parms.loader,
> +           elf_64bit(&elf) ? "64-bit" : "32-bit");
> +
> +    /* Copy the OS image and free temporary buffer. */
> +    elf.dest_base = (void *)(parms.virt_kstart - parms.virt_base);
> +    elf.dest_size = parms.virt_kend - parms.virt_kstart;
> +
> +    saved_current = current;
> +    set_current(v);
> +    rc = elf_load_binary(&elf);
> +    set_current(saved_current);

Same reservations as above.

> +    if ( rc < 0 )
> +    {
> +        printk("Failed to load kernel: %d\n", rc);
> +        printk("Xen dom0 kernel broken ELF: %s\n", elf_check_broken(&elf));
> +        return rc;
> +    }
> +
> +    last_addr = ROUNDUP(parms.virt_kend - parms.virt_base, PAGE_SIZE);
> +
> +    if ( initrd != NULL )
> +    {
> +        rc = hvm_copy_to_phys(d, last_addr, mfn_to_virt(initrd->mod_start),
> +                              initrd->mod_end);
> +        if ( rc )
> +        {
> +            printk("Unable to copy initrd to guest\n");
> +            return rc;
> +        }
> +
> +        mod.paddr = last_addr;
> +        mod.size = initrd->mod_end;
> +        last_addr += ROUNDUP(initrd->mod_end, PAGE_SIZE);
> +    }

mod is left uninitialized in the else case afaict - I don't think all
compilers we support (plus Coverity) can spot the common
dependency on initrd != NULL.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel