Re: [PATCH 1/7] contrib/elf2dmp: Always check for PA resolution failure

[Peter Maydell]

On Sun, 3 Mar 2024 at 10:53, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> Not checking PA resolution failure can result in NULL deference.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
>  contrib/elf2dmp/addrspace.c | 46 
> ++++++++++++++++++++++++++++-----------------
>  1 file changed, 29 insertions(+), 17 deletions(-)
>
> diff --git a/contrib/elf2dmp/addrspace.c b/contrib/elf2dmp/addrspace.c
> index 6f608a517b1e..980a7aa5f8fb 100644
> --- a/contrib/elf2dmp/addrspace.c
> +++ b/contrib/elf2dmp/addrspace.c
> @@ -22,7 +22,7 @@ static struct pa_block *pa_space_find_block(struct pa_space 
> *ps, uint64_t pa)
>      return NULL;
>  }
>
> -static uint8_t *pa_space_resolve(struct pa_space *ps, uint64_t pa)
> +static void *pa_space_resolve(struct pa_space *ps, uint64_t pa)
>  {
>      struct pa_block *block = pa_space_find_block(ps, pa);
>
> @@ -33,6 +33,19 @@ static uint8_t *pa_space_resolve(struct pa_space *ps, 
> uint64_t pa)
>      return block->addr + (pa - block->paddr);
>  }
>
> +static int pa_space_read64(struct pa_space *ps, uint64_t pa, uint64_t *value)
> +{
> +    uint64_t *resolved = pa_space_resolve(ps, pa);
> +
> +    if (!resolved) {
> +        return 1;
> +    }
> +
> +    *value = *resolved;
> +
> +    return 0;
> +}

This is effectively returning a bool, so we could use a 'bool'
for the return type. I also think it would be preferable to
have the return type be 'true for success, false for failure':
in the callsites, having

   if (!get_pml4e(vs, va, &pml4e) || !is_present(pml4e)) {
          return INVALID_PA;
      }

seems to me to read more clearly ("if we couldn't get the PML,
or the PML isn't present, then...").

thanks
-- PMM