[PATCH v1 1/1] hw/riscv/boot: Check the error of fdt_pack()

[PATCH v1 1/1] hw/riscv/boot: Check the error of fdt_pack()

[Alistair Francis]

Coverity reports that we don't check the error result of fdt_pack(), so
let's save the result and assert that it is 0.

Fixes: Coverity CID 1458136
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 hw/riscv/boot.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index 0d38bb7426..25406a3f67 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -182,7 +182,7 @@ uint32_t riscv_load_fdt(hwaddr dram_base, uint64_t mem_size, void *fdt)
 {
     uint32_t temp, fdt_addr;
     hwaddr dram_end = dram_base + mem_size;
-    int fdtsize = fdt_totalsize(fdt);
+    int ret, fdtsize = fdt_totalsize(fdt);
 
     if (fdtsize <= 0) {
         error_report("invalid device-tree");
@@ -198,7 +198,8 @@ uint32_t riscv_load_fdt(hwaddr dram_base, uint64_t mem_size, void *fdt)
     temp = MIN(dram_end, 3072 * MiB);
     fdt_addr = QEMU_ALIGN_DOWN(temp - fdtsize, 16 * MiB);
 
-    fdt_pack(fdt);
+    ret = fdt_pack(fdt);
+    g_assert(ret == 0);
     /* copy in the device tree */
     qemu_fdt_dumpdtb(fdt, fdtsize);
 
-- 
2.31.1

[PATCH v1 1/1] hw/riscv/boot: Check the error of fdt_pack()

[Alistair Francis]

Coverity reports that we don't check the error result of fdt_pack(), so
let's save the result and assert that it is 0.

Fixes: Coverity CID 1458136
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
---
 hw/riscv/boot.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
index 0d38bb7426..25406a3f67 100644
--- a/hw/riscv/boot.c
+++ b/hw/riscv/boot.c
@@ -182,7 +182,7 @@ uint32_t riscv_load_fdt(hwaddr dram_base, uint64_t mem_size, void *fdt)
 {
     uint32_t temp, fdt_addr;
     hwaddr dram_end = dram_base + mem_size;
-    int fdtsize = fdt_totalsize(fdt);
+    int ret, fdtsize = fdt_totalsize(fdt);
 
     if (fdtsize <= 0) {
         error_report("invalid device-tree");
@@ -198,7 +198,8 @@ uint32_t riscv_load_fdt(hwaddr dram_base, uint64_t mem_size, void *fdt)
     temp = MIN(dram_end, 3072 * MiB);
     fdt_addr = QEMU_ALIGN_DOWN(temp - fdtsize, 16 * MiB);
 
-    fdt_pack(fdt);
+    ret = fdt_pack(fdt);
+    g_assert(ret == 0);
     /* copy in the device tree */
     qemu_fdt_dumpdtb(fdt, fdtsize);
 
-- 
2.31.1

Re: [PATCH v1 1/1] hw/riscv/boot: Check the error of fdt_pack()

[Bin Meng]

On Wed, Jul 14, 2021 at 3:22 PM Alistair Francis
<alistair.francis@wdc.com> wrote:
>
> Coverity reports that we don't check the error result of fdt_pack(), so
> let's save the result and assert that it is 0.
>
> Fixes: Coverity CID 1458136
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  hw/riscv/boot.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Re: [PATCH v1 1/1] hw/riscv/boot: Check the error of fdt_pack()

[Bin Meng]

On Wed, Jul 14, 2021 at 3:22 PM Alistair Francis
<alistair.francis@wdc.com> wrote:
>
> Coverity reports that we don't check the error result of fdt_pack(), so
> let's save the result and assert that it is 0.
>
> Fixes: Coverity CID 1458136
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  hw/riscv/boot.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>

Reviewed-by: Bin Meng <bmeng.cn@gmail.com>

Re: [PATCH v1 1/1] hw/riscv/boot: Check the error of fdt_pack()

[Peter Maydell]

On Wed, 14 Jul 2021 at 08:22, Alistair Francis <alistair.francis@wdc.com> wrote:
>
> Coverity reports that we don't check the error result of fdt_pack(), so
> let's save the result and assert that it is 0.
>
> Fixes: Coverity CID 1458136
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  hw/riscv/boot.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index 0d38bb7426..25406a3f67 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -182,7 +182,7 @@ uint32_t riscv_load_fdt(hwaddr dram_base, uint64_t mem_size, void *fdt)
>  {
>      uint32_t temp, fdt_addr;
>      hwaddr dram_end = dram_base + mem_size;
> -    int fdtsize = fdt_totalsize(fdt);
> +    int ret, fdtsize = fdt_totalsize(fdt);
>
>      if (fdtsize <= 0) {
>          error_report("invalid device-tree");
> @@ -198,7 +198,8 @@ uint32_t riscv_load_fdt(hwaddr dram_base, uint64_t mem_size, void *fdt)
>      temp = MIN(dram_end, 3072 * MiB);
>      fdt_addr = QEMU_ALIGN_DOWN(temp - fdtsize, 16 * MiB);
>
> -    fdt_pack(fdt);
> +    ret = fdt_pack(fdt);
> +    g_assert(ret == 0);
>      /* copy in the device tree */
>      qemu_fdt_dumpdtb(fdt, fdtsize);

Are we in the same situation as spapr.c where we believe the call
will only fail if the fdt was corrupt somehow? If so, it would be
nice to also have the comment from spapr.c:
        /* Should only fail if we've built a corrupted tree */

thanks
-- PMM

Re: [PATCH v1 1/1] hw/riscv/boot: Check the error of fdt_pack()

[Peter Maydell]

On Wed, 14 Jul 2021 at 08:22, Alistair Francis <alistair.francis@wdc.com> wrote:
>
> Coverity reports that we don't check the error result of fdt_pack(), so
> let's save the result and assert that it is 0.
>
> Fixes: Coverity CID 1458136
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
>  hw/riscv/boot.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> index 0d38bb7426..25406a3f67 100644
> --- a/hw/riscv/boot.c
> +++ b/hw/riscv/boot.c
> @@ -182,7 +182,7 @@ uint32_t riscv_load_fdt(hwaddr dram_base, uint64_t mem_size, void *fdt)
>  {
>      uint32_t temp, fdt_addr;
>      hwaddr dram_end = dram_base + mem_size;
> -    int fdtsize = fdt_totalsize(fdt);
> +    int ret, fdtsize = fdt_totalsize(fdt);
>
>      if (fdtsize <= 0) {
>          error_report("invalid device-tree");
> @@ -198,7 +198,8 @@ uint32_t riscv_load_fdt(hwaddr dram_base, uint64_t mem_size, void *fdt)
>      temp = MIN(dram_end, 3072 * MiB);
>      fdt_addr = QEMU_ALIGN_DOWN(temp - fdtsize, 16 * MiB);
>
> -    fdt_pack(fdt);
> +    ret = fdt_pack(fdt);
> +    g_assert(ret == 0);
>      /* copy in the device tree */
>      qemu_fdt_dumpdtb(fdt, fdtsize);

Are we in the same situation as spapr.c where we believe the call
will only fail if the fdt was corrupt somehow? If so, it would be
nice to also have the comment from spapr.c:
        /* Should only fail if we've built a corrupted tree */

thanks
-- PMM

Re: [PATCH v1 1/1] hw/riscv/boot: Check the error of fdt_pack()

[Alistair Francis]

On Wed, Jul 14, 2021 at 6:46 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Wed, 14 Jul 2021 at 08:22, Alistair Francis <alistair.francis@wdc.com> wrote:
> >
> > Coverity reports that we don't check the error result of fdt_pack(), so
> > let's save the result and assert that it is 0.
> >
> > Fixes: Coverity CID 1458136
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> >  hw/riscv/boot.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> > index 0d38bb7426..25406a3f67 100644
> > --- a/hw/riscv/boot.c
> > +++ b/hw/riscv/boot.c
> > @@ -182,7 +182,7 @@ uint32_t riscv_load_fdt(hwaddr dram_base, uint64_t mem_size, void *fdt)
> >  {
> >      uint32_t temp, fdt_addr;
> >      hwaddr dram_end = dram_base + mem_size;
> > -    int fdtsize = fdt_totalsize(fdt);
> > +    int ret, fdtsize = fdt_totalsize(fdt);
> >
> >      if (fdtsize <= 0) {
> >          error_report("invalid device-tree");
> > @@ -198,7 +198,8 @@ uint32_t riscv_load_fdt(hwaddr dram_base, uint64_t mem_size, void *fdt)
> >      temp = MIN(dram_end, 3072 * MiB);
> >      fdt_addr = QEMU_ALIGN_DOWN(temp - fdtsize, 16 * MiB);
> >
> > -    fdt_pack(fdt);
> > +    ret = fdt_pack(fdt);
> > +    g_assert(ret == 0);
> >      /* copy in the device tree */
> >      qemu_fdt_dumpdtb(fdt, fdtsize);
>
> Are we in the same situation as spapr.c where we believe the call
> will only fail if the fdt was corrupt somehow? If so, it would be
> nice to also have the comment from spapr.c:
>         /* Should only fail if we've built a corrupted tree */

Yes, we are the same. I added the comment.

Alistair

>
> thanks
> -- PMM

Re: [PATCH v1 1/1] hw/riscv/boot: Check the error of fdt_pack()

[Alistair Francis]

On Wed, Jul 14, 2021 at 6:46 PM Peter Maydell <peter.maydell@linaro.org> wrote:
>
> On Wed, 14 Jul 2021 at 08:22, Alistair Francis <alistair.francis@wdc.com> wrote:
> >
> > Coverity reports that we don't check the error result of fdt_pack(), so
> > let's save the result and assert that it is 0.
> >
> > Fixes: Coverity CID 1458136
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > ---
> >  hw/riscv/boot.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/hw/riscv/boot.c b/hw/riscv/boot.c
> > index 0d38bb7426..25406a3f67 100644
> > --- a/hw/riscv/boot.c
> > +++ b/hw/riscv/boot.c
> > @@ -182,7 +182,7 @@ uint32_t riscv_load_fdt(hwaddr dram_base, uint64_t mem_size, void *fdt)
> >  {
> >      uint32_t temp, fdt_addr;
> >      hwaddr dram_end = dram_base + mem_size;
> > -    int fdtsize = fdt_totalsize(fdt);
> > +    int ret, fdtsize = fdt_totalsize(fdt);
> >
> >      if (fdtsize <= 0) {
> >          error_report("invalid device-tree");
> > @@ -198,7 +198,8 @@ uint32_t riscv_load_fdt(hwaddr dram_base, uint64_t mem_size, void *fdt)
> >      temp = MIN(dram_end, 3072 * MiB);
> >      fdt_addr = QEMU_ALIGN_DOWN(temp - fdtsize, 16 * MiB);
> >
> > -    fdt_pack(fdt);
> > +    ret = fdt_pack(fdt);
> > +    g_assert(ret == 0);
> >      /* copy in the device tree */
> >      qemu_fdt_dumpdtb(fdt, fdtsize);
>
> Are we in the same situation as spapr.c where we believe the call
> will only fail if the fdt was corrupt somehow? If so, it would be
> nice to also have the comment from spapr.c:
>         /* Should only fail if we've built a corrupted tree */

Yes, we are the same. I added the comment.

Alistair

>
> thanks
> -- PMM