Re: [PATCH] hw/s390x: prevent potential NULL dereference

["Philippe Mathieu-Daudé" <philmd@linaro.org>]

Hi Oleg,

On 29/5/24 13:36, Oleg Sviridov wrote:
> Pointer, returned from function 's390_ipl_get_iplb_pv', may be NULL and is dereferenced immediately after.
> 
> Found by Linux Verification Center (linuxtesting.org) with SVACE.
> 
> Signed-off-by: Oleg Sviridov <oleg.sviridov@red-soft.ru>
> ---
>   hw/s390x/ipl.c | 11 ++++++++++-
>   1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/s390x/ipl.c b/hw/s390x/ipl.c
> index e934bf89d1..2fa6a340a1 100644
> --- a/hw/s390x/ipl.c
> +++ b/hw/s390x/ipl.c
> @@ -706,9 +706,14 @@ int s390_ipl_prepare_pv_header(Error **errp)
>   {
>       IplParameterBlock *ipib = s390_ipl_get_iplb_pv();
>       IPLBlockPV *ipib_pv = &ipib->pv;

I suppose ipib_pv being NULL here is a bug elsewhere. You should
add here:

        assert(ipib_pv);

and look at the backtrace or audit the code.

> -    void *hdr = g_malloc(ipib_pv->pv_header_len);
> +    void *hdr;
>       int rc;
>   
> +    if (!ipib_pv) {

(If this is a legit error you have to set errp here before returning).

> +        return -1;
> +    }
> +
> +    hdr = g_malloc(ipib_pv->pv_header_len);
>       cpu_physical_memory_read(ipib_pv->pv_header_addr, hdr,
>                                ipib_pv->pv_header_len);
>       rc = s390_pv_set_sec_parms((uintptr_t)hdr, ipib_pv->pv_header_len, errp);
> @@ -722,6 +727,10 @@ int s390_ipl_pv_unpack(void)
>       IPLBlockPV *ipib_pv = &ipib->pv;
>       int i, rc = 0;
>   

Ditto assert.

> +    if (!ipib_pv) {
> +        return -1;
> +    }
> +
>       for (i = 0; i < ipib_pv->num_comp; i++) {
>           rc = s390_pv_unpack(ipib_pv->components[i].addr,
>                               TARGET_PAGE_ALIGN(ipib_pv->components[i].size),