[Crash-utility] [PATCH 1/2] qemu: parse 'vm_configuration' section

[Pankaj Gupta]

> 
> * Pankaj Gupta (pagupta at redhat.com) wrote:
> >  Teach crash to parse 'QEMU_VM_CONFIGURATION' section. This
> >  section is added by migration code to save VM running state.
> > 
> > Signed-off-by: Pankaj Gupta <pagupta at redhat.com>
> > ---
> >  qemu-load.c | 16 ++++++++++++++++
> >  qemu-load.h |  3 ++-
> >  2 files changed, 18 insertions(+), 1 deletion(-)
> > 
> > diff --git a/qemu-load.c b/qemu-load.c
> > index 8b4372e..7ee58a3 100644
> > --- a/qemu-load.c
> > +++ b/qemu-load.c
> > @@ -208,6 +208,16 @@ get_string (FILE *fp, char *name)
> >  	name[sz] = 0;
> >  	return sz;
> >  }
> > +static int
> > +get_string_len (FILE *fp, char *name, uint32_t sz)
> > +{
> > +	size_t items ATTRIBUTE_UNUSED;
> > +	if (sz == EOF)
> > +		return -1;
> > +	items = fread (name, sz, 1, fp);
> > +	name[sz] = 0;
> > +	return sz;
> > +}
> >  
> >  static void
> >  ram_read_blocks (FILE *fp, uint64_t size)
> > @@ -924,6 +934,7 @@ qemu_load (const struct qemu_device_loader *devices,
> > uint32_t required_features,
> >  	struct qemu_device_list *result = NULL;
> >  	struct qemu_device *last = NULL;;
> >  	size_t items ATTRIBUTE_UNUSED;
> > +	char name[257];
> >  
> >  	switch (get_be32 (fp)) {
> >  	case QEMU_VM_FILE_MAGIC:
> > @@ -961,6 +972,11 @@ qemu_load (const struct qemu_device_loader *devices,
> > uint32_t required_features,
> >  			break;
> >  		if (sec == QEMU_VM_EOF)
> >  			break;
> > +		if (sec == QEMU_VM_CONFIGURATION) {
> > +			uint32_t len = get_be32 (fp);
> > +			get_string_len (fp, name, len);
> > +			continue;
> > +                }
> 
> Be careful about buffer over runs!   That 'len' must be checked to ensure
> it fits into 'name' - or dynamically allocate 'name'. OK, in practice the
> value
> will be small, but this tool is aimed to read crash files that might have
> been
> dumped by a customer and so you have to treat the contents of the file
> as potentially hostile, either due to file corruption or really someone
> trying
> to be nasty.

I agree with you. But I still not sure why in Qemu we selected 'uint32_t' for
length and why its not consistent everywhere? If in good case we expect 'length > 256'
bytes I will do the change and spin new version.

And if we are sure length will be '<=256' bytes, I think validating 'footerSecId' in
a new patch should avoid us parsing corrupt file. What you suggest here? 

> 
> Dave
> 
> >  		d = device_get (devices, result, sec, fp);
> >  		if (!d)
> > diff --git a/qemu-load.h b/qemu-load.h
> > index e52b068..d5b2078 100644
> > --- a/qemu-load.h
> > +++ b/qemu-load.h
> > @@ -29,7 +29,8 @@ enum qemu_save_section {
> >    QEMU_VM_SECTION_PART,
> >    QEMU_VM_SECTION_END,
> >    QEMU_VM_SECTION_FULL,
> > -  QEMU_VM_SUBSECTION
> > +  QEMU_VM_SUBSECTION,
> > +  QEMU_VM_CONFIGURATION = 0x07
> >  };
> >  
> >  enum qemu_features {
> > --
> > 1.9.3
> > 
> --
> Dr. David Alan Gilbert / dgilbert at redhat.com / Manchester, UK
>