Re: [Qemu-stable] [Qemu-devel] [PATCH] fw_cfg: fix memory corruption when all fw_cfg slots are used

[Marcel Apfelbaum]

Hi Laszlo,

On 09/01/2018 14:51, Laszlo Ersek wrote:
> On 01/08/18 22:50, Marcel Apfelbaum wrote:
> > When all the fw_cfg slots are used, a write is made outside the
> > bounds of the fw_cfg files array as part of the sort algorithm.
> >
> > Fix it by avoiding an unnecessary array element move.
> > Fix also an assert while at it.
> >
> > Signed-off-by: Marcel Apfelbaum <address@hidden>
> > ---
> >   hw/nvram/fw_cfg.c | 6 ++++--
> >   1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> > index 753ac0e4ea..4313484b21 100644
> > --- a/hw/nvram/fw_cfg.c
> > +++ b/hw/nvram/fw_cfg.c
> > @@ -784,7 +784,7 @@ void fw_cfg_add_file_callback(FWCfgState *s,  const char 
> > *filename,
> >        * index and "i - 1" is the one being copied from, thus the
> >        * unusual start and end in the for statement.
> >        */
> > -    for (i = count + 1; i > index; i--) {
> > +    for (i = count; i > index; i--) {
> >           s->files->f[i] = s->files->f[i - 1];
> >           s->files->f[i].select = cpu_to_be16(FW_CFG_FILE_FIRST + i);
> >           s->entries[0][FW_CFG_FILE_FIRST + i] =
> > @@ -833,7 +833,6 @@ void *fw_cfg_modify_file(FWCfgState *s, const char 
> > *filename,
> >       assert(s->files);
> >   
> >       index = be32_to_cpu(s->files->count);
> > -    assert(index < fw_cfg_file_slots(s));
> >   
> >       for (i = 0; i < index; i++) {
> >           if (strcmp(filename, s->files->f[i].name) == 0) {
> > @@ -843,6 +842,9 @@ void *fw_cfg_modify_file(FWCfgState *s, const char 
> > *filename,
> >               return ptr;
> >           }
> >       }
> > +
> > +    assert(index < fw_cfg_file_slots(s));
> > +
> >       /* add new one */
> >       fw_cfg_add_file_callback(s, filename, NULL, NULL, NULL, data, len, true);
> >       return NULL;
>
> Given that I was CC'd... 

Its not me! get_maintainers found you...

> I don't understand the purpose of the sorting.
> I've read commit bab47d9a75a3 ("Sort the fw_cfg file list", 2016-04-07),
> yes. It does not spell out the goal.
>
> I assume the idea is migration compatibility for the same machine type
> between QEMU releases. Is that correct?

I think the commit intended only to avoid fw_cfg files order to change
for the same machine between QEMU versions since is guest visible.

> A new release might assign a
> different selector key to the same fw_cfg file, and a guest that is
> migrated while reading fw_cfg could end up with half-half of different
> blobs.
>
> Wasn't this somehow addressed by keeping fw_cfg blobs in RAM blocks or
> some such?

Adding Dave, maybe he can answer to that.

> Also, assuming the problem is real (i.e., we need to stick with the same
> numeric values regardless of moving around the insertion points in the
> source code), then why is it safe for "non-legacy" machine types to do
> ... er... "something else" than keep the same numeric values? The commit
> says,
>
>      This will avoid any future issues of moving the file creation
>      around, it doesn't matter what order they are created now,
>      the will always be in filename order.
>
> Which is true, but if a new file is introduced (with a pathname in the
> middle), then that will shift up everything behind. Or... is the idea
> that the same machine type on a new QEMU release may only reorder the
> additions of the preexistent fw_cfg files across the source code, but
> must not expose *new* fw_cfg files?

I think this is the reason, and the machine should not have new fw_cfg files.


> If this is the idea, then keeping the list sorted would indeed ensure
> the same numeric values too -- however, I don't think the assumption is
> safe. Certain devices may expose dedicated, standalone fw_cfg blobs, and
> such devices, when they are implemented, are generally enabled for older
> machine types too, retroactively. ... Hm, in that case, is the argument
> that the device can never be present on the *source* (old version) QEMU,
> so for migration to be even considered, it can also not be present on
> the target (new version) QEMU?
>
> Wow, complex. 

For sure. Let's see what Dave can add.

Thanks,
Marcel

>
> Laszlo