fw_cfg: fix memory corruption when all fw_cfg slots are used

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 <[email protected]>

---
 hw/nvram/fw_cfg.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

hw/nvram/fw_cfg.c +0 -0

index 753ac0e4ea..4313484b21 100644

hw/nvram/fw_cfg.c +0 -1

         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) {

hw/nvram/fw_cfg.c +0 -1

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

2.13.5

Hi

Quoted 16 lines

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 <[email protected]>

---
  hw/nvram/fw_cfg.c | 6 ++++--
  1 file changed, 4 insertions(+), 2 deletions(-)

hw/nvram/fw_cfg.c +0 -0

index 753ac0e4ea..4313484b21 100644

hw/nvram/fw_cfg.c void fw_cfg_add_file_callback(FWCfgState *s, const char *filename, · line 787

Quoted 5 lines

hw/nvram/fw_cfg.c +1 -1

@@ -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().

Quoted 9 lines

hw/nvram/fw_cfg.c +0 -1

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

hw/nvram/fw_cfg.c void *fw_cfg_modify_file(FWCfgState *s, const char *filename, · line 847

Quoted 9 lines

hw/nvram/fw_cfg.c +3 -0

      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.

Quoted 8 lines

hw/nvram/fw_cfg.c +0 -1

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

2.13.5

Quoted 2 lines · Marc-André Lureau

Hi

Hi Marc-André,

Quoted 27 lines

Quoted 16 lines · Marcel Apfelbaum

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 <[email protected]>

---
  hw/nvram/fw_cfg.c | 6 ++++--

  1 file changed, 4 insertions(+), 2 deletions(-)

hw/nvram/fw_cfg.c +0 -0

index 753ac0e4ea..4313484b21 100644

hw/nvram/fw_cfg.c void fw_cfg_add_file_callback(FWCfgState *s, · line 788

Quoted 6 lines

hw/nvram/fw_cfg.c +1 -1

@@ -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?)

Quoted 3 lines

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.

Quoted 10 lines

Quoted 10 lines

hw/nvram/fw_cfg.c +0 -0

           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] =

hw/nvram/fw_cfg.c +0 -1

@@ -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));
 

Quoted 14 lines

hw/nvram/fw_cfg.c void *fw_cfg_modify_file(FWCfgState *s, const · line 848

Quoted 10 lines

hw/nvram/fw_cfg.c +0 -0

       for (i = 0; i < index; i++) {

          if (strcmp(filename, s->files->f[i].name) == 0) {

hw/nvram/fw_cfg.c +3 -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

Quoted 14 lines

Quoted 9 lines

hw/nvram/fw_cfg.c +0 -0

       /* add new one */
       fw_cfg_add_file_callback(s, filename, NULL, NULL, NULL, data,

len, true);
      return NULL;

Signature · 1 line

2.13.5

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 <[email protected]>

---
 hw/nvram/fw_cfg.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

hw/nvram/fw_cfg.c +1 -2

index 753ac0e4ea..4313484b21 100644
@@ -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) {

Quoted 30 lines · Marcel Apfelbaum

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 <[email protected]>

---
  hw/nvram/fw_cfg.c | 6 ++++--
  1 file changed, 4 insertions(+), 2 deletions(-)

hw/nvram/fw_cfg.c +1 -2

index 753ac0e4ea..4313484b21 100644
@@ -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));
 

hw/nvram/fw_cfg.c void *fw_cfg_modify_file(FWCfgState *s, const char *filename, · line 850

Quoted 13 lines

hw/nvram/fw_cfg.c +3 -0

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

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?

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?

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?

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.

Laszlo

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 <[email protected]>

---
 hw/nvram/fw_cfg.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

hw/nvram/fw_cfg.c +0 -0

index 753ac0e4ea..4313484b21 100644

hw/nvram/fw_cfg.c +0 -1

@@ -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) {

Quoted 11 lines

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 <[email protected]>

---
  hw/nvram/fw_cfg.c | 6 ++++--
  1 file changed, 4 insertions(+), 2 deletions(-)

hw/nvram/fw_cfg.c void fw_cfg_add_file_callback(FWCfgState *s, const char *filename, · line 790

Quoted 13 lines

hw/nvram/fw_cfg.c +1 -1

index 753ac0e4ea..4313484b21 100644
@@ -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] =

This hunk looks correct to me.

array, so we cannot normally access the element *at* count. However, we are extending the array right now, therefore we can assign (store) the element at count (and then we'll increment count later). But accessing an element at (count+1) is wrong.

Quoted 15 lines

hw/nvram/fw_cfg.c +3 -1

@@ -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));
+

hw/nvram/fw_cfg.c void *fw_cfg_modify_file(FWCfgState *s, const char *filename, · line 850

Quoted 4 lines

hw/nvram/fw_cfg.c +0 -0

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

I think I agree with Marc-André here, when I say, replace the assert with a comment instead? (About the fact that fw_cfg_add_file_callback() will assert(), *if* we reach that far.)

Thanks
Laszlo

On 09/01/2018 15:09, Laszlo Ersek wrote:

Hi Laszlo,

I'll respond first to this mail' I'll take my time with the rest :)

Quoted 12 lines · Marcel Apfelbaum

Quoted 11 lines

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 <[email protected]>

---
  hw/nvram/fw_cfg.c | 6 ++++--

  1 file changed, 4 insertions(+), 2 deletions(-)

Quoted 16 lines

Quoted 13 lines

hw/nvram/fw_cfg.c +1 -1

index 753ac0e4ea..4313484b21 100644
@@ -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);

Quoted 1 line

          s->entries[0][FW_CFG_FILE_FIRST + i] =

This hunk looks correct to me.

After my change or before?

I think I am right.
At this point we have "count" elements in the array. That means the last element in the array is at arr[count - 1]. We want to make room for the new element at index, so we move all the elements from index to index + 1.

The first element we should move is arr[count - 1] to arr[count]. But the code moved arr[count] to arr [count + 1].
This move is not needed.

 We currently have count elements in the

Quoted 21 lines

array, so we cannot normally access the element *at* count. However, we are extending the array right now, therefore we can assign (store) the element at count (and then we'll increment count later). But accessing an element at (count+1) is wrong.

Quoted 16 lines

hw/nvram/fw_cfg.c +3 -1

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

Quoted 8 lines

Quoted 4 lines

hw/nvram/fw_cfg.c +0 -0

       fw_cfg_add_file_callback(s, filename, NULL, NULL, NULL, data,

len, true);
      return NULL;

I think I agree with Marc-André here, when I say, replace the assert with a comment instead? (About the fact that fw_cfg_add_file_callback() will assert(), *if* we reach that far.)

Hmm, what should we add to the comment? "We lost, brace for impact :)"

My point, if we are going to abort, let's abort as early as we can. But if is a consensus, I'll get rid of it.

Quoted 35 lines · Marcel Apfelbaum

On 09/01/2018 15:09, Laszlo Ersek wrote:

Hi Laszlo,

I'll respond first to this mail' I'll take my time with the rest :)

Quoted 12 lines · Marcel Apfelbaum

Quoted 11 lines

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 <[email protected]>

---
  hw/nvram/fw_cfg.c | 6 ++++--

  1 file changed, 4 insertions(+), 2 deletions(-)

Quoted 16 lines

Quoted 13 lines

hw/nvram/fw_cfg.c +1 -1

index 753ac0e4ea..4313484b21 100644
@@ -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);

Quoted 1 line

          s->entries[0][FW_CFG_FILE_FIRST + i] =

This hunk looks correct to me.

Quoted 1 line

After my change or before?

Well, the source code doesn't have "hunks", patches have hunks. :)

So, I meant, this part of your patch was correct, IMO.

Quoted 43 lines

I think I am right.
At this point we have "count" elements in the array. That means the last element in the array is at arr[count - 1]. We want to make room for the new element at index, so we move all the elements from index to index + 1.

The first element we should move is arr[count - 1] to arr[count]. But the code moved arr[count] to arr [count + 1].
This move is not needed.

 We currently have count elements in the

Quoted 21 lines

array, so we cannot normally access the element *at* count. However, we are extending the array right now, therefore we can assign (store) the element at count (and then we'll increment count later). But accessing an element at (count+1) is wrong.

Quoted 16 lines

hw/nvram/fw_cfg.c +3 -1

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

Quoted 8 lines

Quoted 4 lines

hw/nvram/fw_cfg.c +0 -0

       fw_cfg_add_file_callback(s, filename, NULL, NULL, NULL, data,

len, true);
      return NULL;

I think I agree with Marc-André here, when I say, replace the assert with a comment instead? (About the fact that fw_cfg_add_file_callback() will assert(), *if* we reach that far.)

Quoted 4 lines

Hmm, what should we add to the comment? "We lost, brace for impact :)"

My point, if we are going to abort, let's abort as early as we can. But if is a consensus, I'll get rid of it.

No, it's going to be another assert, just later. Assume that at this point we have (index == fw_cfg_file_slots(s)), because the function didn't find the element to modify, so it decides to add a new one, but also we do not have room for the new one. So, with the suggested removal of the assert, we call fw_cfg_add_file_callback().

Then, fw_cfg_add_file_callback() does:

    if (!s->files) {
        dsize = sizeof(uint32_t) + sizeof(FWCfgFile) * fw_cfg_file_slots(s);
        s->files = g_malloc0(dsize);
        fw_cfg_add_bytes(s, FW_CFG_FILE_DIR, s->files, dsize);
    }

    count = be32_to_cpu(s->files->count);
    assert(count < fw_cfg_file_slots(s));

The (!s->files) condition is expected to eval to false (our table is full, so we do have a table).

And then, the assert() below the "if" will fire.

Am I missing something?

Quoted 110 lines · Laszlo Ersek

Quoted 35 lines · Marcel Apfelbaum

On 09/01/2018 15:09, Laszlo Ersek wrote:

Hi Laszlo,

I'll respond first to this mail' I'll take my time with the rest :)

Quoted 12 lines · Marcel Apfelbaum

Quoted 11 lines

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 <[email protected]>

---
  hw/nvram/fw_cfg.c | 6 ++++--

  1 file changed, 4 insertions(+), 2 deletions(-)

Quoted 16 lines

Quoted 13 lines

hw/nvram/fw_cfg.c +1 -1

index 753ac0e4ea..4313484b21 100644
@@ -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);

Quoted 1 line

          s->entries[0][FW_CFG_FILE_FIRST + i] =

This hunk looks correct to me.

Quoted 1 line

After my change or before?

Well, the source code doesn't have "hunks", patches have hunks. :)

So, I meant, this part of your patch was correct, IMO.

Quoted 43 lines

I think I am right.
At this point we have "count" elements in the array. That means the last element in the array is at arr[count - 1]. We want to make room for the new element at index, so we move all the elements from index to index + 1.

The first element we should move is arr[count - 1] to arr[count]. But the code moved arr[count] to arr [count + 1].
This move is not needed.

 We currently have count elements in the

Quoted 21 lines

array, so we cannot normally access the element *at* count. However, we are extending the array right now, therefore we can assign (store) the element at count (and then we'll increment count later). But accessing an element at (count+1) is wrong.

Quoted 16 lines

hw/nvram/fw_cfg.c +3 -1

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

Quoted 8 lines

Quoted 4 lines

hw/nvram/fw_cfg.c +0 -0

       fw_cfg_add_file_callback(s, filename, NULL, NULL, NULL, data,

len, true);
      return NULL;

I think I agree with Marc-André here, when I say, replace the assert with a comment instead? (About the fact that fw_cfg_add_file_callback() will assert(), *if* we reach that far.)

Quoted 4 lines

Hmm, what should we add to the comment? "We lost, brace for impact :)"

My point, if we are going to abort, let's abort as early as we can. But if is a consensus, I'll get rid of it.

No, it's going to be another assert, just later. Assume that at this point we have (index == fw_cfg_file_slots(s)), because the function didn't find the element to modify, so it decides to add a new one, but also we do not have room for the new one. So, with the suggested removal of the assert, we call fw_cfg_add_file_callback().

Then, fw_cfg_add_file_callback() does:

    if (!s->files) {
        dsize = sizeof(uint32_t) + sizeof(FWCfgFile) * fw_cfg_file_slots(s);
        s->files = g_malloc0(dsize);
        fw_cfg_add_bytes(s, FW_CFG_FILE_DIR, s->files, dsize);
    }

    count = be32_to_cpu(s->files->count);
    assert(count < fw_cfg_file_slots(s));

The (!s->files) condition is expected to eval to false (our table is full, so we do have a table).

Quoted 3 lines

And then, the assert() below the "if" will fire.

Am I missing something?

Hm, OK, your point was, abort as *early* as we can.

I guess you are not wrong :) I'm fine either way, then.

Quoted 37 lines · Laszlo Ersek

Quoted 36 lines · Laszlo Ersek

Quoted 35 lines · Marcel Apfelbaum

On 09/01/2018 15:09, Laszlo Ersek wrote:

Hi Laszlo,

I'll respond first to this mail' I'll take my time with the rest :)

Quoted 28 lines · Marcel Apfelbaum

Quoted 24 lines

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 <[email protected]>

---
   hw/nvram/fw_cfg.c | 6 ++++--
   1 file changed, 4 insertions(+), 2 deletions(-)

hw/nvram/fw_cfg.c +1 -1

index 753ac0e4ea..4313484b21 100644
@@ -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);

Quoted 1 line

          s->entries[0][FW_CFG_FILE_FIRST + i] =

This hunk looks correct to me.

Quoted 6 lines

Quoted 6 lines

Quoted 1 line

After my change or before?

Well, the source code doesn't have "hunks", patches have hunks. :)

So, I meant, this part of your patch was correct, IMO.

Quoted 68 lines

Quoted 68 lines

Quoted 43 lines

I think I am right.
At this point we have "count" elements in the array. That means the last element in the array is at arr[count - 1]. We want to make room for the new element at index, so we move all the elements from index to index + 1.

The first element we should move is arr[count - 1] to arr[count]. But the code moved arr[count] to arr [count + 1].
This move is not needed.

 We currently have count elements in the

Quoted 21 lines

array, so we cannot normally access the element *at* count. However, we are extending the array right now, therefore we can assign (store) the element at count (and then we'll increment count later). But accessing an element at (count+1) is wrong.

Quoted 16 lines

hw/nvram/fw_cfg.c +3 -1

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

Quoted 8 lines

Quoted 4 lines

hw/nvram/fw_cfg.c +0 -0

       fw_cfg_add_file_callback(s, filename, NULL, NULL, NULL, data,

len, true);
      return NULL;

I think I agree with Marc-André here, when I say, replace the assert with a comment instead? (About the fact that fw_cfg_add_file_callback() will assert(), *if* we reach that far.)

Quoted 4 lines

Hmm, what should we add to the comment? "We lost, brace for impact :)"

My point, if we are going to abort, let's abort as early as we can. But if is a consensus, I'll get rid of it.

No, it's going to be another assert, just later. Assume that at this point we have (index == fw_cfg_file_slots(s)), because the function didn't find the element to modify, so it decides to add a new one, but also we do not have room for the new one. So, with the suggested removal of the assert, we call fw_cfg_add_file_callback().

Then, fw_cfg_add_file_callback() does:

     if (!s->files) {
         dsize = sizeof(uint32_t) + sizeof(FWCfgFile) * fw_cfg_file_slots(s);
         s->files = g_malloc0(dsize);
         fw_cfg_add_bytes(s, FW_CFG_FILE_DIR, s->files, dsize);
     }

     count = be32_to_cpu(s->files->count);
     assert(count < fw_cfg_file_slots(s));

The (!s->files) condition is expected to eval to false (our table is full, so we do have a table).

Quoted 6 lines

Quoted 3 lines

And then, the assert() below the "if" will fire.

Am I missing something?

Hm, OK, your point was, abort as *early* as we can.

Quoted 1 line

I guess you are not wrong :) I'm fine either way, then.

... which means:

Reviewed-by Laszlo Ersek <[email protected]>

(sorry, need more sleep)

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 <[email protected]>