* Pankaj Gupta (pagupta@xxxxxxxxxx) wrote:
>
> >
> > * Pankaj Gupta (pagupta@xxxxxxxxxx) 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@xxxxxxxxxx>
> > > ---
> > > 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.
No I don't know why we used uint32_t for it in qemu; I suspect it was just the easiest
thing in the code; but I'd expect it to always be less than 256 in practice.
> 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?
The section ID and the length are two separate issues.
I think you *need* to fix it to either check the length or dynamically allocate it.
Checking footerSecID is a separate idea and something you might try but I'm not sure
how easy it would be to fit into crash's code.
Dave
>
> >
> > 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@xxxxxxxxxx / Manchester, UK
> >
--
Dr. David Alan Gilbert / dgilbert@xxxxxxxxxx / Manchester, UK
--
Crash-utility mailing list
Crash-utility@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/crash-utility
