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