Re: [Xen-devel] [PATCH] readnote: Add bzImage kernel support

[Ian Campbell <Ian.Campbell@xxxxxxxxxx>]

On Thu, 2012-04-26 at 04:00 +0100, Xuesen Guo wrote:
> Add the check of bzImage kernel and make it work
> with RHEL 6 bzipped kernels
> 
> Signed-off-by: Xuesen Guo <Xuesen.Guo@xxxxxxxxxxxxxxxxxxxxx>
> 
> diff -r d690c7e896a2 -r ec8e99606fbe tools/xcutils/readnotes.c
> --- a/tools/xcutils/readnotes.c       Thu Apr 05 11:06:03 2012 +0100
> +++ b/tools/xcutils/readnotes.c       Thu Apr 26 10:57:09 2012 +0800
> @@ -18,6 +18,48 @@
>  
>  static xc_interface *xch;
>  
> +/* According to the implemation of xc_dom_probe_bzimage_kernel() function */
> +/* We add support of bzImage kernel */
> +/* Copied from tools/libxc/xc_doom_bzImageloader.c */
> +struct setup_header {
> +     uint8_t  _pad0[0x1f1];  /* skip uninteresting stuff */
> +     uint8_t  setup_sects;
> +     uint16_t root_flags;
> +     uint32_t syssize;
> +     uint16_t ram_size;
> +     uint16_t vid_mode;
> +     uint16_t root_dev;
> +     uint16_t boot_flag;
> +     uint16_t jump;
> +     uint32_t header;
> +#define HDR_MAGIC  "HdrS"
> +#define HDR_MAGIC_SZ 4
> +     uint16_t version;
> +#define VERSION(h,l) (((h)<<8) | (l))
> +     uint32_t realmode_swtch;
> +     uint16_t start_sys;
> +     uint16_t kernel_version;
> +     uint8_t  type_of_loader;
> +     uint8_t  loadflags;
> +     uint16_t setup_move_size;
> +     uint32_t code32_start;
> +     uint32_t ramdisk_image;
> +     uint32_t ramdisk_size;
> +     uint32_t bootsect_kludge;
> +     uint16_t heap_end_ptr;
> +     uint16_t _pad1;
> +     uint32_t cmd_line_ptr;
> +     uint32_t initrd_addr_max;
> +     uint32_t kernel_alignment;
> +     uint8_t  relocatable_kernel;
> +     uint8_t  _pad2[3];
> +     uint32_t cmdline_size;
> +     uint32_t hardware_subarch;
> +     uint64_t hardware_subarch_data;
> +     uint32_t payload_offset;
> +     uint32_t payload_length;
> +} __attribute__((packed));
> +
>  static void print_string_note(const char *prefix, struct elf_binary *elf,
>                             const elf_note *note)
>  {
> @@ -131,6 +173,9 @@ int main(int argc, char **argv)
>       const elf_shdr *shdr;
>       int notes_found = 0;
>  
> +     struct setup_header *hdr;
> +     uint64_t payload_offset, payload_length;
> +
>       if (argc != 2)
>       {
>               fprintf(stderr, "Usage: readnotes <elfimage>\n");
> @@ -159,6 +204,27 @@ int main(int argc, char **argv)
>               fprintf(stderr, "Unable to map %s: %s\n", f, strerror(errno));
>               return 1;
>       }
> +     
> +     /* Check the magic of bzImage kernel */
> +     hdr = (struct setup_header *)image;
> +     if ( memcmp(&hdr->header, HDR_MAGIC, HDR_MAGIC_SZ) == 0 )
> +     {
> +             if ( hdr->version < VERSION(2,8) )
> +             {
> +                     printf("%s: boot protocol too old (%04x)", 
> __FUNCTION__, hdr->version);
> +                     return 1;
> +             }
> +
> +             /* upcast to 64 bits to avoid overflow */
> +             /* setup_sects is u8 and so cannot overflow */
> +             payload_offset = (hdr->setup_sects + 1) * 512;
> +             payload_offset += hdr->payload_offset;
> +             payload_length = hdr->payload_length;

Up to this point this is all the same as xc_dom_probe_bzimage_kernel and
therefore looks to me to be safe and correct.

However xc_dom_probe_bzimage_kernel has some additional checks before it
trusts the offset and length. Specifically it compares them against the
size of the on disk kernel image:
    if ( payload_offset >= dom->kernel_size )
    {
        xc_dom_panic(dom->xch, XC_INVALID_KERNEL, "%s: payload offset overflow",
                     __FUNCTION__);
        return -EINVAL;
    }
    if ( (payload_offset + payload_length) > dom->kernel_size )
    {
        xc_dom_panic(dom->xch, XC_INVALID_KERNEL, "%s: payload length overflow",
                     __FUNCTION__);
        return -EINVAL;
    }

I think you likely want to retain some similar check here, using
st.st_size as the boundary instead of dom->kernel_size.

> +
> +             image = image + payload_offset;
> +             st.st_size = payload_length;
> +     }
> +     
>       size = st.st_size;

The usage of size vs st.st_size in the existing code is not very
obvious, IMHO it would be preferable to consistently always use size
from this point on, since changing st.st_size has the possibility of
surprise. IOW the tail here would be:

                image = image + payload_offset;
                size = payload_length
        } else {
                size = st.st_size;
        }

and then use size and not st.st_size for the remainder of the function.

Ian.

>       usize = xc_dom_check_gzip(xch, image, st.st_size);
> 
> This e-mail is intended solely for the person or entity to which it is 
> addressed
> and may contain confidential and/or privileged information. Any review, 
> dissemination,
> copying, printing or other use of this e-mail by persons or entities other 
> than the 
> addressee is prohibited. If you have received this e-mail in error, please 
> contact
> the sender immediately and delete the material from any computer.
> To unsubscribe send an email to: Unsubscribe@xxxxxxxxxxxxxxxxxxxxx 
> Hitachi Consulting (China) Co., Ltd. (HCCD0411)
> 
> 
> 
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@xxxxxxxxxxxxx
> http://lists.xen.org/xen-devel



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