LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH v2] VMCI: fix NULL pointer dereference when unmapping queue pair
@ 2021-08-18 12:48 Wang Hai
  2021-08-19 12:31 ` Jorgen Hansen
  0 siblings, 1 reply; 2+ messages in thread
From: Wang Hai @ 2021-08-18 12:48 UTC (permalink / raw)
  To: jhansen, vdasa, arnd, gregkh, dtor, georgezhang, acking
  Cc: pv-drivers, linux-kernel

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


^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [PATCH v2] VMCI: fix NULL pointer dereference when unmapping queue pair
  2021-08-18 12:48 [PATCH v2] VMCI: fix NULL pointer dereference when unmapping queue pair Wang Hai
@ 2021-08-19 12:31 ` Jorgen Hansen
  0 siblings, 0 replies; 2+ messages in thread
From: Jorgen Hansen @ 2021-08-19 12:31 UTC (permalink / raw)
  To: Wang Hai
  Cc: Vishnu Dasa, arnd, Greg Kroah-Hartman, dtor, georgezhang, acking,
	Pv-drivers, linux-kernel



> 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>


^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2021-08-19 12:31 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-18 12:48 [PATCH v2] VMCI: fix NULL pointer dereference when unmapping queue pair Wang Hai
2021-08-19 12:31 ` Jorgen Hansen

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).