Re: [PATCH] hyperv: add check for NULL for msg

[Maciej S. Szmigiero]

On 28.09.2023 18:56, Alex Bennée wrote:
> Anastasia Belova <abelova@astralinux.ru> writes:
>
> > cpu_physical_memory_map may return NULL in hyperv_hcall_post_message.
> > Add check for NULL to avoid NULL-dereference.
> >
> > Found by Linux Verification Center (linuxtesting.org) with SVACE.
> >
> > Fixes: 76036a5fc7 ("hyperv: process POST_MESSAGE hypercall")
> > Signed-off-by: Anastasia Belova <abelova@astralinux.ru>
> > ---
> >   hw/hyperv/hyperv.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/hw/hyperv/hyperv.c b/hw/hyperv/hyperv.c
> > index 57b402b956..61c65d7329 100644
> > --- a/hw/hyperv/hyperv.c
> > +++ b/hw/hyperv/hyperv.c
> > @@ -588,7 +588,7 @@ uint16_t hyperv_hcall_post_message(uint64_t param, bool 
> > fast)
> >   
> >       len = sizeof(*msg);
> >       msg = cpu_physical_memory_map(param, &len, 0);
> > -    if (len < sizeof(*msg)) {
> > +    if (!msg || len < sizeof(*msg)) {
> >           ret = HV_STATUS_INSUFFICIENT_MEMORY;
> >           goto unmap;
>
> What is the failure path that returns NULL but leaves len untouched? I
> see in address_space_map():
>
>      if (!memory_access_is_direct(mr, is_write)) {
>          if (qatomic_xchg(&bounce.in_use, true)) {
>              *plen = 0;
>              return NULL;
>          }
>
> and in qemu_ram_ptr_length:
>
>      if (*size == 0) {
>          return NULL;
>      }
>
> but the other paths can't fail AFAICT.

Looks like at least xen_map_cache() can fail with NULL,
and this NULL can then be returned from qemu_ram_ptr_length().

In this case, the returned len would come from the return
value of flatview_extend_translation() call earlier.

To be fair, I am not sure how realistic this scenario is,
but most cpu_physical_memory_map() callers seem to check at
least its return value, if not return value AND the returned
length.

> That's not to say its a bad thing to verify the ptr before attempting a
> de-reference but I would like to understand the failure mode.
>
> As an aside it would also be nice to add (as a fresh commit) a kdoc
> comment for cpu_physical_memory_map() that documents the API.

This would be very nice indeed - to enshrine this function semantics
so there aren't future doubts what it can return.

Thanks,
Maciej