Re: [PATCH-for-5.1] hw/misc/aspeed_sdmc: Fix incorrect memory size

[Markus Armbruster <armbru@redhat.com>]

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> On 7/20/20 6:07 PM, Cédric Le Goater wrote:
>> On 7/20/20 11:58 AM, Philippe Mathieu-Daudé wrote:
>>> The SDRAM Memory Controller has a 32-bit address bus, thus
>>> supports up to 4 GiB of DRAM. There is a signed to unsigned
>>> conversion error with the AST2600 maximum memory size:
>>>
>>>   (uint64_t)(2048 << 20) = (uint64_t)(-2147483648)
>>>                          = 0xffffffff40000000
>>>                          = 16 EiB - 2 GiB
>>>
>>> Fix by using the IEC suffixes which are usually safer, and add
>>> a check to verify the memory is valid. This would have catched

caught

>>> this bug:
>>>
>>>     Unexpected error in aspeed_sdmc_realize() at hw/misc/aspeed_sdmc.c:261:
>>>     qemu-system-arm: Invalid RAM size 16 EiB
>> 
>> Indeed :/
>> 
>>>
>>> Fixes: 1550d72679 ("aspeed/sdmc: Add AST2600 support")
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> ---
>>>  hw/misc/aspeed_sdmc.c | 12 +++++++++---
>>>  1 file changed, 9 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
>>> index 0737d8de81..76dd7e6a20 100644
>>> --- a/hw/misc/aspeed_sdmc.c
>>> +++ b/hw/misc/aspeed_sdmc.c
>>> @@ -256,6 +256,12 @@ static void aspeed_sdmc_realize(DeviceState *dev, Error **errp)
>>>      AspeedSDMCClass *asc = ASPEED_SDMC_GET_CLASS(s);
>>>  
>>>      s->max_ram_size = asc->max_ram_size;
>>> +    if (s->max_ram_size >= 4 * GiB) {
>>> +        char *szstr = size_to_str(s->max_ram_size);
>>> +        error_setg(errp, "Invalid RAM size %s", szstr);
>>> +        g_free(szstr);
>>> +        return;
>>> +    }
>>>
>> 
>> I would put an assert() since the max RAM size is not user configurable. 
>
> As you wish, at this point I'm completely lost with error reporting.

:-/

> Per the manual
> (https://www.mail-archive.com/qemu-devel@nongnu.org/msg723217.html):
>
>  "Many, many devices neglect to clean up properly on error, and get away
>   with it only because all callers treat errors as fatal.
>
>   If you decide to take cleanup shortcuts, say because the cleanup is
>   untestable, consider adding a comment at least."
>
> So I'll go for address + comment:
>
>   assert(s->max_ram_size < 4 * GiB); /* 32-bit address bus */

Makes sense.

Note this is *not* a cleanup shortcut, at least not the kind I had in
mind.

What I had in mind is unclean failure, i.e. returning on error without
proper cleanup: revert changes made so far, free resources.  This is
*wrong*.  But the wrongness doesn't matter when all callers treat errors
as fatal.

Checking an impossible condition with assert() is better than treating
it as an error and bungling its handling.  If you treat it as an error,
do it properly.  Since I'm quite skeptical about the chances of pulling
off "properly" for untestable things, I prefer assertions.

There's another reason.  User errors need to be handled gracefully.
Programming errors should (in my opinion) trigger abort(), so they get
fixed.

When the spot that detects the error can't know which kind it is, you
have to fail cleanly and let the caller decide how to handle the error.

Example: object_property_find() errors out when the property doesn't
exist.  This may be a programming error, e.g. a well-known property
isn't found, because a programmer mistyped the property name.  Or it may
be a user error, e.g. a user mistyped the property name argument of
qom-get.

When functions have multiple failure modes, and only some of them are
programming errors, the caller typically can't tell them apart, and errs
on the side of user error.  Programming errors then get reported as
(typically confusing!) user errors.

The #1 reason for such awkward functions is lazy thinking + eager
typing: by treating anything that can go wrong as an error for the
caller to handle, I can replace thinking about what may go wrong and
what must not go wrong by typing up a bunch of error paths.  Great time
saver as long as I stick to the time-honored strategy of not bothering
to test my error paths.