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 <[email protected]>
Fixes: 8bf503991f87 ("VMCI: host side driver implementation.")
Signed-off-by: Dae R. Jeong <[email protected]>
---
drivers/misc/vmw_vmci/vmci_host.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/misc/vmw_vmci/vmci_host.c b/drivers/misc/vmw_vmci/vmci_host.c
index 857b9851402a..abe79f6fd2a7 100644
--- a/drivers/misc/vmw_vmci/vmci_host.c
+++ b/drivers/misc/vmw_vmci/vmci_host.c
@@ -165,10 +165,16 @@ static int vmci_host_close(struct inode *inode, struct file *filp)
static __poll_t vmci_host_poll(struct file *filp, poll_table *wait)
{
struct vmci_host_dev *vmci_host_dev = filp->private_data;
- struct vmci_ctx *context = vmci_host_dev->context;
+ struct vmci_ctx *context;
__poll_t mask = 0;
if (vmci_host_dev->ct_type == VMCIOBJ_CONTEXT) {
+ /*
+ * Read context only if ct_type == VMCIOBJ_CONTEXT to make
+ * sure that context is initialized
+ */
+ context = vmci_host_dev->context;
+
/* Check for VMCI calls to this VM context. */
if (wait)
poll_wait(filp, &context->host_context.wait_queue,
--
2.17.1
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 <[email protected]>
> Fixes: 8bf503991f87 ("VMCI: host side driver implementation.")
> Signed-off-by: Dae R. Jeong <[email protected]>
If you author and sign-off on the patch, no need for a Reported-by: as
that is obvious :)
And how did you test this change?
thanks,
greg k-h
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 <[email protected]>
> > Fixes: 8bf503991f87 ("VMCI: host side driver implementation.")
> > Signed-off-by: Dae R. Jeong <[email protected]>
>
> 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);
}
}
}
> On Mar 30, 2023, at 4:18 AM, Dae R. Jeong <[email protected]> wrote:
>
> !! External Email
>
> 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 <[email protected]>
>>> Fixes: 8bf503991f87 ("VMCI: host side driver implementation.")
>>> Signed-off-by: Dae R. Jeong <[email protected]>
>>
>> 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:
Hi Dae,
Thanks for your test code and for your patience.
Your test conditions are a little far from regular operating conditions. My
comments/questions are as follows:
- the test uses separate threads to do the ioctl and poll operations in parallel. Normally,
the vmx process schedules periodic poll after the host context init ioctl completes
successfully. Also, the vmx process does not keep calling the host context init ioctl
once this is done. So, the race condition shouldn't arise in the first place due to the
way vmx works. I understand that you are fuzzing, but just letting you know the
normal operating conditions.
- vmx process gets the vmci device handle with elevated privileges. I believe you ran
the test as root as well? Since vmx can't cause the race condition, a malicious app
like your fuzzer would need root privileges to do the damage.
- how easy is it to hit the gpf without the msleep?
Overall, I don't think this is a "must fix" issue today, but I'm also not sure if we should
reject it either due to the simple nature of the patch. For arm, we may need to add
a write barrier before 'vmci_host_dev->ct_type = VMCIOBJ_CONTEXT' in
vmci_host_do_init_context and a read barrier before
'context = vmci_host_dev->context' in your patch.
Thanks,
Vishnu
>
> 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);
> }
> }
> }
>
> !! External Email: This email originated from outside of the organization. Do not click links or open attachments unless you recognize the sender.