| Message ID | 1366965244-20542-1-git-send-email-jasowang@redhat.com |
|---|---|
| State | New |
| Headers | show |
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@redhat.com> > Cc: Petr Matousek <pmatouse@redhat.com> > Signed-off-by: Jason Wang <jasowang@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? 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?
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@redhat.com> >> Cc: Petr Matousek <pmatouse@redhat.com> >> Signed-off-by: Jason Wang <jasowang@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(). > > 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.
On Fri, Apr 26, 2013 at 04:27:55PM +0200, 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@redhat.com> > > Cc: Petr Matousek <pmatouse@redhat.com> > > Signed-off-by: Jason Wang <jasowang@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? > > 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? Good point. I think it's only the first page of the qemu address space. > -- > Petr Matousek / Red Hat Security Response Team
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@redhat.com> > Cc: Petr Matousek <pmatouse@redhat.com> > Signed-off-by: Jason Wang <jasowang@redhat.com> Why do this in virtio-pci and not in virtio.c? If instead we correct the checks in virtio.c we get less code, and all transports will benefit automatically. > --- > 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; > + } > + > switch (size) { > case 1: > val = virtio_config_readb(proxy->vdev, addr); > @@ -430,6 +434,11 @@ static void virtio_pci_config_write(void *opaque, hwaddr addr, > return; > } > addr -= config; > + > + if (addr + size > proxy->vdev->config_len) { > + return; > + } > + > /* > * Virtio-PCI is odd. Ioports are LE but config space is target native > * endian. > diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > index 1c2282c..3397b5e 100644 > --- a/hw/virtio/virtio.c > +++ b/hw/virtio/virtio.c > @@ -563,9 +563,6 @@ uint32_t virtio_config_readb(VirtIODevice *vdev, uint32_t addr) > > vdev->get_config(vdev, vdev->config); > > - if (addr > (vdev->config_len - sizeof(val))) > - return (uint32_t)-1; > - > val = ldub_p(vdev->config + addr); > return val; > } > @@ -576,9 +573,6 @@ uint32_t virtio_config_readw(VirtIODevice *vdev, uint32_t addr) > > vdev->get_config(vdev, vdev->config); > > - if (addr > (vdev->config_len - sizeof(val))) > - return (uint32_t)-1; > - > val = lduw_p(vdev->config + addr); > return val; > } > @@ -589,9 +583,6 @@ uint32_t virtio_config_readl(VirtIODevice *vdev, uint32_t addr) > > vdev->get_config(vdev, vdev->config); > > - if (addr > (vdev->config_len - sizeof(val))) > - return (uint32_t)-1; > - > val = ldl_p(vdev->config + addr); > return val; > } > @@ -600,9 +591,6 @@ void virtio_config_writeb(VirtIODevice *vdev, uint32_t addr, uint32_t data) > { > uint8_t val = data; > > - if (addr > (vdev->config_len - sizeof(val))) > - return; > - > stb_p(vdev->config + addr, val); > > if (vdev->set_config) > @@ -613,9 +601,6 @@ 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) > @@ -626,9 +611,6 @@ void virtio_config_writel(VirtIODevice *vdev, uint32_t addr, uint32_t data) > { > uint32_t val = data; > > - if (addr > (vdev->config_len - sizeof(val))) > - return; > - > stl_p(vdev->config + addr, val); > > if (vdev->set_config) > -- > 1.7.1
On 04/28/2013 03:26 AM, Michael S. Tsirkin 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@redhat.com> >> Cc: Petr Matousek <pmatouse@redhat.com> >> Signed-off-by: Jason Wang <jasowang@redhat.com> > Why do this in virtio-pci and not in virtio.c? > If instead we correct the checks in virtio.c we > get less code, and all transports will benefit > automatically. I wish I could but looks like vitio_config_read{b|w|l} were only used by virtio-pci. Other transport such as ccw and s390-virtio-bus have their own implementation. > >> --- >> 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; >> + } >> + >> switch (size) { >> case 1: >> val = virtio_config_readb(proxy->vdev, addr); >> @@ -430,6 +434,11 @@ static void virtio_pci_config_write(void *opaque, hwaddr addr, >> return; >> } >> addr -= config; >> + >> + if (addr + size > proxy->vdev->config_len) { >> + return; >> + } >> + >> /* >> * Virtio-PCI is odd. Ioports are LE but config space is target native >> * endian. >> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c >> index 1c2282c..3397b5e 100644 >> --- a/hw/virtio/virtio.c >> +++ b/hw/virtio/virtio.c >> @@ -563,9 +563,6 @@ uint32_t virtio_config_readb(VirtIODevice *vdev, uint32_t addr) >> >> vdev->get_config(vdev, vdev->config); >> >> - if (addr > (vdev->config_len - sizeof(val))) >> - return (uint32_t)-1; >> - >> val = ldub_p(vdev->config + addr); >> return val; >> } >> @@ -576,9 +573,6 @@ uint32_t virtio_config_readw(VirtIODevice *vdev, uint32_t addr) >> >> vdev->get_config(vdev, vdev->config); >> >> - if (addr > (vdev->config_len - sizeof(val))) >> - return (uint32_t)-1; >> - >> val = lduw_p(vdev->config + addr); >> return val; >> } >> @@ -589,9 +583,6 @@ uint32_t virtio_config_readl(VirtIODevice *vdev, uint32_t addr) >> >> vdev->get_config(vdev, vdev->config); >> >> - if (addr > (vdev->config_len - sizeof(val))) >> - return (uint32_t)-1; >> - >> val = ldl_p(vdev->config + addr); >> return val; >> } >> @@ -600,9 +591,6 @@ void virtio_config_writeb(VirtIODevice *vdev, uint32_t addr, uint32_t data) >> { >> uint8_t val = data; >> >> - if (addr > (vdev->config_len - sizeof(val))) >> - return; >> - >> stb_p(vdev->config + addr, val); >> >> if (vdev->set_config) >> @@ -613,9 +601,6 @@ 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) >> @@ -626,9 +611,6 @@ void virtio_config_writel(VirtIODevice *vdev, uint32_t addr, uint32_t data) >> { >> uint32_t val = data; >> >> - if (addr > (vdev->config_len - sizeof(val))) >> - return; >> - >> stl_p(vdev->config + addr, val); >> >> if (vdev->set_config) >> -- >> 1.7.1
On Sun, Apr 28, 2013 at 03:54:20PM +0800, Jason Wang wrote: > On 04/28/2013 03:26 AM, Michael S. Tsirkin 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@redhat.com> > >> Cc: Petr Matousek <pmatouse@redhat.com> > >> Signed-off-by: Jason Wang <jasowang@redhat.com> > > Why do this in virtio-pci and not in virtio.c? > > If instead we correct the checks in virtio.c we > > get less code, and all transports will benefit > > automatically. > > I wish I could but looks like vitio_config_read{b|w|l} were only used by > virtio-pci. Other transport such as ccw and s390-virtio-bus have their > own implementation. Okay but still, the bug is in checks in virtio.c, why not fix it there instead of making it assume caller does the checks? > > > >> --- > >> 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; > >> + } > >> + > >> switch (size) { > >> case 1: > >> val = virtio_config_readb(proxy->vdev, addr); > >> @@ -430,6 +434,11 @@ static void virtio_pci_config_write(void *opaque, hwaddr addr, > >> return; > >> } > >> addr -= config; > >> + > >> + if (addr + size > proxy->vdev->config_len) { > >> + return; > >> + } > >> + > >> /* > >> * Virtio-PCI is odd. Ioports are LE but config space is target native > >> * endian. > >> diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c > >> index 1c2282c..3397b5e 100644 > >> --- a/hw/virtio/virtio.c > >> +++ b/hw/virtio/virtio.c > >> @@ -563,9 +563,6 @@ uint32_t virtio_config_readb(VirtIODevice *vdev, uint32_t addr) > >> > >> vdev->get_config(vdev, vdev->config); > >> > >> - if (addr > (vdev->config_len - sizeof(val))) > >> - return (uint32_t)-1; > >> - > >> val = ldub_p(vdev->config + addr); > >> return val; > >> } > >> @@ -576,9 +573,6 @@ uint32_t virtio_config_readw(VirtIODevice *vdev, uint32_t addr) > >> > >> vdev->get_config(vdev, vdev->config); > >> > >> - if (addr > (vdev->config_len - sizeof(val))) > >> - return (uint32_t)-1; > >> - > >> val = lduw_p(vdev->config + addr); > >> return val; > >> } > >> @@ -589,9 +583,6 @@ uint32_t virtio_config_readl(VirtIODevice *vdev, uint32_t addr) > >> > >> vdev->get_config(vdev, vdev->config); > >> > >> - if (addr > (vdev->config_len - sizeof(val))) > >> - return (uint32_t)-1; > >> - > >> val = ldl_p(vdev->config + addr); > >> return val; > >> } > >> @@ -600,9 +591,6 @@ void virtio_config_writeb(VirtIODevice *vdev, uint32_t addr, uint32_t data) > >> { > >> uint8_t val = data; > >> > >> - if (addr > (vdev->config_len - sizeof(val))) > >> - return; > >> - > >> stb_p(vdev->config + addr, val); > >> > >> if (vdev->set_config) > >> @@ -613,9 +601,6 @@ 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) > >> @@ -626,9 +611,6 @@ void virtio_config_writel(VirtIODevice *vdev, uint32_t addr, uint32_t data) > >> { > >> uint32_t val = data; > >> > >> - if (addr > (vdev->config_len - sizeof(val))) > >> - return; > >> - > >> stl_p(vdev->config + addr, val); > >> > >> if (vdev->set_config) > >> -- > >> 1.7.1
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@redhat.com> > >> Cc: Petr Matousek <pmatouse@redhat.com> > >> Signed-off-by: Jason Wang <jasowang@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,
-----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@redhat.com> Cc: Petr Matousek >>>> <pmatouse@redhat.com> Signed-off-by: Jason Wang >>>> <jasowang@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. - -- Kurt Seifried Red Hat Security Response Team (SRT) PGP: 0x5E267993 A90B F995 7350 148F 66BF 7554 160D 4553 5E26 7993 -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.13 (GNU/Linux) iQIcBAEBAgAGBQJRfiKUAAoJEBYNRVNeJnmTcQ0P/iCx/Wz1piIDwvtcEHDrrgJK pSEztM+1RUC8fsqPjwyaOijupGhMMtmtfJpuhlNnzNUSaWpX7p75d7JMMOWsTztI wkxOPIso0Zrj7susqYrg7Qiw6G9FJ6sTZFcSWbWFFHieMiVZZMZ0zKe8Vqh1gnv6 cy8OPIDPRT7mkj8lRhYvvzZjIA4S4cNs3DCSvqYn/vwVX/XQI7IQ9f5zz6wrCjLm prZjHKzZj1MO6C0HLdTw7TWd/gauhbPRyxn13Kb7sk6CeoAx1ip2AX6pS5xILLPE RV55j46MRosqr9C+V9I2ljQTkRUATLbIM8ny7mxHwB1JYtSn6djL3ID4GJz5Q3RY TiCiLACqlahxOWv3UDtx60+mOeiZvhE8gP8TS4LexeqIYIuppP93xq1NHZgqYcQT q/YXxTXBQ/oueXXd2ktiiHwCyWj8FIqnsEIUIDSifpXmlbLPtVxD4MDmqHm+P34m e4Po3kD/y5AO04qy7M1TbyHunIOXBzQnAmNlzAD4Iw/ou0PSuZP8hB+ZCtJUObKv BxHXsLGieHmgZ/7qkkxLw9XAZtW6A9Fu+0yHoU7Ur60YQTg/urZhAuFSv6Cm8vti YRq/xL2+GWG5SzKOBPJ28TUy1bpb3m0tg4bhzzqo8ikruKXr/RhvsQ2BPDaA47F0 oN6qXXGSlpx3XlpE29sr =IDuh -----END PGP SIGNATURE-----
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@redhat.com> Cc: Petr Matousek > >>>> <pmatouse@redhat.com> Signed-off-by: Jason Wang > >>>> <jasowang@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).
Am 28.04.2013 10:35, schrieb Michael S. Tsirkin: > On Sun, Apr 28, 2013 at 03:54:20PM +0800, Jason Wang wrote: >> On 04/28/2013 03:26 AM, Michael S. Tsirkin 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@redhat.com> >>>> Cc: Petr Matousek <pmatouse@redhat.com> >>>> Signed-off-by: Jason Wang <jasowang@redhat.com> >>> Why do this in virtio-pci and not in virtio.c? >>> If instead we correct the checks in virtio.c we >>> get less code, and all transports will benefit >>> automatically. >> >> I wish I could but looks like vitio_config_read{b|w|l} were only used by >> virtio-pci. Other transport such as ccw and s390-virtio-bus have their >> own implementation. > > Okay but still, the bug is in checks in virtio.c, why not fix it there > instead of making it assume caller does the checks? Ping? This issue has been assigned a CVE but the solution does not seem to be agreed on yet - are you working on a different proposal, Jason? Thanks, Andreas
On 05/02/2013 10:35 PM, Andreas Färber wrote: > Am 28.04.2013 10:35, schrieb Michael S. Tsirkin: >> On Sun, Apr 28, 2013 at 03:54:20PM +0800, Jason Wang wrote: >>> On 04/28/2013 03:26 AM, Michael S. Tsirkin 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@redhat.com> >>>>> Cc: Petr Matousek <pmatouse@redhat.com> >>>>> Signed-off-by: Jason Wang <jasowang@redhat.com> >>>> Why do this in virtio-pci and not in virtio.c? >>>> If instead we correct the checks in virtio.c we >>>> get less code, and all transports will benefit >>>> automatically. >>> I wish I could but looks like vitio_config_read{b|w|l} were only used by >>> virtio-pci. Other transport such as ccw and s390-virtio-bus have their >>> own implementation. >> Okay but still, the bug is in checks in virtio.c, why not fix it there >> instead of making it assume caller does the checks? > Ping? This issue has been assigned a CVE but the solution does not seem > to be agreed on yet - are you working on a different proposal, Jason? > > Thanks, > Andreas > Hi, I was just back from vacation, will draft V2 soon according to Michael's comments. Thanks
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; + } + switch (size) { case 1: val = virtio_config_readb(proxy->vdev, addr); @@ -430,6 +434,11 @@ static void virtio_pci_config_write(void *opaque, hwaddr addr, return; } addr -= config; + + if (addr + size > proxy->vdev->config_len) { + return; + } + /* * Virtio-PCI is odd. Ioports are LE but config space is target native * endian. diff --git a/hw/virtio/virtio.c b/hw/virtio/virtio.c index 1c2282c..3397b5e 100644 --- a/hw/virtio/virtio.c +++ b/hw/virtio/virtio.c @@ -563,9 +563,6 @@ uint32_t virtio_config_readb(VirtIODevice *vdev, uint32_t addr) vdev->get_config(vdev, vdev->config); - if (addr > (vdev->config_len - sizeof(val))) - return (uint32_t)-1; - val = ldub_p(vdev->config + addr); return val; } @@ -576,9 +573,6 @@ uint32_t virtio_config_readw(VirtIODevice *vdev, uint32_t addr) vdev->get_config(vdev, vdev->config); - if (addr > (vdev->config_len - sizeof(val))) - return (uint32_t)-1; - val = lduw_p(vdev->config + addr); return val; } @@ -589,9 +583,6 @@ uint32_t virtio_config_readl(VirtIODevice *vdev, uint32_t addr) vdev->get_config(vdev, vdev->config); - if (addr > (vdev->config_len - sizeof(val))) - return (uint32_t)-1; - val = ldl_p(vdev->config + addr); return val; } @@ -600,9 +591,6 @@ void virtio_config_writeb(VirtIODevice *vdev, uint32_t addr, uint32_t data) { uint8_t val = data; - if (addr > (vdev->config_len - sizeof(val))) - return; - stb_p(vdev->config + addr, val); if (vdev->set_config) @@ -613,9 +601,6 @@ 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) @@ -626,9 +611,6 @@ void virtio_config_writel(VirtIODevice *vdev, uint32_t addr, uint32_t data) { uint32_t val = data; - if (addr > (vdev->config_len - sizeof(val))) - return; - stl_p(vdev->config + addr, val); if (vdev->set_config)
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@redhat.com> Cc: Petr Matousek <pmatouse@redhat.com> Signed-off-by: Jason Wang <jasowang@redhat.com> --- hw/virtio/virtio-pci.c | 9 +++++++++ hw/virtio/virtio.c | 18 ------------------ 2 files changed, 9 insertions(+), 18 deletions(-)