diff mbox

[1/5] virtio-pci: ignore unaligned read/write in virtio_address_space_read()/write()

Message ID 20150713102533-mutt-send-email-mst@redhat.com
State New
Headers show

Commit Message

Michael S. Tsirkin July 13, 2015, 7:36 a.m. UTC 
On Mon, Jul 13, 2015 at 01:46:47PM +0800, Jason Wang wrote:
> We abort on unaligned read/write in
> virtio_address_space_read()/write() but since len in under control of
> guest so qemu will simply crash when booting a modern guest (guest is
> try to read when len is zero).
> read.

How can len be 0? Isn't this a guest bug? Or is this
a theoretical issue?

> Fix this by ignoring unaligned write or
> 
> Fixes 1e40356ce5f6ccfa0bb57104a533c62952c560ce
> ("virtio fix cfg endian-ness for BE targets")
> Signed-off-by: Jason Wang <jasowang@redhat.com>

I guess since we ignore some illegal values (e.g. > 4)
we should just whitelist the legal ones.
So the following looks like a slightly cleaner way to
make this change.

--->
virtio-pci: don't crash on illegal length

Some guests seem to access cfg with an illegal length value.
It's worth fixing them but debugging is easier if
qemu does not crash.

Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

Comments

Gerd Hoffmann July 13, 2015, 7:53 a.m. UTC | #1 
On Mo, 2015-07-13 at 10:36 +0300, Michael S. Tsirkin wrote:
> On Mon, Jul 13, 2015 at 01:46:47PM +0800, Jason Wang wrote:
> > We abort on unaligned read/write in
> > virtio_address_space_read()/write() but since len in under control of
> > guest so qemu will simply crash when booting a modern guest (guest is
> > try to read when len is zero).
> > read.
> 
> How can len be 0? Isn't this a guest bug? Or is this
> a theoretical issue?

Something dumping pci config space?
With pci access capability not being used before and therefore zeroed?
Then hitting the "data" field will trigger a zero-length read.

That assert actually triggers when booting a recent linux kernel with
disable-modern=off

cheers,
  Gerd
Michael S. Tsirkin July 13, 2015, 8 a.m. UTC | #2 
On Mon, Jul 13, 2015 at 09:53:43AM +0200, Gerd Hoffmann wrote:
> On Mo, 2015-07-13 at 10:36 +0300, Michael S. Tsirkin wrote:
> > On Mon, Jul 13, 2015 at 01:46:47PM +0800, Jason Wang wrote:
> > > We abort on unaligned read/write in
> > > virtio_address_space_read()/write() but since len in under control of
> > > guest so qemu will simply crash when booting a modern guest (guest is
> > > try to read when len is zero).
> > > read.
> > 
> > How can len be 0? Isn't this a guest bug? Or is this
> > a theoretical issue?
> 
> Something dumping pci config space?
> With pci access capability not being used before and therefore zeroed?
> Then hitting the "data" field will trigger a zero-length read.

I suspect so, yes. All this worries me: what if length was not 0
because the capability was previously used e.g. by bios?

> That assert actually triggers when booting a recent linux kernel with
> disable-modern=off
> 
> cheers,
>   Gerd
> 

Which linux version?  Doesn't seem to trigger for me ...
Jason Wang July 13, 2015, 8:37 a.m. UTC | #3 
On 07/13/2015 03:36 PM, Michael S. Tsirkin wrote:
> On Mon, Jul 13, 2015 at 01:46:47PM +0800, Jason Wang wrote:
>> We abort on unaligned read/write in
>> virtio_address_space_read()/write() but since len in under control of
>> guest so qemu will simply crash when booting a modern guest (guest is
>> try to read when len is zero).
>> read.
> How can len be 0? Isn't this a guest bug? Or is this
> a theoretical issue?

E.g cat /sys/bus/pci/devices/0000\:00\:03.0/config
and also happen during boot (but not virtio specific code, probably pci
core or something else).

>
>> Fix this by ignoring unaligned write or
>>
>> Fixes 1e40356ce5f6ccfa0bb57104a533c62952c560ce
>> ("virtio fix cfg endian-ness for BE targets")
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> I guess since we ignore some illegal values (e.g. > 4)
> we should just whitelist the legal ones.
> So the following looks like a slightly cleaner way to
> make this change.
>
> --->
> virtio-pci: don't crash on illegal length
>
> Some guests seem to access cfg with an illegal length value.
> It's worth fixing them but debugging is easier if
> qemu does not crash.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>

I believe when we can, we should avoid guest trigger-able abort.

>
> diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
> index 6ca0258..c5e8cc0 100644
> --- a/hw/virtio/virtio-pci.c
> +++ b/hw/virtio/virtio-pci.c
> @@ -546,7 +546,8 @@ static void virtio_write_config(PCIDevice *pci_dev, uint32_t address,
>          off = le32_to_cpu(cfg->cap.offset);
>          len = le32_to_cpu(cfg->cap.length);
>  
> -        if (len <= sizeof cfg->pci_cfg_data) {
> +        if (len == 1 || len == 2 || len == 4) {
> +            assert(len <= sizeof cfg->pci_cfg_data);
>              virtio_address_space_write(&proxy->modern_as, off,
>                                         cfg->pci_cfg_data, len);
>          }
> @@ -570,7 +571,8 @@ static uint32_t virtio_read_config(PCIDevice *pci_dev,
>          off = le32_to_cpu(cfg->cap.offset);
>          len = le32_to_cpu(cfg->cap.length);
>  
> -        if (len <= sizeof cfg->pci_cfg_data) {
> +        if (len == 1 || len == 2 || len == 4) {
> +            assert(len <= sizeof cfg->pci_cfg_data);
>              virtio_address_space_read(&proxy->modern_as, off,
>                                        cfg->pci_cfg_data, len);
>          }
>
Gerd Hoffmann July 13, 2015, 8:39 a.m. UTC | #4 
On Mo, 2015-07-13 at 11:00 +0300, Michael S. Tsirkin wrote:
> On Mon, Jul 13, 2015 at 09:53:43AM +0200, Gerd Hoffmann wrote:
> > On Mo, 2015-07-13 at 10:36 +0300, Michael S. Tsirkin wrote:
> > > On Mon, Jul 13, 2015 at 01:46:47PM +0800, Jason Wang wrote:
> > > > We abort on unaligned read/write in
> > > > virtio_address_space_read()/write() but since len in under control of
> > > > guest so qemu will simply crash when booting a modern guest (guest is
> > > > try to read when len is zero).
> > > > read.
> > > 
> > > How can len be 0? Isn't this a guest bug? Or is this
> > > a theoretical issue?
> > 
> > Something dumping pci config space?
> > With pci access capability not being used before and therefore zeroed?
> > Then hitting the "data" field will trigger a zero-length read.
> 
> I suspect so, yes. All this worries me: what if length was not 0
> because the capability was previously used e.g. by bios?
> 
> > That assert actually triggers when booting a recent linux kernel with
> > disable-modern=off
> > 
> > cheers,
> >   Gerd
> > 
> 
> Which linux version?  Doesn't seem to trigger for me ... 

Fedora 22 guest with latest distro kernel (4.0.7)

cheers,
  Gerd
diff mbox

Patch

diff --git a/hw/virtio/virtio-pci.c b/hw/virtio/virtio-pci.c
index 6ca0258..c5e8cc0 100644
--- a/hw/virtio/virtio-pci.c
+++ b/hw/virtio/virtio-pci.c
@@ -546,7 +546,8 @@  static void virtio_write_config(PCIDevice *pci_dev, uint32_t address,
         off = le32_to_cpu(cfg->cap.offset);
         len = le32_to_cpu(cfg->cap.length);
 
-        if (len <= sizeof cfg->pci_cfg_data) {
+        if (len == 1 || len == 2 || len == 4) {
+            assert(len <= sizeof cfg->pci_cfg_data);
             virtio_address_space_write(&proxy->modern_as, off,
                                        cfg->pci_cfg_data, len);
         }
@@ -570,7 +571,8 @@  static uint32_t virtio_read_config(PCIDevice *pci_dev,
         off = le32_to_cpu(cfg->cap.offset);
         len = le32_to_cpu(cfg->cap.length);
 
-        if (len <= sizeof cfg->pci_cfg_data) {
+        if (len == 1 || len == 2 || len == 4) {
+            assert(len <= sizeof cfg->pci_cfg_data);
             virtio_address_space_read(&proxy->modern_as, off,
                                       cfg->pci_cfg_data, len);
         }