[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v6] hw/core/loader-fit: fix freeing errp in fit_load_fdt
From: |
Markus Armbruster |
Subject: |
Re: [PATCH v6] hw/core/loader-fit: fix freeing errp in fit_load_fdt |
Date: |
Fri, 29 Nov 2019 21:03:54 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.2 (gnu/linux) |
Vladimir Sementsov-Ogievskiy <address@hidden> writes:
> 29.11.2019 18:23, Markus Armbruster wrote:
>> Vladimir Sementsov-Ogievskiy <address@hidden> writes:
>>
>>> 29.11.2019 17:11, Vladimir Sementsov-Ogievskiy wrote:
>>>> 29.11.2019 17:03, Markus Armbruster wrote:
>>>>> Vladimir Sementsov-Ogievskiy <address@hidden> 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 <address@hidden>
>>>>>> Reviewed-by: Eric Blake <address@hidden>
>>>>>> ---
>>>>>> 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 :)
[...]