Messages in this thread Patch in this message | | | Date | Thu, 30 Mar 2023 20:18:32 +0900 | From | "Dae R. Jeong" <> | Subject | Re: [PATCH] vmci_host: fix a race condition in vmci_host_poll() causing GPF |
| |
On Wed, Mar 29, 2023 at 12:15:21PM +0200, Greg KH wrote: > On Mon, Mar 27, 2023 at 09:01:53PM +0900, Dae R. Jeong wrote: > > During fuzzing, a general protection fault is observed in > > vmci_host_poll(). > > > > general protection fault, probably for non-canonical address 0xdffffc0000000019: 0000 [#1] PREEMPT SMP KASAN > > KASAN: null-ptr-deref in range [0x00000000000000c8-0x00000000000000cf] > > RIP: 0010:__lock_acquire+0xf3/0x5e00 kernel/locking/lockdep.c:4926 > > <- omitting registers -> > > Call Trace: > > <TASK> > > lock_acquire+0x1a4/0x4a0 kernel/locking/lockdep.c:5672 > > __raw_spin_lock_irqsave include/linux/spinlock_api_smp.h:110 [inline] > > _raw_spin_lock_irqsave+0xb3/0x100 kernel/locking/spinlock.c:162 > > add_wait_queue+0x3d/0x260 kernel/sched/wait.c:22 > > poll_wait include/linux/poll.h:49 [inline] > > vmci_host_poll+0xf8/0x2b0 drivers/misc/vmw_vmci/vmci_host.c:174 > > vfs_poll include/linux/poll.h:88 [inline] > > do_pollfd fs/select.c:873 [inline] > > do_poll fs/select.c:921 [inline] > > do_sys_poll+0xc7c/0x1aa0 fs/select.c:1015 > > __do_sys_ppoll fs/select.c:1121 [inline] > > __se_sys_ppoll+0x2cc/0x330 fs/select.c:1101 > > do_syscall_x64 arch/x86/entry/common.c:51 [inline] > > do_syscall_64+0x4e/0xa0 arch/x86/entry/common.c:82 > > entry_SYSCALL_64_after_hwframe+0x46/0xb0 > > > > Example thread interleaving that causes the general protection fault > > is as follows: > > > > CPU1 (vmci_host_poll) CPU2 (vmci_host_do_init_context) > > ----- ----- > > // Read uninitialized context > > context = vmci_host_dev->context; > > // Initialize context > > vmci_host_dev->context = vmci_ctx_create(); > > vmci_host_dev->ct_type = VMCIOBJ_CONTEXT; > > > > if (vmci_host_dev->ct_type == VMCIOBJ_CONTEXT) { > > // Dereferencing the wrong pointer > > poll_wait(..., &context->host_context); > > } > > > > In this scenario, vmci_host_poll() reads vmci_host_dev->context first, > > and then reads vmci_host_dev->ct_type to check that > > vmci_host_dev->context is initialized. However, since these two reads > > are not atomically executed, there is a chance of a race condition as > > described above. > > > > To fix this race condition, read vmci_host_dev->context after checking > > the value of vmci_host_dev->ct_type so that vmci_host_poll() always > > reads an initialized context. > > > > Reported-by: Dae R. Jeong <threeearcat@gmail.com> > > Fixes: 8bf503991f87 ("VMCI: host side driver implementation.") > > Signed-off-by: Dae R. Jeong <threeearcat@gmail.com> > > If you author and sign-off on the patch, no need for a Reported-by: as > that is obvious :)
For a student like me, two of my names are cooler than one... (shy smile :)...)
> And how did you test this change?
I attach a simple C program used in testing at the end of this email. I used the same C program to trigger the crash before applying the patch, and to test the patch after applying it.
Before applying this patch, I inserted msleep() to trigger the crash easily as follows:
diff --git a/drivers/misc/vmw_vmci/vmci_host.c b/drivers/misc/vmw_vmci/vmci_host.c index 857b9851402a..e925f7effac4 100644 --- a/drivers/misc/vmw_vmci/vmci_host.c +++ b/drivers/misc/vmw_vmci/vmci_host.c @@ -24,6 +24,7 @@ #include <linux/smp.h> #include <linux/fs.h> #include <linux/io.h> +#include <linux/delay.h> #include "vmci_handle_array.h" #include "vmci_queue_pair.h" @@ -168,6 +169,11 @@ static __poll_t vmci_host_poll(struct file *filp, poll_table *wait) struct vmci_ctx *context = vmci_host_dev->context; __poll_t mask = 0; + msleep(1000); + + printk("%s, context=%px, ct_type=%d\n", __func__, context, + vmci_host_dev->ct_type); + if (vmci_host_dev->ct_type == VMCIOBJ_CONTEXT) { /* Check for VMCI calls to this VM context. */ if (wait) @@ -341,6 +347,10 @@ static int vmci_host_do_init_context(struct vmci_host_dev *vmci_host_dev, } vmci_host_dev->ct_type = VMCIOBJ_CONTEXT; + + printk("%s, context=%px, ct_type=%d\n", __func__, + vmci_host_dev->context, vmci_host_dev->ct_type); + atomic_inc(&vmci_host_active_users); vmci_call_vsock_callback(true); With this msleep(), the C program below can easily trigger the GPF.
Then I applied the patch that I propose, and ran the same C program. After applying the patch, I couldn't observe the GPF.
> thanks, > > greg k-h
Best regards, Dae R. Jeong
------------->8----------------- #include <fcntl.h> #include <poll.h> #include <pthread.h> #include <signal.h> #include <stdio.h> #include <sys/ioctl.h> #include <sys/stat.h> #include <sys/types.h> #include <unistd.h> struct init_block { int cid; int flags; }; int fd;
void *th1(void *a) { struct init_block s = { .cid = 2, .flags = 0, }; ioctl(fd, 0x7a0, &s); return NULL; } void *th2(void *a) { struct pollfd pfd = {.fd = fd}; poll(&pfd, 1, 20000); return NULL; } int test() { #define filename "/dev/vmci" fd = openat(AT_FDCWD, filename, 0x2, 0x0); int var = 0x10000; ioctl(fd, 0x7a7, &var); pthread_t pth1, pth2; pthread_create(&pth1, NULL, th1, NULL); pthread_create(&pth2, NULL, th2, NULL); pthread_join(pth1, NULL); pthread_join(pth2, NULL); return 0; } int main() { for (;;) { pid_t pid = fork(); if (pid == 0) { test(); } else { usleep(100 * 1000); kill(pid, SIGKILL); } } }
| |