[PATCH] Revert "KVM: mmio: Fix use-after-free Read in kvm_vm_ioctl_unregister_coalesced_mmio"

[PATCH] Revert "KVM: mmio: Fix use-after-free Read in kvm_vm_ioctl_unregister_coalesced_mmio"

[Michal Luczaj]

The commit states:

    If kvm_io_bus_unregister_dev() return -ENOMEM, we already call
    kvm_iodevice_destructor() inside this function to delete
    'struct kvm_coalesced_mmio_dev *dev' from list and free the dev,
    but kvm_iodevice_destructor() is called again, it will lead the
    above issue.

    Let's check the the return value of kvm_io_bus_unregister_dev(),
    only call kvm_iodevice_destructor() if the return value is 0.

This is not entirely correct. kvm_io_bus_unregister_dev() never invokes
kvm_iodevice_destructor() on the device that was passed for
unregistering. Thus, calling kvm_iodevice_destructor() iff
kvm_io_bus_unregister_dev() returns 0 does not fix a use-after-free,
but introduces a memory leak.

It seems that the actual fix for the use-after-free(s) was
commit 5d3c4c79384a ("KVM: Stop looking for coalesced MMIO zones if the
bus is destroyed"), which made into 5.10.37 (mainline 5.13-rc1). Now,
syzkaller's report from the reverted commit indicates an earlier kernel
version 5.10.0, while the memory leak was introduced in 5.10.52
(5.14-rc2).

Currently, running

    ioctl(vm, KVM_REGISTER_COALESCED_MMIO, &zone);
    // fail the upcoming kmalloc() in kvm_io_bus_unregister_dev()
    ioctl(vm, KVM_UNREGISTER_COALESCED_MMIO, &zone);

results in

[  200.212348] kvm: failed to shrink bus, removing it completely
unreferenced object 0xffff88810e7fa300 (size 64):
  comm "a.out", pid 972, jiffies 4294867275 (age 20.499s)
  hex dump (first 32 bytes):
    58 e3 fd 00 00 c9 ff ff 58 e3 fd 00 00 c9 ff ff  X.......X.......
    c0 66 65 c0 ff ff ff ff 00 40 fd 00 00 c9 ff ff  .fe......@......
  backtrace:
    [<00000000a9f977ff>] kmalloc_trace+0x26/0x60
    [<0000000072e1256d>] kvm_vm_ioctl_register_coalesced_mmio+0x8b/0x420 [kvm]
    [<00000000cc4b12dc>] kvm_vm_ioctl+0x1415/0x2050 [kvm]
    [<000000004e08022f>] __x64_sys_ioctl+0x126/0x190
    [<0000000044a4fad3>] do_syscall_64+0x55/0x80
    [<00000000d7073b12>] entry_SYSCALL_64_after_hwframe+0x46/0xb0

This reverts commit 23fa2e46a5556f787ce2ea1a315d3ab93cced204.

Signed-off-by: Michal Luczaj <mhal@rbox.co>
---
 virt/kvm/coalesced_mmio.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/virt/kvm/coalesced_mmio.c b/virt/kvm/coalesced_mmio.c
index 0be80c213f7f..f08f5e82460b 100644
--- a/virt/kvm/coalesced_mmio.c
+++ b/virt/kvm/coalesced_mmio.c
@@ -186,6 +186,7 @@ int kvm_vm_ioctl_unregister_coalesced_mmio(struct kvm *kvm,
 		    coalesced_mmio_in_range(dev, zone->addr, zone->size)) {
 			r = kvm_io_bus_unregister_dev(kvm,
 				zone->pio ? KVM_PIO_BUS : KVM_MMIO_BUS, &dev->dev);
+			kvm_iodevice_destructor(&dev->dev);
 
 			/*
 			 * On failure, unregister destroys all devices on the
@@ -195,7 +196,6 @@ int kvm_vm_ioctl_unregister_coalesced_mmio(struct kvm *kvm,
 			 */
 			if (r)
 				break;
-			kvm_iodevice_destructor(&dev->dev);
 		}
 	}
 
-- 
2.39.0

Re: [PATCH] Revert "KVM: mmio: Fix use-after-free Read in kvm_vm_ioctl_unregister_coalesced_mmio"

[Sean Christopherson]

On Wed, Jan 18, 2023, Michal Luczaj wrote:
> Signed-off-by: Michal Luczaj <mhal@rbox.co>
> ---
>  virt/kvm/coalesced_mmio.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/virt/kvm/coalesced_mmio.c b/virt/kvm/coalesced_mmio.c
> index 0be80c213f7f..f08f5e82460b 100644
> --- a/virt/kvm/coalesced_mmio.c
> +++ b/virt/kvm/coalesced_mmio.c
> @@ -186,6 +186,7 @@ int kvm_vm_ioctl_unregister_coalesced_mmio(struct kvm *kvm,
>  		    coalesced_mmio_in_range(dev, zone->addr, zone->size)) {
>  			r = kvm_io_bus_unregister_dev(kvm,
>  				zone->pio ? KVM_PIO_BUS : KVM_MMIO_BUS, &dev->dev);
> +			kvm_iodevice_destructor(&dev->dev);
>  
>  			/*
>  			 * On failure, unregister destroys all devices on the
> @@ -195,7 +196,6 @@ int kvm_vm_ioctl_unregister_coalesced_mmio(struct kvm *kvm,
>  			 */
>  			if (r)
>  				break;
> -			kvm_iodevice_destructor(&dev->dev);

Already posted[1], but didn't get queued because there's alternative solution[2]
that yields a far cleaner end result, albeit with a larger patch.  I'll follow
up on Wei's patch to move things along.

[1] https://lore.kernel.org/all/20221219171924.67989-1-seanjc@google.com
[2] https://lore.kernel.org/all/20221229123302.4083-1-wei.w.wang@intel.com

Re: [PATCH] Revert "KVM: mmio: Fix use-after-free Read in kvm_vm_ioctl_unregister_coalesced_mmio"

[Michal Luczaj]

On 1/24/23 00:06, Sean Christopherson wrote:
> On Wed, Jan 18, 2023, Michal Luczaj wrote:
>> Signed-off-by: Michal Luczaj <mhal@rbox.co>
>> ---
>>  virt/kvm/coalesced_mmio.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/virt/kvm/coalesced_mmio.c b/virt/kvm/coalesced_mmio.c
>> index 0be80c213f7f..f08f5e82460b 100644
>> --- a/virt/kvm/coalesced_mmio.c
>> +++ b/virt/kvm/coalesced_mmio.c
>> @@ -186,6 +186,7 @@ int kvm_vm_ioctl_unregister_coalesced_mmio(struct kvm *kvm,
>>  		    coalesced_mmio_in_range(dev, zone->addr, zone->size)) {
>>  			r = kvm_io_bus_unregister_dev(kvm,
>>  				zone->pio ? KVM_PIO_BUS : KVM_MMIO_BUS, &dev->dev);
>> +			kvm_iodevice_destructor(&dev->dev);
>>  
>>  			/*
>>  			 * On failure, unregister destroys all devices on the
>> @@ -195,7 +196,6 @@ int kvm_vm_ioctl_unregister_coalesced_mmio(struct kvm *kvm,
>>  			 */
>>  			if (r)
>>  				break;
>> -			kvm_iodevice_destructor(&dev->dev);
> 
> Already posted[1], but didn't get queued because there's alternative solution[2]
> that yields a far cleaner end result, albeit with a larger patch.  I'll follow
> up on Wei's patch to move things along.
> 
> [1] https://lore.kernel.org/all/20221219171924.67989-1-seanjc@google.com
> [2] https://lore.kernel.org/all/20221229123302.4083-1-wei.w.wang@intel.com

I apologise for the noise, I should have searched the archives before posting.

Michal