Re: [PATCH v2] VMCI: fix NULL pointer dereference when unmapping queue pair

[Jorgen Hansen <jhansen@vmware.com>]

> On 18 Aug 2021, at 14:48, Wang Hai <wanghai38@huawei.com> wrote:
> 
> I got a NULL pointer dereference report when doing fuzz test:
> 
> Call Trace:
>  qp_release_pages+0xae/0x130
>  qp_host_unregister_user_memory.isra.25+0x2d/0x80
>  vmci_qp_broker_unmap+0x191/0x320
>  ? vmci_host_do_alloc_queuepair.isra.9+0x1c0/0x1c0
>  vmci_host_unlocked_ioctl+0x59f/0xd50
>  ? do_vfs_ioctl+0x14b/0xa10
>  ? tomoyo_file_ioctl+0x28/0x30
>  ? vmci_host_do_alloc_queuepair.isra.9+0x1c0/0x1c0
>  __x64_sys_ioctl+0xea/0x120
>  do_syscall_64+0x34/0xb0
>  entry_SYSCALL_64_after_hwframe+0x44/0xae
> 
> When a queue pair is created by the following call, it will not
> register the user memory if the page_store is NULL, and the
> entry->state will be set to VMCIQPB_CREATED_NO_MEM.
> 
> vmci_host_unlocked_ioctl
>  vmci_host_do_alloc_queuepair
>    vmci_qp_broker_alloc
>      qp_broker_alloc
>        qp_broker_create // set entry->state = VMCIQPB_CREATED_NO_MEM;
> 
> When unmapping this queue pair, qp_host_unregister_user_memory() will
> be called to unregister the non-existent user memory, which will
> result in a null pointer reference. It will also change
> VMCIQPB_CREATED_NO_MEM to VMCIQPB_CREATED_MEM, which should not be
> present in this operation.
> 
> Only when the qp broker has mem, it can unregister the user
> memory when unmapping the qp broker.
> 
> Only when the qp broker has no mem, it can register the user
> memory when mapping the qp broker.
> 
> Fixes: 06164d2b72aa ("VMCI: queue pairs implementation.")
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Signed-off-by: Wang Hai <wanghai38@huawei.com>
> ---
> v1->v2: fix bug in vmci_qp_broker_map
> drivers/misc/vmw_vmci/vmci_queue_pair.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/misc/vmw_vmci/vmci_queue_pair.c b/drivers/misc/vmw_vmci/vmci_queue_pair.c
> index 880c33ab9f47..94ebf7f3fd58 100644
> --- a/drivers/misc/vmw_vmci/vmci_queue_pair.c
> +++ b/drivers/misc/vmw_vmci/vmci_queue_pair.c
> @@ -2243,7 +2243,8 @@ int vmci_qp_broker_map(struct vmci_handle handle,
> 
> 	result = VMCI_SUCCESS;
> 
> -	if (context_id != VMCI_HOST_CONTEXT_ID) {
> +	if (context_id != VMCI_HOST_CONTEXT_ID &&
> +	    !QPBROKERSTATE_HAS_MEM(entry)) {
> 		struct vmci_qp_page_store page_store;
> 
> 		page_store.pages = guest_mem;
> @@ -2350,7 +2351,8 @@ int vmci_qp_broker_unmap(struct vmci_handle handle,
> 		goto out;
> 	}
> 
> -	if (context_id != VMCI_HOST_CONTEXT_ID) {
> +	if (context_id != VMCI_HOST_CONTEXT_ID &&
> +	    QPBROKERSTATE_HAS_MEM(entry)) {
> 		qp_acquire_queue_mutex(entry->produce_q);
> 		result = qp_save_headers(entry);
> 		if (result < VMCI_SUCCESS)
> — 
> 2.17.1
> 

Thanks a lot for finding and fixing this. The map and unmap routines are used - among other
things - during virtual machine snapshot. I tested snapshots for VMware Workstation with your
patch, and things still work fine.

Reviewed-by:  Jorgen Hansen <jhansen@vmware.com>