[PATCH v1 0/2] Fix coverity issues for AST2700

[PATCH v1 0/2] Fix coverity issues for AST2700

[Jamin Lin via]

change from v1:
aspeed/soc: coverity defect: DIVIDE_BY_ZERO
aspeed/sdmc: coverity defect: Control flow issues (DEADCODE)

Jamin Lin (2):
  aspeed/soc: fix coverity issue
  aspeed/sdmc: fix coverity issue

 hw/arm/aspeed_ast27x0.c | 6 ++++++
 hw/misc/aspeed_sdmc.c   | 1 -
 2 files changed, 6 insertions(+), 1 deletion(-)

-- 
2.34.1

[PATCH v1 1/2] aspeed/soc: fix coverity issue

[Jamin Lin via]

Fix coverity defect: DIVIDE_BY_ZERO.

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 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;
+    }
+
     /*
      * Emulate ddr capacity hardware behavior.
      * If writes the data to the address which is beyond the ram size,
-- 
2.34.1

Re: [PATCH v1 1/2] aspeed/soc: fix coverity issue

[Cédric Le Goater]

On 6/19/24 11:35 AM, Jamin Lin wrote:
> Fix coverity defect: DIVIDE_BY_ZERO.
> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>


I have rewritten the commit log :

     aspeed/soc: Fix possible divide by zero
     
     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

with that,

Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.



> ---
>   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;
> +    }
> +
>       /*
>        * Emulate ddr capacity hardware behavior.
>        * If writes the data to the address which is beyond the ram size,

Re: [PATCH v1 1/2] aspeed/soc: fix coverity issue

[Peter Maydell]

On Wed, 19 Jun 2024 at 10:35, Jamin Lin <jamin_lin@aspeedtech.com> wrote:
>
> Fix coverity defect: DIVIDE_BY_ZERO.
>
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> ---
>  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;
> +    }
> +

Isn't this a QEMU bug rather than a guest error? The
RAM size presumably should never be zero unless the board
set the ram-size property on the SDMC incorrectly. So the
SDMC device should check (and return an error from its realize
method) that the ram-size property is valid, and then here
we can just assert(ram_size != 0).

thanks
-- PMM

Re: [PATCH v1 1/2] aspeed/soc: fix coverity issue

[Cédric Le Goater]

On 6/24/24 2:18 PM, Peter Maydell wrote:
> On Wed, 19 Jun 2024 at 10:35, Jamin Lin <jamin_lin@aspeedtech.com> wrote:
>>
>> Fix coverity defect: DIVIDE_BY_ZERO.
>>
>> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
>> ---
>>   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;
>> +    }
>> +
> 
> Isn't this a QEMU bug rather than a guest error? The
> RAM size presumably should never be zero unless the board
> set the ram-size property on the SDMC incorrectly. So the
> SDMC device should check (and return an error from its realize
> method) that the ram-size property is valid, 

That's the case in aspeed_sdmc_set_ram_size() which is called from
the aspeed machine init routine when the ram size is set.

Setting the machine ram size to zero on the command line doesn't
report an error though and the size is the default.

> and then here we can just assert(ram_size != 0).

Yes.

Jamin, could you please send a v2 with the commit logs update
I proposed ? See the patches on my aspeed-9.1 branch.

Thanks,

C.

Re: [PATCH v1 1/2] aspeed/soc: fix coverity issue

[Peter Maydell]

On Mon, 24 Jun 2024 at 14:58, Cédric Le Goater <clg@kaod.org> wrote:
>
> On 6/24/24 2:18 PM, Peter Maydell wrote:
> > On Wed, 19 Jun 2024 at 10:35, Jamin Lin <jamin_lin@aspeedtech.com> wrote:
> >>
> >> Fix coverity defect: DIVIDE_BY_ZERO.
> >>
> >> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> >> ---
> >>   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;
> >> +    }
> >> +
> >
> > Isn't this a QEMU bug rather than a guest error? The
> > RAM size presumably should never be zero unless the board
> > set the ram-size property on the SDMC incorrectly. So the
> > SDMC device should check (and return an error from its realize
> > method) that the ram-size property is valid,
>
> That's the case in aspeed_sdmc_set_ram_size() which is called from
> the aspeed machine init routine when the ram size is set.

True, but if the property is never set at all then the
struct field will be left at whatever value it had, which
is 0, I think. So if that's not valid then it either needs
to be a different default or else the realize method should
complain that the property was never set.

thanks
-- PMM

Re: [PATCH v1 1/2] aspeed/soc: fix coverity issue

[Cédric Le Goater]

On 6/24/24 4:01 PM, Peter Maydell wrote:
> On Mon, 24 Jun 2024 at 14:58, Cédric Le Goater <clg@kaod.org> wrote:
>>
>> On 6/24/24 2:18 PM, Peter Maydell wrote:
>>> On Wed, 19 Jun 2024 at 10:35, Jamin Lin <jamin_lin@aspeedtech.com> wrote:
>>>>
>>>> Fix coverity defect: DIVIDE_BY_ZERO.
>>>>
>>>> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
>>>> ---
>>>>    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;
>>>> +    }
>>>> +
>>>
>>> Isn't this a QEMU bug rather than a guest error? The
>>> RAM size presumably should never be zero unless the board
>>> set the ram-size property on the SDMC incorrectly. So the
>>> SDMC device should check (and return an error from its realize
>>> method) that the ram-size property is valid,
>>
>> That's the case in aspeed_sdmc_set_ram_size() which is called from
>> the aspeed machine init routine when the ram size is set.
> 
> True, but if the property is never set at all then the
> struct field will be left at whatever value it had, which
> is 0, I think. So if that's not valid then it either needs
> to be a different default or else the realize method should
> complain that the property was never set.

Ah, yes, and the issue has been there for while. I will send a separate
patch for this.

Thanks,

C.

RE: [PATCH v1 1/2] aspeed/soc: fix coverity issue

[Jamin Lin]

Hi Cedric,
> -----Original Message-----
> From: Cédric Le Goater <clg@kaod.org>
> Sent: Monday, June 24, 2024 9:58 PM
> To: Peter Maydell <peter.maydell@linaro.org>; Jamin Lin
> <jamin_lin@aspeedtech.com>
> Cc: Steven Lee <steven_lee@aspeedtech.com>; Troy Lee
> <leetroy@gmail.com>; Andrew Jeffery <andrew@codeconstruct.com.au>; Joel
> Stanley <joel@jms.id.au>; open list:ASPEED BMCs <qemu-arm@nongnu.org>;
> open list:All patches CC here <qemu-devel@nongnu.org>; Troy Lee
> <troy_lee@aspeedtech.com>; Yunlin Tang <yunlin.tang@aspeedtech.com>
> Subject: Re: [PATCH v1 1/2] aspeed/soc: fix coverity issue
> 
> On 6/24/24 2:18 PM, Peter Maydell wrote:
> > On Wed, 19 Jun 2024 at 10:35, Jamin Lin <jamin_lin@aspeedtech.com>
> wrote:
> >>
> >> Fix coverity defect: DIVIDE_BY_ZERO.
> >>
> >> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> >> ---
> >>   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;
> >> +    }
> >> +
> >
> > Isn't this a QEMU bug rather than a guest error? The RAM size
> > presumably should never be zero unless the board set the ram-size
> > property on the SDMC incorrectly. So the SDMC device should check (and
> > return an error from its realize
> > method) that the ram-size property is valid,
> 
> That's the case in aspeed_sdmc_set_ram_size() which is called from the
> aspeed machine init routine when the ram size is set.
> 
> Setting the machine ram size to zero on the command line doesn't report an
> error though and the size is the default.
> 
> > and then here we can just assert(ram_size != 0).
> 
> Yes.
> 
> Jamin, could you please send a v2 with the commit logs update I proposed ?
> See the patches on my aspeed-9.1 branch.
I resend v2 patch with your commit log, https://www.mail-archive.com/qemu-devel@nongnu.org/msg1050302.html
Do we need to drop this patch, https://www.mail-archive.com/qemu-devel@nongnu.org/msg1050301.html? 

Thanks-Jamin
> 
> Thanks,
> 
> C.

Re: [PATCH v1 1/2] aspeed/soc: fix coverity issue

[Cédric Le Goater]

Hello

On 6/25/24 3:55 AM, Jamin Lin wrote:
> Hi Cedric,
>> -----Original Message-----
>> From: Cédric Le Goater <clg@kaod.org>
>> Sent: Monday, June 24, 2024 9:58 PM
>> To: Peter Maydell <peter.maydell@linaro.org>; Jamin Lin
>> <jamin_lin@aspeedtech.com>
>> Cc: Steven Lee <steven_lee@aspeedtech.com>; Troy Lee
>> <leetroy@gmail.com>; Andrew Jeffery <andrew@codeconstruct.com.au>; Joel
>> Stanley <joel@jms.id.au>; open list:ASPEED BMCs <qemu-arm@nongnu.org>;
>> open list:All patches CC here <qemu-devel@nongnu.org>; Troy Lee
>> <troy_lee@aspeedtech.com>; Yunlin Tang <yunlin.tang@aspeedtech.com>
>> Subject: Re: [PATCH v1 1/2] aspeed/soc: fix coverity issue
>>
>> On 6/24/24 2:18 PM, Peter Maydell wrote:
>>> On Wed, 19 Jun 2024 at 10:35, Jamin Lin <jamin_lin@aspeedtech.com>
>> wrote:
>>>>
>>>> Fix coverity defect: DIVIDE_BY_ZERO.
>>>>
>>>> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
>>>> ---
>>>>    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;
>>>> +    }
>>>> +
>>>
>>> Isn't this a QEMU bug rather than a guest error? The RAM size
>>> presumably should never be zero unless the board set the ram-size
>>> property on the SDMC incorrectly. So the SDMC device should check (and
>>> return an error from its realize
>>> method) that the ram-size property is valid,
>>
>> That's the case in aspeed_sdmc_set_ram_size() which is called from the
>> aspeed machine init routine when the ram size is set.
>>
>> Setting the machine ram size to zero on the command line doesn't report an
>> error though and the size is the default.
>>
>>> and then here we can just assert(ram_size != 0).
>>
>> Yes.
>>
>> Jamin, could you please send a v2 with the commit logs update I proposed ?
>> See the patches on my aspeed-9.1 branch.
> I resend v2 patch with your commit log, https://www.mail-archive.com/qemu-devel@nongnu.org/msg1050302.html

We'll need a v3 with the assert. No hurries. We still have some time
before the soft freeze.

Thanks,

C.




> Do we need to drop this patch, https://www.mail-archive.com/qemu-devel@nongnu.org/msg1050301.html?
> 
> Thanks-Jamin
>>
>> Thanks,
>>
>> C.

[PATCH v1 2/2] aspeed/sdmc: fix coverity issue

[Jamin Lin via]

Fix coverity defect: Control flow issues (DEADCODE)

Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
---
 hw/misc/aspeed_sdmc.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
index 93e2e29ead..94eed9264d 100644
--- a/hw/misc/aspeed_sdmc.c
+++ b/hw/misc/aspeed_sdmc.c
@@ -589,7 +589,6 @@ static void aspeed_2700_sdmc_write(AspeedSDMCState *s, uint32_t reg,
     case R_INT_STATUS:
     case R_INT_CLEAR:
     case R_INT_MASK:
-    case R_MAIN_STATUS:
     case R_ERR_STATUS:
     case R_ECC_FAIL_STATUS:
     case R_ECC_FAIL_ADDR:
-- 
2.34.1

Re: [PATCH v1 2/2] aspeed/sdmc: fix coverity issue

[Cédric Le Goater]

On 6/19/24 11:35 AM, Jamin Lin wrote:
> Fix coverity defect: Control flow issues (DEADCODE)

     aspeed/sdmc: Remove extra R_MAIN_STATUS case
     
     Coverity reports that the newly added 'case R_MAIN_STATUS' is DEADCODE
     because it can not be reached. This is because R_MAIN_STATUS is handled
     before in the "Unprotected registers" switch statement. Remove it.
     
     Fixes: Coverity CID 1547112
     
with that,


Reviewed-by: Cédric Le Goater <clg@redhat.com>

Thanks,

C.



> 
> Signed-off-by: Jamin Lin <jamin_lin@aspeedtech.com>
> ---
>   hw/misc/aspeed_sdmc.c | 1 -
>   1 file changed, 1 deletion(-)
> 
> diff --git a/hw/misc/aspeed_sdmc.c b/hw/misc/aspeed_sdmc.c
> index 93e2e29ead..94eed9264d 100644
> --- a/hw/misc/aspeed_sdmc.c
> +++ b/hw/misc/aspeed_sdmc.c
> @@ -589,7 +589,6 @@ static void aspeed_2700_sdmc_write(AspeedSDMCState *s, uint32_t reg,
>       case R_INT_STATUS:
>       case R_INT_CLEAR:
>       case R_INT_MASK:
> -    case R_MAIN_STATUS:
>       case R_ERR_STATUS:
>       case R_ECC_FAIL_STATUS:
>       case R_ECC_FAIL_ADDR: