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