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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Thu, 19 Aug 2021 12:31:31 +0000
From:   Jorgen Hansen <jhansen@...are.com>
To:     Wang Hai <wanghai38@...wei.com>
CC:     Vishnu Dasa <vdasa@...are.com>, "arnd@...db.de" <arnd@...db.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "dtor@...are.com" <dtor@...are.com>,
        "georgezhang@...are.com" <georgezhang@...are.com>,
        "acking@...are.com" <acking@...are.com>,
        Pv-drivers <Pv-drivers@...are.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] VMCI: fix NULL pointer dereference when unmapping
 queue pair



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