[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 15:48:32 +0200
User-agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:52.0) Gecko/20100101 Thunderbird/52.4.0

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 
       * 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 
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));
      /* 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

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

     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.



reply via email to

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