Re: [PATCH v6] hw/core/loader-fit: fix freeing errp in fit_load_fdt

[Markus Armbruster <armbru@redhat.com>]

Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:

> 29.11.2019 18:23, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>> 
>>> 29.11.2019 17:11, Vladimir Sementsov-Ogievskiy wrote:
>>>> 29.11.2019 17:03, Markus Armbruster wrote:
>>>>> Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com> writes:
>>>>>
>>>>>> fit_load_fdt forget to check that errp is not NULL and to zero it after
>>>>>> freeing. Fix it.
>>>>>>
>>>>>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>>>>>> Reviewed-by: Eric Blake <eblake@redhat.com>
>>>>>> ---
>>>>>>    hw/core/loader-fit.c | 5 ++++-
>>>>>>    1 file changed, 4 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c
>>>>>> index 953b16bc82..3ee9fb2f2e 100644
>>>>>> --- a/hw/core/loader-fit.c
>>>>>> +++ b/hw/core/loader-fit.c
>>>>>> @@ -200,7 +200,10 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb,
>>>>>>        err = fit_image_addr(itb, img_off, "load", &load_addr, errp);
>>>>>>        if (err == -ENOENT) {
>>>>>>            load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB);
>>>>>> -        error_free(*errp);
>>>>>> +        if (errp) {
>>>>>> +            error_free(*errp);
>>>>>> +            *errp = NULL;
>>>>>> +        }
>>>>>>        } else if (err) {
>>>>>>            error_prepend(errp, "unable to read FDT load address from FIT: ");
>>>>>>            ret = err;
>>>>>
>>>>> This fixes latent bugs when fit_image_addr() fails with ENOENT:
>>>>>
>>>>> * If a caller passes a null @errp, we crash dereferencing it
>>>>>
>>>>> * Else, we pass a dangling Error * to the caller, and return success.
>>>>>     If a caller dereferences the Error * regardless, we have a
>>>>>     use-after-free.
>>>>>
>>>>> Not fixed:
>>>>>
>>>>> * If a caller passes &error_abort or &error_fatal, we die instead of
>>>>>     taking the recovery path.
>>>>
>>>> No, if it is error_abort or error_fatal, we will not reach this point, the execution
>>>> finished before, when error was set.
>>>
>>> Ah, that is what you mention.. Hmm. Is it bad? At least crashing on
>>> error_abort without any recovery should not be bad..
>> 
>> Latent bugs aren't bad, but they could possibly become bad :)
>> 
>> When you pass &err to fit_load_fdt(), where @err is a local variable,
>> the ENOENT case is not actually an error; fit_load_fdt() recovers fine
>> and succeeds.
>> 
>> When you pass &error_fatal or &error_abort, it should likewise not be an
>> error.
>
> Ah, yes, understand now.
>
> Interesting, that auto-propageted errp will not catch this. It will only
> help with error_fatal, but not with error_abort..
>
> So, in this case we should wrap error_abort too. And it in every function that
> uses error_free.
>
> And this means, that in this case we can't make error_abort crash at point where
> actual error occures. That is very sad.

If your patches improve &error_abort's backtrace except for the (few)
functions calling error_free(), then I'd call the situation "a bit sad"
at most :)

[...]