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