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 <m...@redhat.com> Cc: Petr Matousek > >>>> <pmato...@redhat.com> Signed-off-by: Jason Wang > >>>> <jasow...@redhat.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