[uq/master] kvmvapic: Prevent reading beyond the end of guest RAM

Submitted by Jan Kiszka on Sept. 30, 2013, 10:35 a.m.

Details

Message ID 524953E1.6000105@siemens.com
State New
Headers show

Commit Message

Jan Kiszka Sept. 30, 2013, 10:35 a.m.
rom_state_paddr is guest provided (caller address of outw(VAPIC_PORT) +
writen 16-bit value) and can be influenced to point beyond the end of
the host memory backing the guest's RAM. Make sure we do not use this
pointer to actually read beyond the limits.

Reading arbitrary guest bytes is harmless, the guest kernel has to
manage access to this I/O port anyway.

Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
---
 hw/i386/kvmvapic.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Michael S. Tsirkin Sept. 30, 2013, 10:51 a.m.
On Mon, Sep 30, 2013 at 12:35:13PM +0200, Jan Kiszka wrote:
> rom_state_paddr is guest provided (caller address of outw(VAPIC_PORT) +
> writen 16-bit value) and can be influenced to point beyond the end of
> the host memory backing the guest's RAM. Make sure we do not use this
> pointer to actually read beyond the limits.
> 
> Reading arbitrary guest bytes is harmless, the guest kernel has to
> manage access to this I/O port anyway.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>

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

> ---
>  hw/i386/kvmvapic.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
> index 1c2dbf5..2d87600 100644
> --- a/hw/i386/kvmvapic.c
> +++ b/hw/i386/kvmvapic.c
> @@ -596,6 +596,9 @@ static int vapic_map_rom_writable(VAPICROMState *s)
>      section = memory_region_find(as, 0, 1);
>  
>      /* read ROM size from RAM region */
> +    if (rom_paddr + 2 >= memory_region_size(section.mr)) {
> +        return -1;
> +    }
>      ram = memory_region_get_ram_ptr(section.mr);
>      rom_size = ram[rom_paddr + 2] * ROM_BLOCK_SIZE;
>      if (rom_size == 0) {
> -- 
> 1.8.1.1.298.ge7eed54
Gleb Natapov Oct. 4, 2013, 10:18 a.m.
On Mon, Sep 30, 2013 at 12:35:13PM +0200, Jan Kiszka wrote:
> rom_state_paddr is guest provided (caller address of outw(VAPIC_PORT) +
> writen 16-bit value) and can be influenced to point beyond the end of
> the host memory backing the guest's RAM. Make sure we do not use this
> pointer to actually read beyond the limits.
> 
> Reading arbitrary guest bytes is harmless, the guest kernel has to
> manage access to this I/O port anyway.
> 
> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com>
Applied, thanks.

> ---
>  hw/i386/kvmvapic.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
> index 1c2dbf5..2d87600 100644
> --- a/hw/i386/kvmvapic.c
> +++ b/hw/i386/kvmvapic.c
> @@ -596,6 +596,9 @@ static int vapic_map_rom_writable(VAPICROMState *s)
>      section = memory_region_find(as, 0, 1);
>  
>      /* read ROM size from RAM region */
> +    if (rom_paddr + 2 >= memory_region_size(section.mr)) {
> +        return -1;
> +    }
>      ram = memory_region_get_ram_ptr(section.mr);
>      rom_size = ram[rom_paddr + 2] * ROM_BLOCK_SIZE;
>      if (rom_size == 0) {
> -- 
> 1.8.1.1.298.ge7eed54

--
			Gleb.

Patch hide | download patch | download mbox

diff --git a/hw/i386/kvmvapic.c b/hw/i386/kvmvapic.c
index 1c2dbf5..2d87600 100644
--- a/hw/i386/kvmvapic.c
+++ b/hw/i386/kvmvapic.c
@@ -596,6 +596,9 @@  static int vapic_map_rom_writable(VAPICROMState *s)
     section = memory_region_find(as, 0, 1);
 
     /* read ROM size from RAM region */
+    if (rom_paddr + 2 >= memory_region_size(section.mr)) {
+        return -1;
+    }
     ram = memory_region_get_ram_ptr(section.mr);
     rom_size = ram[rom_paddr + 2] * ROM_BLOCK_SIZE;
     if (rom_size == 0) {