[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20130429075144.GA16785@redhat.com>
Date: Mon, 29 Apr 2013 10:51:44 +0300
From: "Michael S. Tsirkin" <mst@...hat.com>
To: Kurt Seifried <kseifried@...hat.com>
Cc: Jason Wang <jasowang@...hat.com>, Kurt Seifried <kseifrie@...hat.com>,
aliguori@...ibm.com, qemu-devel@...gnu.org,
oss-security@...ts.openwall.com
Subject: Re: Re: [PATCH 1/3] virtio-pci: properly validate
address before accessing config
On Mon, Apr 29, 2013 at 01:34:44AM -0600, Kurt Seifried wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 04/28/2013 05:29 AM, Petr Matousek wrote:
> > 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,
> >
>
> Ok so this does NOT affect Linux systems using mmap_min_addr with a
> sane default value (e.g. larger than 0). However more than a few
> systems have shipped with mmap_min_addr set to 0, not picking on
> Debian, just the first result I found:
>
> http://wiki.debian.org/mmap_min_addr
>
> "In Debian 5.0.0 through 5.0.3 inclusive, the 2.6.26 kernel is shipped
> with a default mmap_min_addr of '0'."
>
> Hopefully everyone has upgraded to 5.0.4 =).
>
> So there is a realistic potential for systems to be affected (e.g. if
> this had been fixed 10 years ago I'd have probably not given a CVE,
> but 3 years is not a super long time).
>
> Please use CVE-2013-2016 for virtio-pci: properly validate address
> before accessing config.
>
Please note this only affects virtio-rng which appeared in qemu v1.3.0 and on,
released on Mon Dec 3 2012. So you'd need a distro with an old kernel
that has an updated qemu or qemu-kvm (or backported the bug in virtio-rng).
--
MST
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.
