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

[Marcel Apfelbaum]

On 09/01/2018 13:15, Marc-André Lureau wrote:
> Hi

Hi Marc-André,

> On Mon, Jan 8, 2018 at 10:50 PM, Marcel Apfelbaum <address@hidden> 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--) {
>
> Good catch (could be worth a test in fw_cfg-test.c for ASAN check?)
>
> Just some thought, I wonder if the sorting should be done once after
> all entries are added, with some higher function like g_array_sort().

I personally have nothing against this kind of insertion sort.

> >           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));
> > +
>
> Well, the assert is redundant with the one in
> fw_cfg_add_file_callback() at this point. I think the original assert
> is there for sanity check only, before iterating over files.

Is not in fw_cfg_add_file_callback(), is in fw_cfg_modify_file()
which strangely can decide to add a file if is not there already.

The previous place of the assert was not good since it checked to see
if there is enough room to add a file, but most of the times,
as the function name suggests, we only modify one.

Let's say we have all the slots full and we want to modify an existing file.
The assert triggers abort even if is not needed.
However, if the file is not there the assert checks we have room before it adds
the file.

Thanks for reviewing the patch!
Marcel


> >       /* add new one */
> >       fw_cfg_add_file_callback(s, filename, NULL, NULL, NULL, data, len, true);
> >       return NULL;
> > --
> > 2.13.5