[Qemu-devel] [0/4] Assorted bugfixes

[Qemu-devel] [0/4] Assorted bugfixes

[David Gibson]

Hi Anthony,

Here are several simple non-target-specific bugfixes for qemu.  Please apply.

[Qemu-devel] [PATCH 1/4] load_image_targphys() should enforce the max size

[David Gibson]

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>

load_image_targphys() gets passed a max size for the file, but
doesn't enforce it at all. Add a check and return -1 (error) if
the file is too big, without loading it.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/loader.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/hw/loader.c b/hw/loader.c
index 446b628..7ad9e22 100644
--- a/hw/loader.c
+++ b/hw/loader.c
@@ -108,6 +108,8 @@ int load_image_targphys(const char *filename,
     int size;
 
     size = get_image_size(filename);
+    if (size > max_sz)
+	return -1;
     if (size > 0)
         rom_add_file_fixed(filename, addr, -1);
     return size;
-- 
1.7.7.3

[Qemu-devel] [PATCH 2/4] Fix dirty logging with 32-bit qemu & 64-bit guests

[David Gibson]

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>

The kvm_get_dirty_pages_log_range() function uses two address
variables to step through the monitored memory region to update the
dirty log.  However, these variables have type unsigned long, which
can overflow if running a 64-bit guest with a 32-bit qemu binary.

This patch changes these to target_phys_addr_t which will have the
correct size.

Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 kvm-all.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/kvm-all.c b/kvm-all.c
index 3174f42..363c697 100644
--- a/kvm-all.c
+++ b/kvm-all.c
@@ -344,7 +344,8 @@ static int kvm_get_dirty_pages_log_range(MemoryRegionSection *section,
                                          unsigned long *bitmap)
 {
     unsigned int i, j;
-    unsigned long page_number, addr, addr1, c;
+    unsigned long page_number, c;
+    target_phys_addr_t addr, addr1;
     unsigned int len = ((section->size / TARGET_PAGE_SIZE) + HOST_LONG_BITS - 1) / HOST_LONG_BITS;
 
     /*
-- 
1.7.7.3

[Qemu-devel] [PATCH 3/4] pci: Make bounds checks on config space accesses actually work

[David Gibson]

The pci_host_config_{read,write}_common() functions perform PCI config
accesses.  They take a limit parameter which they appear to be supposed
to bounds check against, however the bounds checking logic, such as it is,
is completely broken.

Currently, it takes the minimum of the supplied length and the remaining
space in the region and passes that as the length to the underlying
config_{read,write} function pointer.  This means that accesses which
partially overrun the region will be silently truncated - which makes
little sense.  Accesses which entirely overrun the region will *not*
be blocked (an exploitable bug), because in that case (limit - addr) will
be negative and so the unsigned MIN will always return len instead.  Even
if signed arithmetic was used, the config_{read,write} callback wouldn't
know what to do with a negative len parameter.

This patch handles things more sanely by simply ignoring writes which
overrun, and returning -1 for reads, which is the usual hardware convention
for reads to unpopulated IO regions.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 hw/pci_host.c |   10 ++++++++--
 1 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/hw/pci_host.c b/hw/pci_host.c
index 44c6c20..16b3ac3 100644
--- a/hw/pci_host.c
+++ b/hw/pci_host.c
@@ -51,14 +51,20 @@ void pci_host_config_write_common(PCIDevice *pci_dev, uint32_t addr,
                                   uint32_t limit, uint32_t val, uint32_t len)
 {
     assert(len <= 4);
-    pci_dev->config_write(pci_dev, addr, val, MIN(len, limit - addr));
+    if ((addr + len) <= limit) {
+        pci_dev->config_write(pci_dev, addr, val, len);
+    }
 }
 
 uint32_t pci_host_config_read_common(PCIDevice *pci_dev, uint32_t addr,
                                      uint32_t limit, uint32_t len)
 {
     assert(len <= 4);
-    return pci_dev->config_read(pci_dev, addr, MIN(len, limit - addr));
+    if ((addr + len) <= limit) {
+        return pci_dev->config_read(pci_dev, addr, len);
+    } else {
+        return ~0x0;
+    }
 }
 
 void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len)
-- 
1.7.7.3

[Qemu-devel] [PATCH 4/4] Update gitignore file

[David Gibson]

This patch adds several auto-generated files to .gitignore which were
previously missing.

Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
---
 .gitignore |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/.gitignore b/.gitignore
index 406f75f..f5aab2c 100644
--- a/.gitignore
+++ b/.gitignore
@@ -42,6 +42,10 @@ qemu-ga
 qemu-monitor.texi
 QMP/qmp-commands.txt
 test-coroutine
+test-qmp-input-visitor
+test-qmp-output-visitor
+fsdev/virtfs-proxy-helper.1
+fsdev/virtfs-proxy-helper.pod
 .gdbinit
 *.a
 *.aux
-- 
1.7.7.3

Re: [Qemu-devel] [PATCH 1/4] load_image_targphys() should enforce the max size

[Stefan Weil]

Am 11.01.2012 06:44, schrieb David Gibson:
> From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
>
> load_image_targphys() gets passed a max size for the file, but
> doesn't enforce it at all. Add a check and return -1 (error) if
> the file is too big, without loading it.
>
> Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> hw/loader.c | 2 ++
> 1 files changed, 2 insertions(+), 0 deletions(-)
>
> diff --git a/hw/loader.c b/hw/loader.c
> index 446b628..7ad9e22 100644
> --- a/hw/loader.c
> +++ b/hw/loader.c
> @@ -108,6 +108,8 @@ int load_image_targphys(const char *filename,
> int size;
>
> size = get_image_size(filename);
> + if (size > max_sz)
> + return -1;
> if (size > 0)
> rom_add_file_fixed(filename, addr, -1);
> return size;

Even if this file is full of block statements without braces,
we should not add more of them. See CODING_STYLE and
scripts/checkpatch.pl.

There remains an additional problem:
Using 'int' for the size of files was sufficient 10 years ago,
but it is that no longer. get_image_size() silently reduced the
return value from lseek() to an 'int' value. So even with your
patch, very large files will be loaded (partially)!

Regards,
Stefan Weil

Re: [Qemu-devel] [PATCH 3/4] pci: Make bounds checks on config space accesses actually work

[Stefan Weil]

Am 11.01.2012 06:44, schrieb David Gibson:
> The pci_host_config_{read,write}_common() functions perform PCI config
> accesses. They take a limit parameter which they appear to be supposed
> to bounds check against, however the bounds checking logic, such as it is,
> is completely broken.
>
> Currently, it takes the minimum of the supplied length and the remaining
> space in the region and passes that as the length to the underlying
> config_{read,write} function pointer. This means that accesses which
> partially overrun the region will be silently truncated - which makes
> little sense. Accesses which entirely overrun the region will *not*
> be blocked (an exploitable bug), because in that case (limit - addr) will
> be negative and so the unsigned MIN will always return len instead. Even
> if signed arithmetic was used, the config_{read,write} callback wouldn't
> know what to do with a negative len parameter.
>
> This patch handles things more sanely by simply ignoring writes which
> overrun, and returning -1 for reads, which is the usual hardware 
> convention
> for reads to unpopulated IO regions.
>
> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
> hw/pci_host.c | 10 ++++++++--
> 1 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/hw/pci_host.c b/hw/pci_host.c
> index 44c6c20..16b3ac3 100644
> --- a/hw/pci_host.c
> +++ b/hw/pci_host.c
> @@ -51,14 +51,20 @@ void pci_host_config_write_common(PCIDevice 
> *pci_dev, uint32_t addr,
> uint32_t limit, uint32_t val, uint32_t len)
> {
> assert(len <= 4);
> - pci_dev->config_write(pci_dev, addr, val, MIN(len, limit - addr));
> + if ((addr + len) <= limit) {
> + pci_dev->config_write(pci_dev, addr, val, len);
> + }
> }
>
> uint32_t pci_host_config_read_common(PCIDevice *pci_dev, uint32_t addr,
> uint32_t limit, uint32_t len)
> {
> assert(len <= 4);
> - return pci_dev->config_read(pci_dev, addr, MIN(len, limit - addr));
> + if ((addr + len) <= limit) {
> + return pci_dev->config_read(pci_dev, addr, len);
> + } else {
> + return ~0x0;
> + }
> }
>
> void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len)

Some people use QEMU to detect this kind of errors in an emulated
systems. Therefore an additional debug output would be good here.

As long as there is no QEMU standard for reporting this kind of errors,
a PCI_DPRINTF statement might be best.

Regards,
Stefan Weil

Re: [Qemu-devel] [PATCH 1/4] load_image_targphys() should enforce the max size

[David Gibson]

On Wed, Jan 11, 2012 at 07:19:00AM +0100, Stefan Weil wrote:
> Am 11.01.2012 06:44, schrieb David Gibson:
> >From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> >
> >load_image_targphys() gets passed a max size for the file, but
> >doesn't enforce it at all. Add a check and return -1 (error) if
> >the file is too big, without loading it.
> >
> >Signed-off-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>
> >Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> >---
> >hw/loader.c | 2 ++
> >1 files changed, 2 insertions(+), 0 deletions(-)
> >
> >diff --git a/hw/loader.c b/hw/loader.c
> >index 446b628..7ad9e22 100644
> >--- a/hw/loader.c
> >+++ b/hw/loader.c
> >@@ -108,6 +108,8 @@ int load_image_targphys(const char *filename,
> >int size;
> >
> >size = get_image_size(filename);
> >+ if (size > max_sz)
> >+ return -1;
> >if (size > 0)
> >rom_add_file_fixed(filename, addr, -1);
> >return size;
> 
> Even if this file is full of block statements without braces,
> we should not add more of them. See CODING_STYLE and
> scripts/checkpatch.pl.

I know the coding style, I thought the counterexample right next to it
would take precedence.  Corrected in a respin.

> There remains an additional problem:
> Using 'int' for the size of files was sufficient 10 years ago,
> but it is that no longer. get_image_size() silently reduced the
> return value from lseek() to an 'int' value. So even with your
> patch, very large files will be loaded (partially)!

Well, sure, but that's an independent bug.  I'll fix it later if I get
to it, but that kind of leads to changing the types of a whole bunch
of things.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [Qemu-devel] [PATCH 3/4] pci: Make bounds checks on config space accesses actually work

[David Gibson]

On Wed, Jan 11, 2012 at 07:25:52AM +0100, Stefan Weil wrote:
> Am 11.01.2012 06:44, schrieb David Gibson:
> >The pci_host_config_{read,write}_common() functions perform PCI config
> >accesses. They take a limit parameter which they appear to be supposed
> >to bounds check against, however the bounds checking logic, such as it is,
> >is completely broken.
> >
> >Currently, it takes the minimum of the supplied length and the remaining
> >space in the region and passes that as the length to the underlying
> >config_{read,write} function pointer. This means that accesses which
> >partially overrun the region will be silently truncated - which makes
> >little sense. Accesses which entirely overrun the region will *not*
> >be blocked (an exploitable bug), because in that case (limit - addr) will
> >be negative and so the unsigned MIN will always return len instead. Even
> >if signed arithmetic was used, the config_{read,write} callback wouldn't
> >know what to do with a negative len parameter.
> >
> >This patch handles things more sanely by simply ignoring writes which
> >overrun, and returning -1 for reads, which is the usual hardware
> >convention
> >for reads to unpopulated IO regions.
> >
> >Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> >---
> >hw/pci_host.c | 10 ++++++++--
> >1 files changed, 8 insertions(+), 2 deletions(-)
> >
> >diff --git a/hw/pci_host.c b/hw/pci_host.c
> >index 44c6c20..16b3ac3 100644
> >--- a/hw/pci_host.c
> >+++ b/hw/pci_host.c
> >@@ -51,14 +51,20 @@ void pci_host_config_write_common(PCIDevice
> >*pci_dev, uint32_t addr,
> >uint32_t limit, uint32_t val, uint32_t len)
> >{
> >assert(len <= 4);
> >- pci_dev->config_write(pci_dev, addr, val, MIN(len, limit - addr));
> >+ if ((addr + len) <= limit) {
> >+ pci_dev->config_write(pci_dev, addr, val, len);
> >+ }
> >}
> >
> >uint32_t pci_host_config_read_common(PCIDevice *pci_dev, uint32_t addr,
> >uint32_t limit, uint32_t len)
> >{
> >assert(len <= 4);
> >- return pci_dev->config_read(pci_dev, addr, MIN(len, limit - addr));
> >+ if ((addr + len) <= limit) {
> >+ return pci_dev->config_read(pci_dev, addr, len);
> >+ } else {
> >+ return ~0x0;
> >+ }
> >}
> >
> >void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len)
> 
> Some people use QEMU to detect this kind of errors in an emulated
> systems. Therefore an additional debug output would be good here.
> 
> As long as there is no QEMU standard for reporting this kind of errors,
> a PCI_DPRINTF statement might be best.

This seems like an independent enhancement to me.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: [Qemu-devel] [PATCH 3/4] pci: Make bounds checks on config space accesses actually work

[Stefan Weil]

   Am 12.01.2012 04:27, schrieb David Gibson:
> On Wed, Jan 11, 2012 at 07:25:52AM +0100, Stefan Weil wrote:
>> Am 11.01.2012 06:44, schrieb David Gibson:
>>> The pci_host_config_{read,write}_common() functions perform PCI config
>>> accesses. They take a limit parameter which they appear to be supposed
>>> to bounds check against, however the bounds checking logic, such as it is,
>>> is completely broken.
>>>
>>> Currently, it takes the minimum of the supplied length and the remaining
>>> space in the region and passes that as the length to the underlying
>>> config_{read,write} function pointer. This means that accesses which
>>> partially overrun the region will be silently truncated - which makes
>>> little sense. Accesses which entirely overrun the region will *not*
>>> be blocked (an exploitable bug), because in that case (limit - addr) will
>>> be negative and so the unsigned MIN will always return len instead. Even
>>> if signed arithmetic was used, the config_{read,write} callback wouldn't
>>> know what to do with a negative len parameter.
>>>
>>> This patch handles things more sanely by simply ignoring writes which
>>> overrun, and returning -1 for reads, which is the usual hardware
>>> convention
>>> for reads to unpopulated IO regions.
>>>
>>> Signed-off-by: David Gibson<david@gibson.dropbear.id.au>
>>> ---
>>> hw/pci_host.c | 10 ++++++++--
>>> 1 files changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/hw/pci_host.c b/hw/pci_host.c
>>> index 44c6c20..16b3ac3 100644
>>> --- a/hw/pci_host.c
>>> +++ b/hw/pci_host.c
>>> @@ -51,14 +51,20 @@ void pci_host_config_write_common(PCIDevice
>>> *pci_dev, uint32_t addr,
>>> uint32_t limit, uint32_t val, uint32_t len)
>>> {
>>> assert(len<= 4);
>>> - pci_dev->config_write(pci_dev, addr, val, MIN(len, limit - addr));
>>> + if ((addr + len)<= limit) {
>>> + pci_dev->config_write(pci_dev, addr, val, len);
>>> + }
>>> }
>>>
>>> uint32_t pci_host_config_read_common(PCIDevice *pci_dev, uint32_t addr,
>>> uint32_t limit, uint32_t len)
>>> {
>>> assert(len<= 4);
>>> - return pci_dev->config_read(pci_dev, addr, MIN(len, limit - addr));
>>> + if ((addr + len)<= limit) {
>>> + return pci_dev->config_read(pci_dev, addr, len);
>>> + } else {
>>> + return ~0x0;
>>> + }
>>> }
>>>
>>> void pci_data_write(PCIBus *s, uint32_t addr, uint32_t val, int len)
>> Some people use QEMU to detect this kind of errors in an emulated
>> systems. Therefore an additional debug output would be good here.
>>
>> As long as there is no QEMU standard for reporting this kind of errors,
>> a PCI_DPRINTF statement might be best.
> This seems like an independent enhancement to me.

I don't mind if the debug output is added in a separate patch.

Adding a TODO comment now helps to remember that there
is something to be done later.

A TODO comment would also be good for PATCH 1/4.

Another point: instead of ~0, you could also use UINT32_MAX.

Regards,
Stefan Weil