Re: [Qemu-devel] [PATCH 4/6] fw_cfg: exit with error when dupe fw_cfg file name inserted

[Laszlo Ersek <lersek@redhat.com>]

On 03/16/15 15:15, Gabriel L. Somlo wrote:
> Currently, when fw_cfg_add_file_callback() is invoked with a
> duplicate file name, it gets to insert the data blob at the
> next available selector, but exit (signalling the error via
> a call to the trace_fw_cfg_add_file_dupe() function) before
> incrementing the next available selector as soon as it finds
> the requested file name to be a dupe.
> 
> As a consequence, the immediately following invocation of
> fw_cfg_add_file_callback() will cause the data blob pointer
> at the current selector to be overwritten, and therefore
> leak the old data blob inserted during the previous failed
> call (or, instead, trigger the newly added assertion which
> guards against leaking data blobs by overwriting their
> pointers).
> 
> This patch modifies fw_cfg_add_file_callback() to exit qemu
> with an error whenever a duplicate fw_cfg file name insertion
> is requested.
> 
> Signed-off-by: Gabriel Somlo <somlo@cmu.edu>
> ---
>  hw/nvram/fw_cfg.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 5501a97..86f120e 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -481,17 +481,18 @@ void fw_cfg_add_file_callback(FWCfgState *s,  const char *filename,
>      index = be32_to_cpu(s->files->count);
>      assert(index < FW_CFG_FILE_SLOTS);
>  
> +    for (i = 0; i < index; i++) {
> +        if (strcmp(filename, s->files->f[i].name) == 0) {
> +            error_report("duplicate fw_cfg file name: %s", filename);
> +            exit(1);
> +        }
> +    }
> +
>      fw_cfg_add_bytes_read_callback(s, FW_CFG_FILE_FIRST + index,
>                                     callback, callback_opaque, data, len);
>  
>      pstrcpy(s->files->f[index].name, sizeof(s->files->f[index].name),
>              filename);
> -    for (i = 0; i < index; i++) {
> -        if (strcmp(s->files->f[index].name, s->files->f[i].name) == 0) {
> -            trace_fw_cfg_add_file_dupe(s, s->files->f[index].name);
> -            return;
> -        }
> -    }
>  
>      s->files->f[index].size   = cpu_to_be32(len);
>      s->files->f[index].select = cpu_to_be16(FW_CFG_FILE_FIRST + index);
> 

I agree with the general direction of this patch. I also accept that
exiting here on error is probably "best", considering how changing the
prototype to propagate an error would turn upside down the many callers
for little benefit.

The details can be improved however:
- your hoisted comparison compares the full input filename against
filenames already in the directory. That will miss matches if only a
distant suffix of the filename differs. The pre-patch code truncates
first, then compares. This becomes an issue because you're going to
expose the filename on the command line (ie. it becomes UI from API).

In fact, what speaks against simply replacing the trace + the return
statement with error_report() + exit() in-place?

Alternatively, you could assert that the input filename is not overlong,
and check that predicate in the caller (patch 5/6).

- Another trace call removed; if there are no other such calls, the
tracepoint definition should be removed too.

... I'm tired, I'll have to stop reviewing here.

Thanks
Laszlo