[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH] fw_cfg: fix memory corruption when all fw_cfg s

From: Marcel Apfelbaum
Subject: Re: [Qemu-devel] [PATCH] fw_cfg: fix memory corruption when all fw_cfg slots are used
Date: Tue, 9 Jan 2018 14:36:34 +0200
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:52.0) Gecko/20100101 Thunderbird/52.4.0

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

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 
       * 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 

      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 
              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!

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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]