Xen project Mailing List

Re: [Xen-devel] [PATCH] readnote: Add bzImage kernel support
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
©2013 Xen Project, A Linux Foundation Collaborative Project. All Rights Reserved.
Linux Foundation is a registered trademark of The Linux Foundation.
Xen Project is a trademark of The Linux Foundation.