linux-kernel - Re: [PATCH] vmci_host: use smp_load_acquire/smp_store_release when accessing vmci_host_dev->ct

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] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 22 Nov 2023 14:34:55 +0000
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Yewon Choi <woni9911@...il.com>
Cc:     Bryan Tan <bryantan@...are.com>, Vishnu Dasa <vdasa@...are.com>,
        VMware PV-Drivers Reviewers <pv-drivers@...are.com>,
        Arnd Bergmann <arnd@...db.de>, linux-kernel@...r.kernel.org,
        threeearcat@...il.com
Subject: Re: [PATCH] vmci_host: use smp_load_acquire/smp_store_release when
 accessing vmci_host_dev->ct_type

On Wed, Nov 22, 2023 at 09:20:08PM +0900, Yewon Choi wrote:
> In vmci_host.c, missing memory barrier between vmci_host_dev->ct_type
> and vmci_host_dev->context may cause uninitialized data access.
> 
> One of possible execution flows is as follows:
> 
> CPU 1 (vmci_host_do_init_context)
> =====
> vmci_host_dev->context = vmci_ctx_create(...) // 1
> vmci_host_dev->ct_type = VMCIOBJ_CONTEXT; // 2
> 
> CPU 2 (vmci_host_poll)
> =====
> if (vmci_host_dev->ct_type == VMCIOBJ_CONTEXT) { // 3
> 	context = vmci_host_dev->context; // 4
> 	poll_wait(..., &context->host_context.wait_queue, ...);
> 
> While ct_type serves as a flag indicating that context is initialized,
> there is no memory barrier which prevents reordering between
> 1,2 and 3, 4. So it is possible that 4 reads uninitialized
> vmci_host_dev->context.
> In this case, the null dereference occurs in poll_wait().
> 
> In order to prevent this kind of reordering, we change plain accesses
> to ct_type into smp_load_acquire() and smp_store_release().
> 
> Signed-off-by: Yewon Choi <woni9911@...il.com>
> ---
>  drivers/misc/vmw_vmci/vmci_host.c | 40 ++++++++++++++++++-------------
>  1 file changed, 23 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/misc/vmw_vmci/vmci_host.c b/drivers/misc/vmw_vmci/vmci_host.c
> index abe79f6fd2a7..e83b6e0fe55b 100644
> --- a/drivers/misc/vmw_vmci/vmci_host.c
> +++ b/drivers/misc/vmw_vmci/vmci_host.c
> @@ -139,7 +139,7 @@ static int vmci_host_close(struct inode *inode, struct file *filp)
>  {
>  	struct vmci_host_dev *vmci_host_dev = filp->private_data;
>  
> -	if (vmci_host_dev->ct_type == VMCIOBJ_CONTEXT) {
> +	if (smp_load_acquire(&vmci_host_dev->ct_type) == VMCIOBJ_CONTEXT) {

This is getting tricky, why not use a normal lock to ensure that all is
safe?  close isn't on a "fast path", so this shouldn't be a speed issue,
right?

thanks,

greg k-h