[v2,1/2] aspeed/soc: Fix possible divide by zero

Commit Message

Jamin Lin June 25, 2024, 1:50 a.m. UTC

Coverity reports a possible DIVIDE_BY_ZERO issue regarding the
"ram_size" object property. This can not happen because RAM has
predefined valid sizes per SoC. Nevertheless, add a test to
close the issue.

Fixes: Coverity CID 1547113
Signed-off-by: Jamin Lin <[e-mail]>
Reviewed-by: Cédric Le Goater <[e-mail]>
[ clg: Rewrote commit log ]
Signed-off-by: Cédric Le Goater <[e-mail]>
---
 hw/arm/aspeed_ast27x0.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

cmd June 25, 2024, 6 a.m. UTC | #1

Hi

On 25/06/2024 03:50, Jamin Lin via wrote:
> Coverity reports a possible DIVIDE_BY_ZERO issue regarding the
> "ram_size" object property. This can not happen because RAM has
> predefined valid sizes per SoC. Nevertheless, add a test to
> close the issue.
>
> Fixes: Coverity CID 1547113
> Signed-off-by: Jamin Lin <[e-mail]>
> Reviewed-by: Cédric Le Goater <[e-mail]>
> [ clg: Rewrote commit log ]
> Signed-off-by: Cédric Le Goater <[e-mail]>
> ---
>   hw/arm/aspeed_ast27x0.c | 6 ++++++
>   1 file changed, 6 insertions(+)
>
> diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c
> index b6876b4862..d14a46df6f 100644
> --- a/hw/arm/aspeed_ast27x0.c
> +++ b/hw/arm/aspeed_ast27x0.c
> @@ -211,6 +211,12 @@ static void aspeed_ram_capacity_write(void *opaque, hwaddr addr, uint64_t data,
>       ram_size = object_property_get_uint(OBJECT(&s->sdmc), "ram-size",
>                                           &error_abort);
>   
> +    if (!ram_size) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: ram_size is zero",  __func__);
> +        return;
> +    }
> +
If we are sure that the error cannot happen, shouldn't we assert instead?
>       /*
>        * Emulate ddr capacity hardware behavior.
>        * If writes the data to the address which is beyond the ram size,

Cédric Le Goater June 25, 2024, 6:03 a.m. UTC | #2

On 6/25/24 8:00 AM, cmd wrote:
> Hi
> 
> On 25/06/2024 03:50, Jamin Lin via wrote:
>> Coverity reports a possible DIVIDE_BY_ZERO issue regarding the
>> "ram_size" object property. This can not happen because RAM has
>> predefined valid sizes per SoC. Nevertheless, add a test to
>> close the issue.
>>
>> Fixes: Coverity CID 1547113
>> Signed-off-by: Jamin Lin <[e-mail]>
>> Reviewed-by: Cédric Le Goater <[e-mail]>
>> [ clg: Rewrote commit log ]
>> Signed-off-by: Cédric Le Goater <[e-mail]>
>> ---
>>   hw/arm/aspeed_ast27x0.c | 6 ++++++
>>   1 file changed, 6 insertions(+)
>>
>> diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c
>> index b6876b4862..d14a46df6f 100644
>> --- a/hw/arm/aspeed_ast27x0.c
>> +++ b/hw/arm/aspeed_ast27x0.c
>> @@ -211,6 +211,12 @@ static void aspeed_ram_capacity_write(void *opaque, hwaddr addr, uint64_t data,
>>       ram_size = object_property_get_uint(OBJECT(&s->sdmc), "ram-size",
>>                                           &error_abort);
>> +    if (!ram_size) {
>> +        qemu_log_mask(LOG_GUEST_ERROR,
>> +                      "%s: ram_size is zero",  __func__);
>> +        return;
>> +    }
>> +
> If we are sure that the error cannot happen, shouldn't we assert instead?

Yes. That is what Peter suggested. This needs to be changed.


Thanks,

C.



>>       /*
>>        * Emulate ddr capacity hardware behavior.
>>        * If writes the data to the address which is beyond the ram size,

cmd June 25, 2024, 6:07 a.m. UTC | #3

On 25/06/2024 08:03, Cédric Le Goater wrote:
> On 6/25/24 8:00 AM, cmd wrote:
>> Hi
>>
>> On 25/06/2024 03:50, Jamin Lin via wrote:
>>> Coverity reports a possible DIVIDE_BY_ZERO issue regarding the
>>> "ram_size" object property. This can not happen because RAM has
>>> predefined valid sizes per SoC. Nevertheless, add a test to
>>> close the issue.
>>>
>>> Fixes: Coverity CID 1547113
>>> Signed-off-by: Jamin Lin <[e-mail]>
>>> Reviewed-by: Cédric Le Goater <[e-mail]>
>>> [ clg: Rewrote commit log ]
>>> Signed-off-by: Cédric Le Goater <[e-mail]>
>>> ---
>>>   hw/arm/aspeed_ast27x0.c | 6 ++++++
>>>   1 file changed, 6 insertions(+)
>>>
>>> diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c
>>> index b6876b4862..d14a46df6f 100644
>>> --- a/hw/arm/aspeed_ast27x0.c
>>> +++ b/hw/arm/aspeed_ast27x0.c
>>> @@ -211,6 +211,12 @@ static void aspeed_ram_capacity_write(void 
>>> *opaque, hwaddr addr, uint64_t data,
>>>       ram_size = object_property_get_uint(OBJECT(&s->sdmc), "ram-size",
>>>                                           &error_abort);
>>> +    if (!ram_size) {
>>> +        qemu_log_mask(LOG_GUEST_ERROR,
>>> +                      "%s: ram_size is zero",  __func__);
>>> +        return;
>>> +    }
>>> +
>> If we are sure that the error cannot happen, shouldn't we assert 
>> instead?
>
> Yes. That is what Peter suggested. This needs to be changed.
>
>
> Thanks,
>
> C.
>
Ok fine, I didn't see the message, sorry!

Thanks

 >cmd

>
>
>>>       /*
>>>        * Emulate ddr capacity hardware behavior.
>>>        * If writes the data to the address which is beyond the ram 
>>> size,
>

Jamin Lin June 25, 2024, 6:15 a.m. UTC | #4

Hi cmd, Cedric and Peter,

> -----Original Message-----
> From: cmd <[e-mail]>
> Sent: Tuesday, June 25, 2024 2:07 PM
> To: Cédric Le Goater <[e-mail]>; Jamin Lin <[e-mail]>;
> Peter Maydell <[e-mail]>; Steven Lee
> <[e-mail]>; Troy Lee <[e-mail]>; Andrew
> Jeffery <[e-mail]>; Joel Stanley <[e-mail]>; open
> list:ASPEED BMCs <[e-mail]>; open list:All patches CC here
> <[e-mail]>
> Cc: Cédric Le Goater <[e-mail]>
> Subject: Re: [PATCH v2 1/2] aspeed/soc: Fix possible divide by zero
> 
> 
> On 25/06/2024 08:03, Cédric Le Goater wrote:
> > On 6/25/24 8:00 AM, cmd wrote:
> >> Hi
> >>
> >> On 25/06/2024 03:50, Jamin Lin via wrote:
> >>> Coverity reports a possible DIVIDE_BY_ZERO issue regarding the
> >>> "ram_size" object property. This can not happen because RAM has
> >>> predefined valid sizes per SoC. Nevertheless, add a test to close
> >>> the issue.
> >>>
> >>> Fixes: Coverity CID 1547113
> >>> Signed-off-by: Jamin Lin <[e-mail]>
> >>> Reviewed-by: Cédric Le Goater <[e-mail]> [ clg: Rewrote commit
> >>> log ]
> >>> Signed-off-by: Cédric Le Goater <[e-mail]>
> >>> ---
> >>>   hw/arm/aspeed_ast27x0.c | 6 ++++++
> >>>   1 file changed, 6 insertions(+)
> >>>
> >>> diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c index
> >>> b6876b4862..d14a46df6f 100644
> >>> --- a/hw/arm/aspeed_ast27x0.c
> >>> +++ b/hw/arm/aspeed_ast27x0.c
> >>> @@ -211,6 +211,12 @@ static void aspeed_ram_capacity_write(void
> >>> *opaque, hwaddr addr, uint64_t data,
> >>>       ram_size = object_property_get_uint(OBJECT(&s->sdmc),
> >>> "ram-size",
> >>>                                           &error_a
> bort);
> >>> +    if (!ram_size) {
> >>> +        qemu_log_mask(LOG_GUEST_ERROR,
> >>> +                      "%s: ram_size is zero",  __func__);
> >>> +        return;
> >>> +    }
> >>> +
> >> If we are sure that the error cannot happen, shouldn't we assert
> >> instead?
> >
> > Yes. That is what Peter suggested. This needs to be changed.
> >
Thanks for review and suggestion.
How about this change?

assert(ram_size > 0);

If you agree, I will send v3 patch.
Thanks-Jamin

> >
> > Thanks,
> >
> > C.
> >
> Ok fine, I didn't see the message, sorry!
> 
> Thanks
> 
>  >cmd
> 
> >
> >
> >>>       /*
> >>>        * Emulate ddr capacity hardware behavior.
> >>>        * If writes the data to the address which is beyond the ram
> >>> size,
> >

Cédric Le Goater June 25, 2024, 6:37 a.m. UTC | #5

On 6/25/24 8:15 AM, Jamin Lin wrote:
> Hi cmd, Cedric and Peter,
> 
>> -----Original Message-----
>> From: cmd <[e-mail]>
>> Sent: Tuesday, June 25, 2024 2:07 PM
>> To: Cédric Le Goater <[e-mail]>; Jamin Lin <[e-mail]>;
>> Peter Maydell <[e-mail]>; Steven Lee
>> <[e-mail]>; Troy Lee <[e-mail]>; Andrew
>> Jeffery <[e-mail]>; Joel Stanley <[e-mail]>; open
>> list:ASPEED BMCs <[e-mail]>; open list:All patches CC here
>> <[e-mail]>
>> Cc: Cédric Le Goater <[e-mail]>
>> Subject: Re: [PATCH v2 1/2] aspeed/soc: Fix possible divide by zero
>>
>>
>> On 25/06/2024 08:03, Cédric Le Goater wrote:
>>> On 6/25/24 8:00 AM, cmd wrote:
>>>> Hi
>>>>
>>>> On 25/06/2024 03:50, Jamin Lin via wrote:
>>>>> Coverity reports a possible DIVIDE_BY_ZERO issue regarding the
>>>>> "ram_size" object property. This can not happen because RAM has
>>>>> predefined valid sizes per SoC. Nevertheless, add a test to close
>>>>> the issue.
>>>>>
>>>>> Fixes: Coverity CID 1547113
>>>>> Signed-off-by: Jamin Lin <[e-mail]>
>>>>> Reviewed-by: Cédric Le Goater <[e-mail]> [ clg: Rewrote commit
>>>>> log ]
>>>>> Signed-off-by: Cédric Le Goater <[e-mail]>
>>>>> ---
>>>>>    hw/arm/aspeed_ast27x0.c | 6 ++++++
>>>>>    1 file changed, 6 insertions(+)
>>>>>
>>>>> diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c index
>>>>> b6876b4862..d14a46df6f 100644
>>>>> --- a/hw/arm/aspeed_ast27x0.c
>>>>> +++ b/hw/arm/aspeed_ast27x0.c
>>>>> @@ -211,6 +211,12 @@ static void aspeed_ram_capacity_write(void
>>>>> *opaque, hwaddr addr, uint64_t data,
>>>>>        ram_size = object_property_get_uint(OBJECT(&s->sdmc),
>>>>> "ram-size",
>>>>>                                            &error_a
>> bort);
>>>>> +    if (!ram_size) {
>>>>> +        qemu_log_mask(LOG_GUEST_ERROR,
>>>>> +                      "%s: ram_size is zero",  __func__);
>>>>> +        return;
>>>>> +    }
>>>>> +
>>>> If we are sure that the error cannot happen, shouldn't we assert
>>>> instead?
>>>
>>> Yes. That is what Peter suggested. This needs to be changed.
>>>
> Thanks for review and suggestion.
> How about this change?
> 
> assert(ram_size > 0);

yes.

I will send another patch fixing a long standing issue in the SDMC
model not checking the ram_size value in the realize handler. It
relies on the "ram-size" property being set.

Thanks,

C.


> If you agree, I will send v3 patch.
> Thanks-Jamin
> 
>>>
>>> Thanks,
>>>
>>> C.
>>>
>> Ok fine, I didn't see the message, sorry!
>>
>> Thanks
>>
>>   >cmd
>>
>>>
>>>
>>>>>        /*
>>>>>         * Emulate ddr capacity hardware behavior.
>>>>>         * If writes the data to the address which is beyond the ram
>>>>> size,
>>>

Jamin Lin June 25, 2024, 6:41 a.m. UTC | #6

Hi Cedric,

> -----Original Message-----
> From: Cédric Le Goater <[e-mail]>
> Sent: Tuesday, June 25, 2024 2:38 PM
> To: Jamin Lin <[e-mail]>; cmd
> <[e-mail]>; Peter Maydell
> <[e-mail]>; Steven Lee <[e-mail]>; Troy
> Lee <[e-mail]>; Andrew Jeffery <[e-mail]>;
> Joel Stanley <[e-mail]>; open list:ASPEED BMCs
> <[e-mail]>; open list:All patches CC here
> <[e-mail]>
> Cc: Cédric Le Goater <[e-mail]>
> Subject: Re: [PATCH v2 1/2] aspeed/soc: Fix possible divide by zero
> 
> On 6/25/24 8:15 AM, Jamin Lin wrote:
> > Hi cmd, Cedric and Peter,
> >
> >> -----Original Message-----
> >> From: cmd <[e-mail]>
> >> Sent: Tuesday, June 25, 2024 2:07 PM
> >> To: Cédric Le Goater <[e-mail]>; Jamin Lin
> >> <[e-mail]>; Peter Maydell <[e-mail]>;
> >> Steven Lee <[e-mail]>; Troy Lee
> <[e-mail]>;
> >> Andrew Jeffery <[e-mail]>; Joel Stanley
> >> <[e-mail]>; open list:ASPEED BMCs <[e-mail]>; open
> >> list:All patches CC here <[e-mail]>
> >> Cc: Cédric Le Goater <[e-mail]>
> >> Subject: Re: [PATCH v2 1/2] aspeed/soc: Fix possible divide by zero
> >>
> >>
> >> On 25/06/2024 08:03, Cédric Le Goater wrote:
> >>> On 6/25/24 8:00 AM, cmd wrote:
> >>>> Hi
> >>>>
> >>>> On 25/06/2024 03:50, Jamin Lin via wrote:
> >>>>> Coverity reports a possible DIVIDE_BY_ZERO issue regarding the
> >>>>> "ram_size" object property. This can not happen because RAM has
> >>>>> predefined valid sizes per SoC. Nevertheless, add a test to close
> >>>>> the issue.
> >>>>>
> >>>>> Fixes: Coverity CID 1547113
> >>>>> Signed-off-by: Jamin Lin <[e-mail]>
> >>>>> Reviewed-by: Cédric Le Goater <[e-mail]> [ clg: Rewrote
> >>>>> commit log ]
> >>>>> Signed-off-by: Cédric Le Goater <[e-mail]>
> >>>>> ---
> >>>>>    hw/arm/aspeed_ast27x0.c | 6 ++++++
> >>>>>    1 file changed, 6 insertions(+)
> >>>>>
> >>>>> diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c
> >>>>> index b6876b4862..d14a46df6f 100644
> >>>>> --- a/hw/arm/aspeed_ast27x0.c
> >>>>> +++ b/hw/arm/aspeed_ast27x0.c
> >>>>> @@ -211,6 +211,12 @@ static void aspeed_ram_capacity_write(void
> >>>>> *opaque, hwaddr addr, uint64_t data,
> >>>>>        ram_size = object_property_get_uint(OBJECT(&s->sdmc),
> >>>>> "ram-size",
> >>>>>                                            &erro
> r_a
> >> bort);
> >>>>> +    if (!ram_size) {
> >>>>> +        qemu_log_mask(LOG_GUEST_ERROR,
> >>>>> +                      "%s: ram_size is zero",  __func__);
> >>>>> +        return;
> >>>>> +    }
> >>>>> +
> >>>> If we are sure that the error cannot happen, shouldn't we assert
> >>>> instead?
> >>>
> >>> Yes. That is what Peter suggested. This needs to be changed.
> >>>
> > Thanks for review and suggestion.
> > How about this change?
> >
> > assert(ram_size > 0);
> 
> yes.
> 
> I will send another patch fixing a long standing issue in the SDMC model not
> checking the ram_size value in the realize handler. It relies on the "ram-size"
> property being set.
> 
> Thanks,
> 
Will send v3 patch and thanks for your review and help.
Jamin

> C.
> 
> 
> > If you agree, I will send v3 patch.
> > Thanks-Jamin
> >
> >>>
> >>> Thanks,
> >>>
> >>> C.
> >>>
> >> Ok fine, I didn't see the message, sorry!
> >>
> >> Thanks
> >>
> >>   >cmd
> >>
> >>>
> >>>
> >>>>>        /*
> >>>>>         * Emulate ddr capacity hardware behavior.
> >>>>>         * If writes the data to the address which is beyond the
> >>>>> ram size,
> >>>

Patch

diff --git a/hw/arm/aspeed_ast27x0.c b/hw/arm/aspeed_ast27x0.c
index b6876b4862..d14a46df6f 100644
--- a/hw/arm/aspeed_ast27x0.c
+++ b/hw/arm/aspeed_ast27x0.c
@@ -211,6 +211,12 @@  static void aspeed_ram_capacity_write(void *opaque, hwaddr addr, uint64_t data,
     ram_size = object_property_get_uint(OBJECT(&s->sdmc), "ram-size",
                                         &error_abort);
 
+    if (!ram_size) {
+        qemu_log_mask(LOG_GUEST_ERROR,
+                      "%s: ram_size is zero",  __func__);
+        return;
+    }
+
     /*
      * Emulate ddr capacity hardware behavior.
      * If writes the data to the address which is beyond the ram size,