From: Yewon Choi <woni9911@gmail.com>
To: Bryan Tan <bryantan@vmware.com>, Vishnu Dasa <vdasa@vmware.com>,
VMware PV-Drivers Reviewers <pv-drivers@vmware.com>,
Arnd Bergmann <arnd@arndb.de>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
linux-kernel@vger.kernel.org
Cc: threeearcat@gmail.com
Subject: [PATCH] vmci_host: use smp_load_acquire/smp_store_release when accessing vmci_host_dev->ct_type
Date: Wed, 22 Nov 2023 21:20:08 +0900 [thread overview]
Message-ID: <20231122122005.GA4661@libra05> (raw)
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@gmail.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) {
vmci_ctx_destroy(vmci_host_dev->context);
vmci_host_dev->context = NULL;
@@ -168,7 +168,7 @@ static __poll_t vmci_host_poll(struct file *filp, poll_table *wait)
struct vmci_ctx *context;
__poll_t mask = 0;
- if (vmci_host_dev->ct_type == VMCIOBJ_CONTEXT) {
+ if (smp_load_acquire(&vmci_host_dev->ct_type) == VMCIOBJ_CONTEXT) {
/*
* Read context only if ct_type == VMCIOBJ_CONTEXT to make
* sure that context is initialized
@@ -309,7 +309,7 @@ static int vmci_host_do_init_context(struct vmci_host_dev *vmci_host_dev,
mutex_lock(&vmci_host_dev->lock);
- if (vmci_host_dev->ct_type != VMCIOBJ_NOT_SET) {
+ if (smp_load_acquire(&vmci_host_dev->ct_type) != VMCIOBJ_NOT_SET) {
vmci_ioctl_err("received VMCI init on initialized handle\n");
retval = -EINVAL;
goto out;
@@ -346,7 +346,13 @@ static int vmci_host_do_init_context(struct vmci_host_dev *vmci_host_dev,
goto out;
}
- vmci_host_dev->ct_type = VMCIOBJ_CONTEXT;
+ /*
+ * Make sure that ct_type is written after
+ * vmci_host_dev->context is initialized.
+ *
+ * This pairs with smp_load_acquire() in vmci_host_XXX.
+ */
+ smp_store_release(&vmci_host_dev->ct_type, VMCIOBJ_CONTEXT);
atomic_inc(&vmci_host_active_users);
vmci_call_vsock_callback(true);
@@ -366,7 +372,7 @@ static int vmci_host_do_send_datagram(struct vmci_host_dev *vmci_host_dev,
struct vmci_datagram *dg = NULL;
u32 cid;
- if (vmci_host_dev->ct_type != VMCIOBJ_CONTEXT) {
+ if (smp_load_acquire(&vmci_host_dev->ct_type) != VMCIOBJ_CONTEXT) {
vmci_ioctl_err("only valid for contexts\n");
return -EINVAL;
}
@@ -422,7 +428,7 @@ static int vmci_host_do_receive_datagram(struct vmci_host_dev *vmci_host_dev,
int retval;
size_t size;
- if (vmci_host_dev->ct_type != VMCIOBJ_CONTEXT) {
+ if (smp_load_acquire(&vmci_host_dev->ct_type) != VMCIOBJ_CONTEXT) {
vmci_ioctl_err("only valid for contexts\n");
return -EINVAL;
}
@@ -453,7 +459,7 @@ static int vmci_host_do_alloc_queuepair(struct vmci_host_dev *vmci_host_dev,
int vmci_status;
int __user *retptr;
- if (vmci_host_dev->ct_type != VMCIOBJ_CONTEXT) {
+ if (smp_load_acquire(&vmci_host_dev->ct_type) != VMCIOBJ_CONTEXT) {
vmci_ioctl_err("only valid for contexts\n");
return -EINVAL;
}
@@ -522,7 +528,7 @@ static int vmci_host_do_queuepair_setva(struct vmci_host_dev *vmci_host_dev,
struct vmci_qp_set_va_info __user *info = uptr;
s32 result;
- if (vmci_host_dev->ct_type != VMCIOBJ_CONTEXT) {
+ if (smp_load_acquire(&vmci_host_dev->ct_type) != VMCIOBJ_CONTEXT) {
vmci_ioctl_err("only valid for contexts\n");
return -EINVAL;
}
@@ -570,7 +576,7 @@ static int vmci_host_do_queuepair_setpf(struct vmci_host_dev *vmci_host_dev,
return -EINVAL;
}
- if (vmci_host_dev->ct_type != VMCIOBJ_CONTEXT) {
+ if (smp_load_acquire(&vmci_host_dev->ct_type) != VMCIOBJ_CONTEXT) {
vmci_ioctl_err("only valid for contexts\n");
return -EINVAL;
}
@@ -641,7 +647,7 @@ static int vmci_host_do_qp_detach(struct vmci_host_dev *vmci_host_dev,
struct vmci_qp_dtch_info __user *info = uptr;
s32 result;
- if (vmci_host_dev->ct_type != VMCIOBJ_CONTEXT) {
+ if (smp_load_acquire(&vmci_host_dev->ct_type) != VMCIOBJ_CONTEXT) {
vmci_ioctl_err("only valid for contexts\n");
return -EINVAL;
}
@@ -668,7 +674,7 @@ static int vmci_host_do_ctx_add_notify(struct vmci_host_dev *vmci_host_dev,
s32 result;
u32 cid;
- if (vmci_host_dev->ct_type != VMCIOBJ_CONTEXT) {
+ if (smp_load_acquire(&vmci_host_dev->ct_type) != VMCIOBJ_CONTEXT) {
vmci_ioctl_err("only valid for contexts\n");
return -EINVAL;
}
@@ -691,7 +697,7 @@ static int vmci_host_do_ctx_remove_notify(struct vmci_host_dev *vmci_host_dev,
u32 cid;
int result;
- if (vmci_host_dev->ct_type != VMCIOBJ_CONTEXT) {
+ if (smp_load_acquire(&vmci_host_dev->ct_type) != VMCIOBJ_CONTEXT) {
vmci_ioctl_err("only valid for contexts\n");
return -EINVAL;
}
@@ -715,7 +721,7 @@ static int vmci_host_do_ctx_get_cpt_state(struct vmci_host_dev *vmci_host_dev,
void *cpt_buf;
int retval;
- if (vmci_host_dev->ct_type != VMCIOBJ_CONTEXT) {
+ if (smp_load_acquire(&vmci_host_dev->ct_type) != VMCIOBJ_CONTEXT) {
vmci_ioctl_err("only valid for contexts\n");
return -EINVAL;
}
@@ -747,7 +753,7 @@ static int vmci_host_do_ctx_set_cpt_state(struct vmci_host_dev *vmci_host_dev,
void *cpt_buf;
int retval;
- if (vmci_host_dev->ct_type != VMCIOBJ_CONTEXT) {
+ if (smp_load_acquire(&vmci_host_dev->ct_type) != VMCIOBJ_CONTEXT) {
vmci_ioctl_err("only valid for contexts\n");
return -EINVAL;
}
@@ -785,7 +791,7 @@ static int vmci_host_do_set_notify(struct vmci_host_dev *vmci_host_dev,
{
struct vmci_set_notify_info notify_info;
- if (vmci_host_dev->ct_type != VMCIOBJ_CONTEXT) {
+ if (smp_load_acquire(&vmci_host_dev->ct_type) != VMCIOBJ_CONTEXT) {
vmci_ioctl_err("only valid for contexts\n");
return -EINVAL;
}
@@ -818,7 +824,7 @@ static int vmci_host_do_notify_resource(struct vmci_host_dev *vmci_host_dev,
return -EINVAL;
}
- if (vmci_host_dev->ct_type != VMCIOBJ_CONTEXT) {
+ if (smp_load_acquire(&vmci_host_dev->ct_type) != VMCIOBJ_CONTEXT) {
vmci_ioctl_err("only valid for contexts\n");
return -EINVAL;
}
@@ -867,7 +873,7 @@ static int vmci_host_do_recv_notifications(struct vmci_host_dev *vmci_host_dev,
u32 cid;
int retval = 0;
- if (vmci_host_dev->ct_type != VMCIOBJ_CONTEXT) {
+ if (smp_load_acquire(&vmci_host_dev->ct_type) != VMCIOBJ_CONTEXT) {
vmci_ioctl_err("only valid for contexts\n");
return -EINVAL;
}
--
2.37.3
next reply other threads:[~2023-11-22 12:21 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-11-22 12:20 Yewon Choi [this message]
2023-11-22 14:34 ` [PATCH] vmci_host: use smp_load_acquire/smp_store_release when accessing vmci_host_dev->ct_type Greg Kroah-Hartman
2023-11-23 7:49 ` Yewon Choi
2023-11-23 8:44 ` Greg Kroah-Hartman
2023-11-23 10:06 ` Dae R. Jeong
2023-11-23 10:14 ` Greg Kroah-Hartman
2023-11-23 11:08 ` Dae R. Jeong
2023-11-23 12:21 ` Greg Kroah-Hartman
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20231122122005.GA4661@libra05 \
--to=woni9911@gmail.com \
--cc=arnd@arndb.de \
--cc=bryantan@vmware.com \
--cc=gregkh@linuxfoundation.org \
--cc=linux-kernel@vger.kernel.org \
--cc=pv-drivers@vmware.com \
--cc=threeearcat@gmail.com \
--cc=vdasa@vmware.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox