diff mbox series

vhost-vdpa: fix use-after-free in _compat_vdpa_reset

Message ID 1698275594-19204-1-git-send-email-si-wei.liu@oracle.com
State New
Headers
Series vhost-vdpa: fix use-after-free in _compat_vdpa_reset |

Commit Message

Si-Wei Liu Oct. 25, 2023, 11:13 p.m. UTC 
  When the vhost-vdpa device is being closed, vhost_vdpa_cleanup() doesn't
clean up the vqs pointer after free. This could lead to use-after-tree
when _compat_vdpa_reset() tries to access the vqs that are freed already.
Fix is to set vqs pointer to NULL at the end of vhost_vdpa_cleanup()
after getting freed, which is guarded by atomic opened state.

  BUG: unable to handle page fault for address: 00000001005b4af4
  #PF: supervisor read access in kernel mode
  #PF: error_code(0x0000) - not-present page
  PGD 16a80a067 P4D 0
  Oops: 0000 [#1] PREEMPT SMP NOPTI
  CPU: 4 PID: 40387 Comm: qemu-kvm Not tainted 6.6.0-rc7+ #3
  Hardware name: Dell Inc. PowerEdge R750/0PJ80M, BIOS 1.8.2 09/14/2022
  RIP: 0010:_compat_vdpa_reset.isra.0+0x27/0xb0 [vhost_vdpa]
  Code: 90 90 90 0f 1f 44 00 00 41 55 4c 8d ae 08 03 00 00 41 54 55 48
  89 f5 53 4c 8b a6 00 03 00 00 48 85 ff 74 49 48 8b 07 4c 89 ef <48> 8b
  80 88 45 00 00 48 c1 e8 08 48 83 f0 01 89 c3 e8 73 5e 9b dc
  RSP: 0018:ff73a85762073ba0 EFLAGS: 00010286
  RAX: 00000001005b056c RBX: ff32b13ca6994c68 RCX: 0000000000000002
  RDX: 0000000000000001 RSI: ff32b13c07559000 RDI: ff32b13c07559308
  RBP: ff32b13c07559000 R08: 0000000000000000 R09: ff32b12ca497c0f0
  R10: ff73a85762073c58 R11: 0000000c106f9de3 R12: ff32b12c95b1d050
  R13: ff32b13c07559308 R14: ff32b12d0ddc5100 R15: 0000000000008002
  FS:  00007fec5b8cbf80(0000) GS:ff32b13bbfc80000(0000)
  knlGS:0000000000000000
  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
  CR2: 00000001005b4af4 CR3: 000000015644a003 CR4: 0000000000773ee0
  PKRU: 55555554
  Call Trace:
   <TASK>
   ? __die+0x20/0x70
   ? page_fault_oops+0x76/0x170
   ? exc_page_fault+0x65/0x150
   ? asm_exc_page_fault+0x22/0x30
   ? _compat_vdpa_reset.isra.0+0x27/0xb0 [vhost_vdpa]
   vhost_vdpa_open+0x57/0x280 [vhost_vdpa]
   ? __pfx_chrdev_open+0x10/0x10
   chrdev_open+0xc6/0x260
   ? __pfx_chrdev_open+0x10/0x10
   do_dentry_open+0x16e/0x530
   do_open+0x21c/0x400
   path_openat+0x111/0x290
   do_filp_open+0xb2/0x160
   ? __check_object_size.part.0+0x5e/0x140
   do_sys_openat2+0x96/0xd0
   __x64_sys_openat+0x53/0xa0
   do_syscall_64+0x59/0x90
   ? syscall_exit_to_user_mode+0x22/0x40
   ? do_syscall_64+0x69/0x90
   ? syscall_exit_to_user_mode+0x22/0x40
   ? do_syscall_64+0x69/0x90
   ? do_syscall_64+0x69/0x90
   ? syscall_exit_to_user_mode+0x22/0x40
   ? do_syscall_64+0x69/0x90
   ? exc_page_fault+0x65/0x150
   entry_SYSCALL_64_after_hwframe+0x6e/0xd8

Fixes: 10cbf8dfaf93 ("vhost-vdpa: clean iotlb map during reset for older userspace")
Fixes: ac7e98c73c05 ("vhost-vdpa: fix NULL pointer deref in _compat_vdpa_reset")
Reported-by: Lei Yang <leiyang@redhat.com>
Closes: https://lore.kernel.org/all/CAPpAL=yHDqn1AztEcN3MpS8o4M+BL_HVy02FdpiHN7DWd91HwQ@mail.gmail.com/
Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
---
 drivers/vhost/vdpa.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Michael S. Tsirkin Oct. 26, 2023, 5:26 a.m. UTC | #1 
On Wed, Oct 25, 2023 at 04:13:14PM -0700, Si-Wei Liu wrote:
> When the vhost-vdpa device is being closed, vhost_vdpa_cleanup() doesn't
> clean up the vqs pointer after free. This could lead to use-after-tree
> when _compat_vdpa_reset() tries to access the vqs that are freed already.
> Fix is to set vqs pointer to NULL at the end of vhost_vdpa_cleanup()
> after getting freed, which is guarded by atomic opened state.
> 
>   BUG: unable to handle page fault for address: 00000001005b4af4
>   #PF: supervisor read access in kernel mode
>   #PF: error_code(0x0000) - not-present page
>   PGD 16a80a067 P4D 0
>   Oops: 0000 [#1] PREEMPT SMP NOPTI
>   CPU: 4 PID: 40387 Comm: qemu-kvm Not tainted 6.6.0-rc7+ #3
>   Hardware name: Dell Inc. PowerEdge R750/0PJ80M, BIOS 1.8.2 09/14/2022
>   RIP: 0010:_compat_vdpa_reset.isra.0+0x27/0xb0 [vhost_vdpa]
>   Code: 90 90 90 0f 1f 44 00 00 41 55 4c 8d ae 08 03 00 00 41 54 55 48
>   89 f5 53 4c 8b a6 00 03 00 00 48 85 ff 74 49 48 8b 07 4c 89 ef <48> 8b
>   80 88 45 00 00 48 c1 e8 08 48 83 f0 01 89 c3 e8 73 5e 9b dc
>   RSP: 0018:ff73a85762073ba0 EFLAGS: 00010286
>   RAX: 00000001005b056c RBX: ff32b13ca6994c68 RCX: 0000000000000002
>   RDX: 0000000000000001 RSI: ff32b13c07559000 RDI: ff32b13c07559308
>   RBP: ff32b13c07559000 R08: 0000000000000000 R09: ff32b12ca497c0f0
>   R10: ff73a85762073c58 R11: 0000000c106f9de3 R12: ff32b12c95b1d050
>   R13: ff32b13c07559308 R14: ff32b12d0ddc5100 R15: 0000000000008002
>   FS:  00007fec5b8cbf80(0000) GS:ff32b13bbfc80000(0000)
>   knlGS:0000000000000000
>   CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>   CR2: 00000001005b4af4 CR3: 000000015644a003 CR4: 0000000000773ee0
>   PKRU: 55555554
>   Call Trace:
>    <TASK>
>    ? __die+0x20/0x70
>    ? page_fault_oops+0x76/0x170
>    ? exc_page_fault+0x65/0x150
>    ? asm_exc_page_fault+0x22/0x30
>    ? _compat_vdpa_reset.isra.0+0x27/0xb0 [vhost_vdpa]
>    vhost_vdpa_open+0x57/0x280 [vhost_vdpa]
>    ? __pfx_chrdev_open+0x10/0x10
>    chrdev_open+0xc6/0x260
>    ? __pfx_chrdev_open+0x10/0x10
>    do_dentry_open+0x16e/0x530
>    do_open+0x21c/0x400
>    path_openat+0x111/0x290
>    do_filp_open+0xb2/0x160
>    ? __check_object_size.part.0+0x5e/0x140
>    do_sys_openat2+0x96/0xd0
>    __x64_sys_openat+0x53/0xa0
>    do_syscall_64+0x59/0x90
>    ? syscall_exit_to_user_mode+0x22/0x40
>    ? do_syscall_64+0x69/0x90
>    ? syscall_exit_to_user_mode+0x22/0x40
>    ? do_syscall_64+0x69/0x90
>    ? do_syscall_64+0x69/0x90
>    ? syscall_exit_to_user_mode+0x22/0x40
>    ? do_syscall_64+0x69/0x90
>    ? exc_page_fault+0x65/0x150
>    entry_SYSCALL_64_after_hwframe+0x6e/0xd8
> 
> Fixes: 10cbf8dfaf93 ("vhost-vdpa: clean iotlb map during reset for older userspace")
> Fixes: ac7e98c73c05 ("vhost-vdpa: fix NULL pointer deref in _compat_vdpa_reset")

So these two are all in next correct?

I really do not like it how 10cbf8dfaf936e3ef1f5d7fdc6e9dada268ba6bb
introduced a regression and then apparently we keep fixing things up?

Can I squash these 3 commits?


> Reported-by: Lei Yang <leiyang@redhat.com>
> Closes: https://lore.kernel.org/all/CAPpAL=yHDqn1AztEcN3MpS8o4M+BL_HVy02FdpiHN7DWd91HwQ@mail.gmail.com/
> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
> ---
>  drivers/vhost/vdpa.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
> index 9a2343c45df0..30df5c58db73 100644
> --- a/drivers/vhost/vdpa.c
> +++ b/drivers/vhost/vdpa.c
> @@ -1355,6 +1355,7 @@ static void vhost_vdpa_cleanup(struct vhost_vdpa *v)
>  	vhost_vdpa_free_domain(v);
>  	vhost_dev_cleanup(&v->vdev);
>  	kfree(v->vdev.vqs);
> +	v->vdev.vqs = NULL;
>  }
>  
>  static int vhost_vdpa_open(struct inode *inode, struct file *filep)
> -- 
> 2.39.3
Si-Wei Liu Oct. 26, 2023, 6:55 a.m. UTC | #2 
On 10/25/2023 10:26 PM, Michael S. Tsirkin wrote:
> On Wed, Oct 25, 2023 at 04:13:14PM -0700, Si-Wei Liu wrote:
>> When the vhost-vdpa device is being closed, vhost_vdpa_cleanup() doesn't
>> clean up the vqs pointer after free. This could lead to use-after-tree
>> when _compat_vdpa_reset() tries to access the vqs that are freed already.
>> Fix is to set vqs pointer to NULL at the end of vhost_vdpa_cleanup()
>> after getting freed, which is guarded by atomic opened state.
>>
>>    BUG: unable to handle page fault for address: 00000001005b4af4
>>    #PF: supervisor read access in kernel mode
>>    #PF: error_code(0x0000) - not-present page
>>    PGD 16a80a067 P4D 0
>>    Oops: 0000 [#1] PREEMPT SMP NOPTI
>>    CPU: 4 PID: 40387 Comm: qemu-kvm Not tainted 6.6.0-rc7+ #3
>>    Hardware name: Dell Inc. PowerEdge R750/0PJ80M, BIOS 1.8.2 09/14/2022
>>    RIP: 0010:_compat_vdpa_reset.isra.0+0x27/0xb0 [vhost_vdpa]
>>    Code: 90 90 90 0f 1f 44 00 00 41 55 4c 8d ae 08 03 00 00 41 54 55 48
>>    89 f5 53 4c 8b a6 00 03 00 00 48 85 ff 74 49 48 8b 07 4c 89 ef <48> 8b
>>    80 88 45 00 00 48 c1 e8 08 48 83 f0 01 89 c3 e8 73 5e 9b dc
>>    RSP: 0018:ff73a85762073ba0 EFLAGS: 00010286
>>    RAX: 00000001005b056c RBX: ff32b13ca6994c68 RCX: 0000000000000002
>>    RDX: 0000000000000001 RSI: ff32b13c07559000 RDI: ff32b13c07559308
>>    RBP: ff32b13c07559000 R08: 0000000000000000 R09: ff32b12ca497c0f0
>>    R10: ff73a85762073c58 R11: 0000000c106f9de3 R12: ff32b12c95b1d050
>>    R13: ff32b13c07559308 R14: ff32b12d0ddc5100 R15: 0000000000008002
>>    FS:  00007fec5b8cbf80(0000) GS:ff32b13bbfc80000(0000)
>>    knlGS:0000000000000000
>>    CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>    CR2: 00000001005b4af4 CR3: 000000015644a003 CR4: 0000000000773ee0
>>    PKRU: 55555554
>>    Call Trace:
>>     <TASK>
>>     ? __die+0x20/0x70
>>     ? page_fault_oops+0x76/0x170
>>     ? exc_page_fault+0x65/0x150
>>     ? asm_exc_page_fault+0x22/0x30
>>     ? _compat_vdpa_reset.isra.0+0x27/0xb0 [vhost_vdpa]
>>     vhost_vdpa_open+0x57/0x280 [vhost_vdpa]
>>     ? __pfx_chrdev_open+0x10/0x10
>>     chrdev_open+0xc6/0x260
>>     ? __pfx_chrdev_open+0x10/0x10
>>     do_dentry_open+0x16e/0x530
>>     do_open+0x21c/0x400
>>     path_openat+0x111/0x290
>>     do_filp_open+0xb2/0x160
>>     ? __check_object_size.part.0+0x5e/0x140
>>     do_sys_openat2+0x96/0xd0
>>     __x64_sys_openat+0x53/0xa0
>>     do_syscall_64+0x59/0x90
>>     ? syscall_exit_to_user_mode+0x22/0x40
>>     ? do_syscall_64+0x69/0x90
>>     ? syscall_exit_to_user_mode+0x22/0x40
>>     ? do_syscall_64+0x69/0x90
>>     ? do_syscall_64+0x69/0x90
>>     ? syscall_exit_to_user_mode+0x22/0x40
>>     ? do_syscall_64+0x69/0x90
>>     ? exc_page_fault+0x65/0x150
>>     entry_SYSCALL_64_after_hwframe+0x6e/0xd8
>>
>> Fixes: 10cbf8dfaf93 ("vhost-vdpa: clean iotlb map during reset for older userspace")
>> Fixes: ac7e98c73c05 ("vhost-vdpa: fix NULL pointer deref in _compat_vdpa_reset")
> So these two are all in next correct?
>
> I really do not like it how 10cbf8dfaf936e3ef1f5d7fdc6e9dada268ba6bb
> introduced a regression and then apparently we keep fixing things up?
Sorry my bad. The latest one should be all of it.

>
> Can I squash these 3 commits?
Sure. Or if you want me to send a v5 with all 3 commits squashed in, I 
can do for sure.

Thanks,
-Siwei
>
>
>> Reported-by: Lei Yang <leiyang@redhat.com>
>> Closes: https://lore.kernel.org/all/CAPpAL=yHDqn1AztEcN3MpS8o4M+BL_HVy02FdpiHN7DWd91HwQ@mail.gmail.com/
>> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
>> ---
>>   drivers/vhost/vdpa.c | 1 +
>>   1 file changed, 1 insertion(+)
>>
>> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
>> index 9a2343c45df0..30df5c58db73 100644
>> --- a/drivers/vhost/vdpa.c
>> +++ b/drivers/vhost/vdpa.c
>> @@ -1355,6 +1355,7 @@ static void vhost_vdpa_cleanup(struct vhost_vdpa *v)
>>   	vhost_vdpa_free_domain(v);
>>   	vhost_dev_cleanup(&v->vdev);
>>   	kfree(v->vdev.vqs);
>> +	v->vdev.vqs = NULL;
>>   }
>>   
>>   static int vhost_vdpa_open(struct inode *inode, struct file *filep)
>> -- 
>> 2.39.3
Si-Wei Liu Oct. 26, 2023, 7:19 a.m. UTC | #3 
On 10/25/2023 11:55 PM, Si-Wei Liu wrote:
>
>
> On 10/25/2023 10:26 PM, Michael S. Tsirkin wrote:
>> On Wed, Oct 25, 2023 at 04:13:14PM -0700, Si-Wei Liu wrote:
>>> When the vhost-vdpa device is being closed, vhost_vdpa_cleanup() 
>>> doesn't
>>> clean up the vqs pointer after free. This could lead to use-after-tree
>>> when _compat_vdpa_reset() tries to access the vqs that are freed 
>>> already.
>>> Fix is to set vqs pointer to NULL at the end of vhost_vdpa_cleanup()
>>> after getting freed, which is guarded by atomic opened state.
>>>
>>>    BUG: unable to handle page fault for address: 00000001005b4af4
>>>    #PF: supervisor read access in kernel mode
>>>    #PF: error_code(0x0000) - not-present page
>>>    PGD 16a80a067 P4D 0
>>>    Oops: 0000 [#1] PREEMPT SMP NOPTI
>>>    CPU: 4 PID: 40387 Comm: qemu-kvm Not tainted 6.6.0-rc7+ #3
>>>    Hardware name: Dell Inc. PowerEdge R750/0PJ80M, BIOS 1.8.2 
>>> 09/14/2022
>>>    RIP: 0010:_compat_vdpa_reset.isra.0+0x27/0xb0 [vhost_vdpa]
>>>    Code: 90 90 90 0f 1f 44 00 00 41 55 4c 8d ae 08 03 00 00 41 54 55 48
>>>    89 f5 53 4c 8b a6 00 03 00 00 48 85 ff 74 49 48 8b 07 4c 89 ef 
>>> <48> 8b
>>>    80 88 45 00 00 48 c1 e8 08 48 83 f0 01 89 c3 e8 73 5e 9b dc
>>>    RSP: 0018:ff73a85762073ba0 EFLAGS: 00010286
>>>    RAX: 00000001005b056c RBX: ff32b13ca6994c68 RCX: 0000000000000002
>>>    RDX: 0000000000000001 RSI: ff32b13c07559000 RDI: ff32b13c07559308
>>>    RBP: ff32b13c07559000 R08: 0000000000000000 R09: ff32b12ca497c0f0
>>>    R10: ff73a85762073c58 R11: 0000000c106f9de3 R12: ff32b12c95b1d050
>>>    R13: ff32b13c07559308 R14: ff32b12d0ddc5100 R15: 0000000000008002
>>>    FS:  00007fec5b8cbf80(0000) GS:ff32b13bbfc80000(0000)
>>>    knlGS:0000000000000000
>>>    CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
>>>    CR2: 00000001005b4af4 CR3: 000000015644a003 CR4: 0000000000773ee0
>>>    PKRU: 55555554
>>>    Call Trace:
>>>     <TASK>
>>>     ? __die+0x20/0x70
>>>     ? page_fault_oops+0x76/0x170
>>>     ? exc_page_fault+0x65/0x150
>>>     ? asm_exc_page_fault+0x22/0x30
>>>     ? _compat_vdpa_reset.isra.0+0x27/0xb0 [vhost_vdpa]
>>>     vhost_vdpa_open+0x57/0x280 [vhost_vdpa]
>>>     ? __pfx_chrdev_open+0x10/0x10
>>>     chrdev_open+0xc6/0x260
>>>     ? __pfx_chrdev_open+0x10/0x10
>>>     do_dentry_open+0x16e/0x530
>>>     do_open+0x21c/0x400
>>>     path_openat+0x111/0x290
>>>     do_filp_open+0xb2/0x160
>>>     ? __check_object_size.part.0+0x5e/0x140
>>>     do_sys_openat2+0x96/0xd0
>>>     __x64_sys_openat+0x53/0xa0
>>>     do_syscall_64+0x59/0x90
>>>     ? syscall_exit_to_user_mode+0x22/0x40
>>>     ? do_syscall_64+0x69/0x90
>>>     ? syscall_exit_to_user_mode+0x22/0x40
>>>     ? do_syscall_64+0x69/0x90
>>>     ? do_syscall_64+0x69/0x90
>>>     ? syscall_exit_to_user_mode+0x22/0x40
>>>     ? do_syscall_64+0x69/0x90
>>>     ? exc_page_fault+0x65/0x150
>>>     entry_SYSCALL_64_after_hwframe+0x6e/0xd8
>>>
>>> Fixes: 10cbf8dfaf93 ("vhost-vdpa: clean iotlb map during reset for 
>>> older userspace")
>>> Fixes: ac7e98c73c05 ("vhost-vdpa: fix NULL pointer deref in 
>>> _compat_vdpa_reset")
>> So these two are all in next correct?
>>
>> I really do not like it how 10cbf8dfaf936e3ef1f5d7fdc6e9dada268ba6bb
>> introduced a regression and then apparently we keep fixing things up?
> Sorry my bad. The latest one should be all of it.
>
>>
>> Can I squash these 3 commits?
> Sure. Or if you want me to send a v5 with all 3 commits squashed in, I 
> can do for sure.
Saw you squashed it with the 2 fixups in place, thank you! Sent a v5 
anyway, just in case if you need a fresh series.

Thanks,
-Siwei

>
> Thanks,
> -Siwei
>>
>>
>>> Reported-by: Lei Yang <leiyang@redhat.com>
>>> Closes: 
>>> https://lore.kernel.org/all/CAPpAL=yHDqn1AztEcN3MpS8o4M+BL_HVy02FdpiHN7DWd91HwQ@mail.gmail.com/
>>> Signed-off-by: Si-Wei Liu <si-wei.liu@oracle.com>
>>> ---
>>>   drivers/vhost/vdpa.c | 1 +
>>>   1 file changed, 1 insertion(+)
>>>
>>> diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
>>> index 9a2343c45df0..30df5c58db73 100644
>>> --- a/drivers/vhost/vdpa.c
>>> +++ b/drivers/vhost/vdpa.c
>>> @@ -1355,6 +1355,7 @@ static void vhost_vdpa_cleanup(struct 
>>> vhost_vdpa *v)
>>>       vhost_vdpa_free_domain(v);
>>>       vhost_dev_cleanup(&v->vdev);
>>>       kfree(v->vdev.vqs);
>>> +    v->vdev.vqs = NULL;
>>>   }
>>>     static int vhost_vdpa_open(struct inode *inode, struct file *filep)
>>> -- 
>>> 2.39.3
>
diff mbox series

Patch

diff --git a/drivers/vhost/vdpa.c b/drivers/vhost/vdpa.c
index 9a2343c45df0..30df5c58db73 100644
--- a/drivers/vhost/vdpa.c
+++ b/drivers/vhost/vdpa.c
@@ -1355,6 +1355,7 @@  static void vhost_vdpa_cleanup(struct vhost_vdpa *v)
 	vhost_vdpa_free_domain(v);
 	vhost_dev_cleanup(&v->vdev);
 	kfree(v->vdev.vqs);
+	v->vdev.vqs = NULL;
 }
 
 static int vhost_vdpa_open(struct inode *inode, struct file *filep)