diff mbox series

[1/3] KVM: Fix coalesced mmio ring buffer out-of-bounds access

Message ID 1568708186-20260-1-git-send-email-wanpengli@tencent.com (mailing list archive)
State New, archived
Headers show
Series [1/3] KVM: Fix coalesced mmio ring buffer out-of-bounds access | expand

Commit Message

Wanpeng Li Sept. 17, 2019, 8:16 a.m. UTC 
From: Wanpeng Li <wanpengli@tencent.com>

Reported by syzkaller:

	#PF: supervisor write access in kernel mode
	#PF: error_code(0x0002) - not-present page
	PGD 403c01067 P4D 403c01067 PUD 0
	Oops: 0002 [#1] SMP PTI
	CPU: 1 PID: 12564 Comm: a.out Tainted: G           OE     5.3.0-rc4+ #4
	RIP: 0010:coalesced_mmio_write+0xcc/0x130 [kvm]
	Call Trace:
	 __kvm_io_bus_write+0x91/0xe0 [kvm]
	 kvm_io_bus_write+0x79/0xf0 [kvm]
	 write_mmio+0xae/0x170 [kvm]
	 emulator_read_write_onepage+0x252/0x430 [kvm]
	 emulator_read_write+0xcd/0x180 [kvm]
	 emulator_write_emulated+0x15/0x20 [kvm]
	 segmented_write+0x59/0x80 [kvm]
	 writeback+0x113/0x250 [kvm]
	 x86_emulate_insn+0x78c/0xd80 [kvm]
	 x86_emulate_instruction+0x386/0x7c0 [kvm]
	 kvm_mmu_page_fault+0xf9/0x9e0 [kvm]
	 handle_ept_violation+0x10a/0x220 [kvm_intel]
	 vmx_handle_exit+0xbe/0x6b0 [kvm_intel]
	 vcpu_enter_guest+0x4dc/0x18d0 [kvm]
	 kvm_arch_vcpu_ioctl_run+0x407/0x660 [kvm]
	 kvm_vcpu_ioctl+0x3ad/0x690 [kvm]
	 do_vfs_ioctl+0xa2/0x690
	 ksys_ioctl+0x6d/0x80
	 __x64_sys_ioctl+0x1a/0x20
	 do_syscall_64+0x74/0x720
	 entry_SYSCALL_64_after_hwframe+0x49/0xbe
	RIP: 0010:coalesced_mmio_write+0xcc/0x130 [kvm]

Both the coalesced_mmio ring buffer indexs ring->first and ring->last are 
bigger than KVM_COALESCED_MMIO_MAX from the testcase, array out-of-bounds 
access triggers by ring->coalesced_mmio[ring->last].phys_addr = addr; 
assignment. This patch fixes it by mod indexs by KVM_COALESCED_MMIO_MAX.

syzkaller source: https://syzkaller.appspot.com/x/repro.c?x=134b2826a00000

Reported-by: syzbot+983c866c3dd6efa3662a@syzkaller.appspotmail.com
Cc: stable@vger.kernel.org
Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
---
 virt/kvm/coalesced_mmio.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Paolo Bonzini Sept. 17, 2019, 8:18 a.m. UTC | #1 
I think we should consider the embargo for CVE-2019-14821 to be broken.
 Since your patch is better, I'll push that one instead as soon as I get
confirmation.

Paolo

On 17/09/19 10:16, Wanpeng Li wrote:
> From: Wanpeng Li <wanpengli@tencent.com>
> 
> Reported by syzkaller:
> 
> 	#PF: supervisor write access in kernel mode
> 	#PF: error_code(0x0002) - not-present page
> 	PGD 403c01067 P4D 403c01067 PUD 0
> 	Oops: 0002 [#1] SMP PTI
> 	CPU: 1 PID: 12564 Comm: a.out Tainted: G           OE     5.3.0-rc4+ #4
> 	RIP: 0010:coalesced_mmio_write+0xcc/0x130 [kvm]
> 	Call Trace:
> 	 __kvm_io_bus_write+0x91/0xe0 [kvm]
> 	 kvm_io_bus_write+0x79/0xf0 [kvm]
> 	 write_mmio+0xae/0x170 [kvm]
> 	 emulator_read_write_onepage+0x252/0x430 [kvm]
> 	 emulator_read_write+0xcd/0x180 [kvm]
> 	 emulator_write_emulated+0x15/0x20 [kvm]
> 	 segmented_write+0x59/0x80 [kvm]
> 	 writeback+0x113/0x250 [kvm]
> 	 x86_emulate_insn+0x78c/0xd80 [kvm]
> 	 x86_emulate_instruction+0x386/0x7c0 [kvm]
> 	 kvm_mmu_page_fault+0xf9/0x9e0 [kvm]
> 	 handle_ept_violation+0x10a/0x220 [kvm_intel]
> 	 vmx_handle_exit+0xbe/0x6b0 [kvm_intel]
> 	 vcpu_enter_guest+0x4dc/0x18d0 [kvm]
> 	 kvm_arch_vcpu_ioctl_run+0x407/0x660 [kvm]
> 	 kvm_vcpu_ioctl+0x3ad/0x690 [kvm]
> 	 do_vfs_ioctl+0xa2/0x690
> 	 ksys_ioctl+0x6d/0x80
> 	 __x64_sys_ioctl+0x1a/0x20
> 	 do_syscall_64+0x74/0x720
> 	 entry_SYSCALL_64_after_hwframe+0x49/0xbe
> 	RIP: 0010:coalesced_mmio_write+0xcc/0x130 [kvm]
> 
> Both the coalesced_mmio ring buffer indexs ring->first and ring->last are 
> bigger than KVM_COALESCED_MMIO_MAX from the testcase, array out-of-bounds 
> access triggers by ring->coalesced_mmio[ring->last].phys_addr = addr; 
> assignment. This patch fixes it by mod indexs by KVM_COALESCED_MMIO_MAX.
> 
> syzkaller source: https://syzkaller.appspot.com/x/repro.c?x=134b2826a00000
> 
> Reported-by: syzbot+983c866c3dd6efa3662a@syzkaller.appspotmail.com
> Cc: stable@vger.kernel.org
> Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
> ---
>  virt/kvm/coalesced_mmio.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/virt/kvm/coalesced_mmio.c b/virt/kvm/coalesced_mmio.c
> index 5294abb..cff1ec9 100644
> --- a/virt/kvm/coalesced_mmio.c
> +++ b/virt/kvm/coalesced_mmio.c
> @@ -73,6 +73,8 @@ static int coalesced_mmio_write(struct kvm_vcpu *vcpu,
>  
>  	spin_lock(&dev->kvm->ring_lock);
>  
> +	ring->first = ring->first % KVM_COALESCED_MMIO_MAX;
> +	ring->last = ring->last % KVM_COALESCED_MMIO_MAX;
>  	if (!coalesced_mmio_has_room(dev)) {
>  		spin_unlock(&dev->kvm->ring_lock);
>  		return -EOPNOTSUPP;
>
Jim Mattson Sept. 17, 2019, 2:58 p.m. UTC | #2 
On Tue, Sep 17, 2019 at 1:16 AM Wanpeng Li <kernellwp@gmail.com> wrote:
>
> From: Wanpeng Li <wanpengli@tencent.com>
>
> Reported by syzkaller:
>
>         #PF: supervisor write access in kernel mode
>         #PF: error_code(0x0002) - not-present page
>         PGD 403c01067 P4D 403c01067 PUD 0
>         Oops: 0002 [#1] SMP PTI
>         CPU: 1 PID: 12564 Comm: a.out Tainted: G           OE     5.3.0-rc4+ #4
>         RIP: 0010:coalesced_mmio_write+0xcc/0x130 [kvm]
>         Call Trace:
>          __kvm_io_bus_write+0x91/0xe0 [kvm]
>          kvm_io_bus_write+0x79/0xf0 [kvm]
>          write_mmio+0xae/0x170 [kvm]
>          emulator_read_write_onepage+0x252/0x430 [kvm]
>          emulator_read_write+0xcd/0x180 [kvm]
>          emulator_write_emulated+0x15/0x20 [kvm]
>          segmented_write+0x59/0x80 [kvm]
>          writeback+0x113/0x250 [kvm]
>          x86_emulate_insn+0x78c/0xd80 [kvm]
>          x86_emulate_instruction+0x386/0x7c0 [kvm]
>          kvm_mmu_page_fault+0xf9/0x9e0 [kvm]
>          handle_ept_violation+0x10a/0x220 [kvm_intel]
>          vmx_handle_exit+0xbe/0x6b0 [kvm_intel]
>          vcpu_enter_guest+0x4dc/0x18d0 [kvm]
>          kvm_arch_vcpu_ioctl_run+0x407/0x660 [kvm]
>          kvm_vcpu_ioctl+0x3ad/0x690 [kvm]
>          do_vfs_ioctl+0xa2/0x690
>          ksys_ioctl+0x6d/0x80
>          __x64_sys_ioctl+0x1a/0x20
>          do_syscall_64+0x74/0x720
>          entry_SYSCALL_64_after_hwframe+0x49/0xbe
>         RIP: 0010:coalesced_mmio_write+0xcc/0x130 [kvm]
>
> Both the coalesced_mmio ring buffer indexs ring->first and ring->last are
> bigger than KVM_COALESCED_MMIO_MAX from the testcase, array out-of-bounds
> access triggers by ring->coalesced_mmio[ring->last].phys_addr = addr;
> assignment. This patch fixes it by mod indexs by KVM_COALESCED_MMIO_MAX.
>
> syzkaller source: https://syzkaller.appspot.com/x/repro.c?x=134b2826a00000
>
> Reported-by: syzbot+983c866c3dd6efa3662a@syzkaller.appspotmail.com
> Cc: stable@vger.kernel.org
> Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
> ---
>  virt/kvm/coalesced_mmio.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/virt/kvm/coalesced_mmio.c b/virt/kvm/coalesced_mmio.c
> index 5294abb..cff1ec9 100644
> --- a/virt/kvm/coalesced_mmio.c
> +++ b/virt/kvm/coalesced_mmio.c
> @@ -73,6 +73,8 @@ static int coalesced_mmio_write(struct kvm_vcpu *vcpu,
>
>         spin_lock(&dev->kvm->ring_lock);
>
> +       ring->first = ring->first % KVM_COALESCED_MMIO_MAX;
> +       ring->last = ring->last % KVM_COALESCED_MMIO_MAX;

I don't think this is sufficient, since the memory that ring points to
is shared with userspace. Userspace can overwrite your corrected
values with illegal ones before they are used. Not exactly a TOCTTOU
issue, since there isn't technically a 'check' here, but the same
idea.

>         if (!coalesced_mmio_has_room(dev)) {
>                 spin_unlock(&dev->kvm->ring_lock);
>                 return -EOPNOTSUPP;
> --
> 2.7.4
>
Matt Delco Sept. 17, 2019, 3:18 p.m. UTC | #3 
On Tue, Sep 17, 2019 at 7:59 AM Jim Mattson <jmattson@google.com> wrote:
> On Tue, Sep 17, 2019 at 1:16 AM Wanpeng Li <kernellwp@gmail.com> wrote:
> > From: Wanpeng Li <wanpengli@tencent.com>
> >
> > Reported by syzkaller:
> >
> >         #PF: supervisor write access in kernel mode
> >         #PF: error_code(0x0002) - not-present page
> >         PGD 403c01067 P4D 403c01067 PUD 0
> >         Oops: 0002 [#1] SMP PTI
> >         CPU: 1 PID: 12564 Comm: a.out Tainted: G           OE     5.3.0-rc4+ #4
> >         RIP: 0010:coalesced_mmio_write+0xcc/0x130 [kvm]
> >         Call Trace:
> >          __kvm_io_bus_write+0x91/0xe0 [kvm]
> >          kvm_io_bus_write+0x79/0xf0 [kvm]
> >          write_mmio+0xae/0x170 [kvm]
> >          emulator_read_write_onepage+0x252/0x430 [kvm]
> >          emulator_read_write+0xcd/0x180 [kvm]
> >          emulator_write_emulated+0x15/0x20 [kvm]
> >          segmented_write+0x59/0x80 [kvm]
> >          writeback+0x113/0x250 [kvm]
> >          x86_emulate_insn+0x78c/0xd80 [kvm]
> >          x86_emulate_instruction+0x386/0x7c0 [kvm]
> >          kvm_mmu_page_fault+0xf9/0x9e0 [kvm]
> >          handle_ept_violation+0x10a/0x220 [kvm_intel]
> >          vmx_handle_exit+0xbe/0x6b0 [kvm_intel]
> >          vcpu_enter_guest+0x4dc/0x18d0 [kvm]
> >          kvm_arch_vcpu_ioctl_run+0x407/0x660 [kvm]
> >          kvm_vcpu_ioctl+0x3ad/0x690 [kvm]
> >          do_vfs_ioctl+0xa2/0x690
> >          ksys_ioctl+0x6d/0x80
> >          __x64_sys_ioctl+0x1a/0x20
> >          do_syscall_64+0x74/0x720
> >          entry_SYSCALL_64_after_hwframe+0x49/0xbe
> >         RIP: 0010:coalesced_mmio_write+0xcc/0x130 [kvm]
> >
> > Both the coalesced_mmio ring buffer indexs ring->first and ring->last are
> > bigger than KVM_COALESCED_MMIO_MAX from the testcase, array out-of-bounds
> > access triggers by ring->coalesced_mmio[ring->last].phys_addr = addr;
> > assignment. This patch fixes it by mod indexs by KVM_COALESCED_MMIO_MAX.
> >
> > syzkaller source: https://syzkaller.appspot.com/x/repro.c?x=134b2826a00000
> >
> > Reported-by: syzbot+983c866c3dd6efa3662a@syzkaller.appspotmail.com
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Wanpeng Li <wanpengli@tencent.com>
> > ---
> >  virt/kvm/coalesced_mmio.c | 2 ++
> >  1 file changed, 2 insertions(+)
> >
> > diff --git a/virt/kvm/coalesced_mmio.c b/virt/kvm/coalesced_mmio.c
> > index 5294abb..cff1ec9 100644
> > --- a/virt/kvm/coalesced_mmio.c
> > +++ b/virt/kvm/coalesced_mmio.c
> > @@ -73,6 +73,8 @@ static int coalesced_mmio_write(struct kvm_vcpu *vcpu,
> >
> >         spin_lock(&dev->kvm->ring_lock);
> >
> > +       ring->first = ring->first % KVM_COALESCED_MMIO_MAX;

This update to first doesn't provide any worthwhile benefit (it's not
used to compute the address of a write) and likely adds the overhead
cost of a 2nd divide operation (via the non-power-of-2 modulus).  If
first is invalid then the app and/or kernel will be confused about
whether the ring is empty or full, but no serious harm will occur (and
since the only write to first is by an app the app is only causing
harm to itself).

> > +       ring->last = ring->last % KVM_COALESCED_MMIO_MAX;
>
> I don't think this is sufficient, since the memory that ring points to
> is shared with userspace. Userspace can overwrite your corrected
> values with illegal ones before they are used. Not exactly a TOCTTOU
> issue, since there isn't technically a 'check' here, but the same
> idea.
>
> >         if (!coalesced_mmio_has_room(dev)) {
> >                 spin_unlock(&dev->kvm->ring_lock);
> >                 return -EOPNOTSUPP;
> > --
> > 2.7.4
> >
diff mbox series

Patch

diff --git a/virt/kvm/coalesced_mmio.c b/virt/kvm/coalesced_mmio.c
index 5294abb..cff1ec9 100644
--- a/virt/kvm/coalesced_mmio.c
+++ b/virt/kvm/coalesced_mmio.c
@@ -73,6 +73,8 @@  static int coalesced_mmio_write(struct kvm_vcpu *vcpu,
 
 	spin_lock(&dev->kvm->ring_lock);
 
+	ring->first = ring->first % KVM_COALESCED_MMIO_MAX;
+	ring->last = ring->last % KVM_COALESCED_MMIO_MAX;
 	if (!coalesced_mmio_has_room(dev)) {
 		spin_unlock(&dev->kvm->ring_lock);
 		return -EOPNOTSUPP;