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

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

[Vladimir Sementsov-Ogievskiy]

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

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

[Markus Armbruster]

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.

We need to use a local_err here.

I'll search for the pattern, and post fix(es).

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

[Vladimir Sementsov-Ogievskiy]

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.

> 
> We need to use a local_err here.
> 
> I'll search for the pattern, and post fix(es).
> 


-- 
Best regards,
Vladimir

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

[Vladimir Sementsov-Ogievskiy]

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

> 
>>
>> We need to use a local_err here.
>>
>> I'll search for the pattern, and post fix(es).
>>
> 
> 


-- 
Best regards,
Vladimir

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

[Markus Armbruster]

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.

General rule: when you want to handle some (or all) of a function's
errors yourself, you must not pass your own @errp argument.  Instead,
pass &err and propagate the errors you don't handle yourself with
error_propagate(errp, err).

>>> We need to use a local_err here.
>>>
>>> I'll search for the pattern, and post fix(es).

Found several bugs already...

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

[Vladimir Sementsov-Ogievskiy]

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.

> 
> General rule: when you want to handle some (or all) of a function's
> errors yourself, you must not pass your own @errp argument.  Instead,
> pass &err and propagate the errors you don't handle yourself with
> error_propagate(errp, err).
> 
>>>> We need to use a local_err here.
>>>>
>>>> I'll search for the pattern, and post fix(es).
> 
> Found several bugs already...
> 


-- 
Best regards,
Vladimir

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

[Markus Armbruster]

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

[...]

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

[Markus Armbruster]

Markus Armbruster <armbru@redhat.com> writes:

> 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.
>
> General rule: when you want to handle some (or all) of a function's
> errors yourself, you must not pass your own @errp argument.  Instead,
> pass &err and propagate the errors you don't handle yourself with
> error_propagate(errp, err).
>
>>>> We need to use a local_err here.
>>>>
>>>> I'll search for the pattern, and post fix(es).
>
> Found several bugs already...

[PATCH 00/21] Error handling fixes, may contain 4.2 material
Message-Id: <20191130194240.10517-1-armbru@redhat.com>

I hope it doesn't clash too badly with your work.

PATCH 10 fixes the one we discussed above.