[<prev] [next>] [thread-next>] [day] [month] [year] [list]
Message-ID: <20130428112936.GP8388@dhcp-25-225.brq.redhat.com>
Date: Sun, 28 Apr 2013 13:29:36 +0200
From: Petr Matousek <pmatouse@...hat.com>
To: Jason Wang <jasowang@...hat.com>, Kurt Seifried <kseifrie@...hat.com>
Cc: aliguori@...ibm.com, mst@...hat.com, qemu-devel@...gnu.org,
oss-security@...ts.openwall.com
Subject: Re: [PATCH 1/3] virtio-pci: properly validate address before
accessing config
On Sat, Apr 27, 2013 at 01:13:16PM +0800, Jason Wang wrote:
> On 04/26/2013 10:27 PM, Petr Matousek wrote:
> > On Fri, Apr 26, 2013 at 04:34:02PM +0800, Jason Wang wrote:
> >> There are several several issues in the current checking:
> >>
> >> - The check was based on the minus of unsigned values which can overflow
> >> - It was done after .{set|get}_config() which can lead crash when config_len is
> >> zero since vdev->config is NULL
> >>
> >> Fix this by:
> >>
> >> - Validate the address in virtio_pci_config_{read|write}() before
> >> .{set|get}_config
> >> - Use addition instead minus to do the validation
> >>
> >> Cc: Michael S. Tsirkin <mst@...hat.com>
> >> Cc: Petr Matousek <pmatouse@...hat.com>
> >> Signed-off-by: Jason Wang <jasowang@...hat.com>
> >> ---
> >> hw/virtio/virtio-pci.c | 9 +++++++++
> >> hw/virtio/virtio.c | 18 ------------------
> >> 2 files changed, 9 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> >> index a1f15a8..7f6c7d1 100644
> >> --- a/hw/virtio/virtio-pci.c
> >> +++ b/hw/virtio/virtio-pci.c
> >> @@ -400,6 +400,10 @@ static uint64_t virtio_pci_config_read(void *opaque, hwaddr addr,
> >> }
> >> addr -= config;
> >>
> >> + if (addr + size > proxy->vdev->config_len) {
> >> + return (uint32_t)-1;
> >> + }
> >> +
> > What is the range of values addr can be? I guess it's not arbitrary and
> > not fully in guests hands. Can it be higher than corresponding pci
> > config space size?
>
> Not fully in guests hands. It depends on size the config size.
> Unfortunately, qemu will roundup the size to power of 2 in
> virtio_pci_device_plugged():
>
> size = VIRTIO_PCI_REGION_SIZE(&proxy->pci_dev)
> + virtio_bus_get_vdev_config_len(bus);
>
> if (size & (size - 1)) {
> size = 1 << qemu_fls(size);
> }
>
> So, for virtio-rng, though its region size is 20, it will be rounded up
> to 32, which left guest the possibility to access beyond the config
> space. So some check is needs in virito_pci_config_read().
Ok, in that case it would make sense to document the preconditions that
assures that addr + size won't overflow. Or add the values in a safe way
(check that the sum is not less than one of the addend).
> > IOW, can guest touch anything interesting or will all accesses end in
> > the first page in the qemu address space, considering vdev->config being
> > NULL?
> >
>
> There's another theoretical issue as pointed by Anthony, see
> virtio_config_writew():
>
> void virtio_config_writew(VirtIODevice *vdev, uint32_t addr, uint32_t data)
> {
> uint16_t val = data;
>
> if (addr > (vdev->config_len - sizeof(val)))
> return;
>
> stw_p(vdev->config + addr, val);
>
> if (vdev->set_config)
> vdev->set_config(vdev, vdev->config);
> }
>
> If there's a device whose config_len is 1, the check will fail and we
> can access some other location.
>
> But since virtio-rng has zero config length and addr here should be less
> than 12, and all other device's config length is all greater than 4.
> Only first page could be access here.
So the only practical attack (virtio-rng device that has config length
0) can only end in the first page of qemu address space which is on any
not-so-much recent kernel protected by mmap_min_addr and will result in
qemu process crash. Access to pci config space is privileged operation,
so root user in the guest can crash the guest (something that root can
do anyways).
Don't get me wrong, we still need the fix to avoid any potential issues
in the future, but I'm leaning towards not treating this issue as a
security (CVE) one due to the lack of practical exploitability.
@Kurt -- do we assign CVE identifiers to issues that rely on an option
(or lack of) that when set in a way that would allow the issue in
question to be exploited is known to be insecure?
References:
https://lists.gnu.org/archive/html/qemu-devel/2013-04/msg05013.html
https://bugzilla.redhat.com/show_bug.cgi?id=957155
The option is mmap_min_addr which assures that no mapping can be present
at the beginning of the address space and all accesses will result in
sigsegv. Default setting of mmap_min_addr is enough to avoid this issue
from having security consequences. Disabling mmap_min_addr (setting to
0) means that some if not all of the "kernel NULL pointer dereferences"
out there could be used for privilege escalation.
Thanks,
--
Petr Matousek / Red Hat Security Response Team
Powered by blists - more mailing lists
Please check out the
Open Source Software Security Wiki, which is counterpart to this
mailing list.
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
