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

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

[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 <marcel@redhat.com>
---
 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--) {
         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));
+
     /* add new one */
     fw_cfg_add_file_callback(s, filename, NULL, NULL, NULL, data, len, true);
     return NULL;
-- 
2.13.5

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

[Marc-André Lureau]

Hi

On Mon, Jan 8, 2018 at 10:50 PM, Marcel Apfelbaum <marcel@redhat.com> 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 <marcel@redhat.com>
> ---
>  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().

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

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





-- 
Marc-André Lureau

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 <marcel@redhat.com> 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 <marcel@redhat.com>
>> ---
>>   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
>>
>>
> 
> 
> 
> 
> 

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

[Laszlo Ersek]

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 <marcel@redhat.com>
> ---
>  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--) {
>          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));
> +
>      /* 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

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

[Laszlo Ersek]

On 01/09/18 13:36, Marcel Apfelbaum wrote:
> 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 <marcel@redhat.com>
>> 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 <marcel@redhat.com>
>>> ---
>>>   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.

I think Marc-André agrees, but his point seems to be that you could
simply remove the assert from the top of fw_cfg_modify_file() --
because, if we reach the fw_cfg_add_file_callback() call at the end of
the function, then fw_cfg_add_file_callback() will verify the same
assert almost immediately.

I'm still staring at the other half of the patch.

Thanks
Laszlo

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

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

[Laszlo Ersek]

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 <marcel@redhat.com>
> ---
>  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--) {
>          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. We currently have count elements in the
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.

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

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

[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 :)

> 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 <marcel@redhat.com>
>> ---
>>   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--) {
>>           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.

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

Thanks,
Marcel

> 
> Thanks
> Laszlo
> 

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

[Laszlo Ersek]

On 01/09/18 14:18, Marcel Apfelbaum wrote:
> 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 :)
> 
>> 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 <marcel@redhat.com>
>>> ---
>>>   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--) {
>>>           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.
> 
> 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.

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

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?

Thanks!
Laszlo

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

[Laszlo Ersek]

On 01/09/18 14:33, Laszlo Ersek wrote:
> On 01/09/18 14:18, Marcel Apfelbaum wrote:
>> 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 :)
>>
>>> 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 <marcel@redhat.com>
>>>> ---
>>>>   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--) {
>>>>           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.
>>
>> 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.
> 
>>
>> 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
>>> 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.
>>>
>>>> @@ -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 */
>>>>       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.
> 
> 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?

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

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

Thanks
Laszlo

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

[Laszlo Ersek]

On 01/09/18 14:35, Laszlo Ersek wrote:
> On 01/09/18 14:33, Laszlo Ersek wrote:
>> On 01/09/18 14:18, Marcel Apfelbaum wrote:
>>> 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 :)
>>>
>>>> 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 <marcel@redhat.com>
>>>>> ---
>>>>>   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--) {
>>>>>           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.
>>>
>>> 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.
>>
>>>
>>> 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
>>>> 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.
>>>>
>>>>> @@ -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 */
>>>>>       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.
>>
>> 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?
> 
> Hm, OK, your point was, abort as *early* as we can.
> 
> I guess you are not wrong :) I'm fine either way, then.

... which means:

Reviewed-by: Laszlo Ersek <lersek@redhat.com>

(sorry, need more sleep)

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

[Marcel Apfelbaum]

On 09/01/2018 15:36, Laszlo Ersek wrote:
> On 01/09/18 14:35, Laszlo Ersek wrote:
>> On 01/09/18 14:33, Laszlo Ersek wrote:
>>> On 01/09/18 14:18, Marcel Apfelbaum wrote:
>>>> 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 :)
>>>>
>>>>> 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 <marcel@redhat.com>
>>>>>> ---
>>>>>>    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--) {
>>>>>>            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.
>>>>
>>>> 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.
>>>

Thanks, I didn't realize the distinction.

>>>>
>>>> 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
>>>>> 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.
>>>>>
>>>>>> @@ -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 */
>>>>>>        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.
>>>
>>> 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?
>>
>> Hm, OK, your point was, abort as *early* as we can.
>>

Right (if we are going to loose, let's loose as soon as we can).

>> I guess you are not wrong :) I'm fine either way, then.
> 
> ... which means:
> 
> Reviewed-by: Laszlo Ersek <lersek@redhat.com>
> 

Appreciated!

> (sorry, need more sleep)
> 

Thanks for looking into it,
Marcel

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

[Marcel Apfelbaum]

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 <marcel@redhat.com>
>> ---
>>   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--) {
>>           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));
>> +
>>       /* 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
> blobs.
> 
> 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
> 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?
> 

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.

Thanks,
Marcel

 >
> Laszlo
> 

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

[Eduardo Habkost]

On Mon, Jan 08, 2018 at 11:50:07PM +0200, 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 <marcel@redhat.com>

Queued on machine-next, thanks!

-- 
Eduardo