[PATCH] hyperv: add check for NULL for msg

[PATCH] hyperv: add check for NULL for msg

[Anastasia Belova]

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;
     }
-- 
2.30.2

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

[Maciej S. Szmigiero]

On 28.09.2023 15:25, Anastasia Belova wrote:
> 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>

Makes sense to me, thanks.

Did you run your static checker through the remaining QEMU files,
too?

I can see similar cpu_physical_memory_map() usage in, for example:
target/s390x/helper.c, hw/nvram/spapr_nvram.c, hw/hyperv/vmbus.c,
display/ramfb.c...

Thanks,
Maciej

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

[Анастасия Любимова]

28/09/23 19:18, Maciej S. Szmigiero пишет:
> On 28.09.2023 15:25, Anastasia Belova wrote:
>> 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>
>
> Makes sense to me, thanks.
>
> Did you run your static checker through the remaining QEMU files,
> too?
>
> I can see similar cpu_physical_memory_map() usage in, for example:
> target/s390x/helper.c, hw/nvram/spapr_nvram.c, hw/hyperv/vmbus.c,
> display/ramfb.c...
>
It seems that configurations for analysis do not contain these files
so the checker hasn't warned us. Additional time is needed to
analyze these pieces of code and form patches if necessary.

Anastasia Belova

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

[Maciej S. Szmigiero]

On 26.10.2023 11:31, Анастасия Любимова wrote:
> 
> 28/09/23 19:18, Maciej S. Szmigiero пишет:
>> On 28.09.2023 15:25, Anastasia Belova wrote:
>>> 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>
>>
>> Makes sense to me, thanks.
>>
>> Did you run your static checker through the remaining QEMU files,
>> too?
>>
>> I can see similar cpu_physical_memory_map() usage in, for example:
>> target/s390x/helper.c, hw/nvram/spapr_nvram.c, hw/hyperv/vmbus.c,
>> display/ramfb.c...
>>
> It seems that configurations for analysis do not contain these files
> so the checker hasn't warned us. Additional time is needed to
> analyze these pieces of code and form patches if necessary.

No problem, it's not an urgent issue.  
> Anastasia Belova

Thanks,
Maciej

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

[Alex Bennée]

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.

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.

-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro

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