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

From: Jorgen Hansen
Date: Thu Aug 19 2021 - 08:31:36 EST




> On 18 Aug 2021, at 14:48, Wang Hai <wanghai38@xxxxxxxxxx> 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@xxxxxxxxxx>
> Signed-off-by: Wang Hai <wanghai38@xxxxxxxxxx>
> ---
> 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@xxxxxxxxxx>