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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 17 Sep 2019 08:18:42 -0700
From:   Matt Delco <delco@...omium.org>
To:     Wanpeng Li <kernellwp@...il.com>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Jim Mattson <jmattson@...gle.com>,
        kvm list <kvm@...r.kernel.org>,
        Paolo Bonzini <pbonzini@...hat.com>,
        Radim Krčmář <rkrcmar@...hat.com>,
        Sean Christopherson <sean.j.christopherson@...el.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Joerg Roedel <joro@...tes.org>, stable@...r.kernel.org
Subject: Re: [PATCH 1/3] KVM: Fix coalesced mmio ring buffer out-of-bounds access

On Tue, Sep 17, 2019 at 7:59 AM Jim Mattson <jmattson@...gle.com> wrote:
> On Tue, Sep 17, 2019 at 1:16 AM Wanpeng Li <kernellwp@...il.com> wrote:
> > From: Wanpeng Li <wanpengli@...cent.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@...kaller.appspotmail.com
> > Cc: stable@...r.kernel.org
> > Signed-off-by: Wanpeng Li <wanpengli@...cent.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
> >