Patchwork [Xen-devel,0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move

login
register
mail settings
Submitter Ian Jackson
Date Dec. 7, 2012, 4:14 p.m.
Message ID <20674.5586.142286.869968@mariner.uk.xensource.com>
Download mbox | patch
Permalink /patch/204569/
State New
Headers show

Comments

Ian Jackson - Dec. 7, 2012, 4:14 p.m.
Stefano Stabellini writes ("[Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move"):
> after reviewing the patch "fix multiply issue for int and uint types"
> with Ian Jackson, we realized that cpu_ioreq_pio and cpu_ioreq_move are
> in much need for a simplification as well as removal of a possible
> integer overflow.
> 
> This patch series tries to accomplish both switching to two new helper
> functions and using a more obvious arithmetic. Doing so it should also
> fix the original problem that Dongxiao was experiencing. The C language
> can be a nasty backstabber when signed and unsigned integers are
> involved.

I think the attached patch is better as it removes some formulaic
code.  I don't think I have a guest which can repro the bug so I have
only compile tested it.

Dongxiao, would you care to take a look ?

PS: I'm pretty sure the original overflows aren't security problems.

Thanks,
Ian.

commit d19731e4e452e3415a5c03771d0406efc803baa9
Author: Ian Jackson <ian.jackson@eu.citrix.com>
Date:   Fri Dec 7 16:02:04 2012 +0000

    cpu_ioreq_pio, cpu_ioreq_move: introduce read_phys_req_item, write_phys_req_item
    
    The current code compare i (int) with req->count (uint32_t) in a for
    loop, risking an infinite loop if req->count is >INT_MAX.  It also
    does the multiplication of req->size in a too-small type, leading to
    integer overflows.
    
    Turn read_physical and write_physical into two different helper
    functions, read_phys_req_item and write_phys_req_item, that take care
    of adding or subtracting offset depending on sign.
    
    This removes the formulaic multiplication to a single place where the
    integer overflows can be dealt with by casting to wide-enough unsigned
    types.
    
    Reported-By: Dongxiao Xu <dongxiao.xu@intel.com>
    Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
    Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
Ian Campbell - Dec. 7, 2012, 4:30 p.m.
On Fri, 2012-12-07 at 16:14 +0000, Ian Jackson wrote:
> +    target_phys_addr_t offset = (target_phys_addr_t)req->size * i;
> +    if (req->df) addr -= offset;
> +    else addr -= offset;

One of these -= should be a += I presume?

[...]
> +                write_phys_req_item((target_phys_addr_t) req->data, req, i, &tmp);

This seems to be the only one with this cast, why?

write_phys_req_item takes a target_phys_addr_t so this will happen
regardless I think.

Ian.
Stefano Stabellini - Dec. 10, 2012, 4:08 p.m.
On Fri, 7 Dec 2012, Ian Jackson wrote:
> Stefano Stabellini writes ("[Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move"):
> > after reviewing the patch "fix multiply issue for int and uint types"
> > with Ian Jackson, we realized that cpu_ioreq_pio and cpu_ioreq_move are
> > in much need for a simplification as well as removal of a possible
> > integer overflow.
> > 
> > This patch series tries to accomplish both switching to two new helper
> > functions and using a more obvious arithmetic. Doing so it should also
> > fix the original problem that Dongxiao was experiencing. The C language
> > can be a nasty backstabber when signed and unsigned integers are
> > involved.
> 
> I think the attached patch is better as it removes some formulaic
> code.  I don't think I have a guest which can repro the bug so I have
> only compile tested it.
> 
> Dongxiao, would you care to take a look ?
> 
> PS: I'm pretty sure the original overflows aren't security problems.
> 
> Thanks,
> Ian.
> 
> commit d19731e4e452e3415a5c03771d0406efc803baa9
> Author: Ian Jackson <ian.jackson@eu.citrix.com>
> Date:   Fri Dec 7 16:02:04 2012 +0000
> 
>     cpu_ioreq_pio, cpu_ioreq_move: introduce read_phys_req_item, write_phys_req_item
>     
>     The current code compare i (int) with req->count (uint32_t) in a for
>     loop, risking an infinite loop if req->count is >INT_MAX.  It also
>     does the multiplication of req->size in a too-small type, leading to
>     integer overflows.
>     
>     Turn read_physical and write_physical into two different helper
>     functions, read_phys_req_item and write_phys_req_item, that take care
>     of adding or subtracting offset depending on sign.
>     
>     This removes the formulaic multiplication to a single place where the
>     integer overflows can be dealt with by casting to wide-enough unsigned
>     types.
>     
>     Reported-By: Dongxiao Xu <dongxiao.xu@intel.com>
>     Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
>     Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> 
> diff --git a/i386-dm/helper2.c b/i386-dm/helper2.c
> index c6d049c..9b8552c 100644
> --- a/i386-dm/helper2.c
> +++ b/i386-dm/helper2.c
> @@ -339,21 +339,40 @@ static void do_outp(CPUState *env, unsigned long addr,
>      }
>  }
>  
> -static inline void read_physical(uint64_t addr, unsigned long size, void *val)
> +/*
> + * Helper functions which read/write an object from/to physical guest
> + * memory, as part of the implementation of an ioreq.
> + *
> + * Equivalent to
> + *   cpu_physical_memory_rw(addr + (req->df ? -1 : +1) * req->size * i,
> + *                          val, req->size, 0/1)
> + * except without the integer overflow problems.
> + */
> +static void rw_phys_req_item(target_phys_addr_t addr,
> +                             ioreq_t *req, uint32_t i, void *val, int rw)
>  {
> -    return cpu_physical_memory_rw((target_phys_addr_t)addr, val, size, 0);
> +    /* Do everything unsigned so overflow just results in a truncated result
> +     * and accesses to undesired parts of guest memory, which is up
> +     * to the guest */
> +    target_phys_addr_t offset = (target_phys_addr_t)req->size * i;
> +    if (req->df) addr -= offset;
> +    else addr -= offset;

This can't be right, can it?

The search/replace changes below look correct.

For the sake of consistency, could you please send a patch against
upstream QEMU to qemu-devel? The corresponding code is in xen-all.c
(cpu_ioreq_pio and cpu_ioreq_move).



> +    cpu_physical_memory_rw(addr, val, req->size, rw);
>  }
> -
> -static inline void write_physical(uint64_t addr, unsigned long size, void *val)
> +static inline void read_phys_req_item(target_phys_addr_t addr,
> +                                      ioreq_t *req, uint32_t i, void *val)
>  {
> -    return cpu_physical_memory_rw((target_phys_addr_t)addr, val, size, 1);
> +    rw_phys_req_item(addr, req, i, val, 0);
> +}
> +static inline void write_phys_req_item(target_phys_addr_t addr,
> +                                       ioreq_t *req, uint32_t i, void *val)
> +{
> +    rw_phys_req_item(addr, req, i, val, 1);
>  }
>  
>  static void cpu_ioreq_pio(CPUState *env, ioreq_t *req)
>  {
> -    int i, sign;
> -
> -    sign = req->df ? -1 : 1;
> +    uint32_t i;
>  
>      if (req->dir == IOREQ_READ) {
>          if (!req->data_is_ptr) {
> @@ -363,9 +382,7 @@ static void cpu_ioreq_pio(CPUState *env, ioreq_t *req)
>  
>              for (i = 0; i < req->count; i++) {
>                  tmp = do_inp(env, req->addr, req->size);
> -                write_physical((target_phys_addr_t) req->data
> -                  + (sign * i * req->size),
> -                  req->size, &tmp);
> +                write_phys_req_item((target_phys_addr_t) req->data, req, i, &tmp);
>              }
>          }
>      } else if (req->dir == IOREQ_WRITE) {
> @@ -375,9 +392,7 @@ static void cpu_ioreq_pio(CPUState *env, ioreq_t *req)
>              for (i = 0; i < req->count; i++) {
>                  unsigned long tmp = 0;
>  
> -                read_physical((target_phys_addr_t) req->data
> -                  + (sign * i * req->size),
> -                  req->size, &tmp);
> +                read_phys_req_item(req->data, req, i, &tmp);
>                  do_outp(env, req->addr, req->size, tmp);
>              }
>          }
> @@ -386,22 +401,16 @@ static void cpu_ioreq_pio(CPUState *env, ioreq_t *req)
>  
>  static void cpu_ioreq_move(CPUState *env, ioreq_t *req)
>  {
> -    int i, sign;
> -
> -    sign = req->df ? -1 : 1;
> +    uint32_t i;
>  
>      if (!req->data_is_ptr) {
>          if (req->dir == IOREQ_READ) {
>              for (i = 0; i < req->count; i++) {
> -                read_physical(req->addr
> -                  + (sign * i * req->size),
> -                  req->size, &req->data);
> +                read_phys_req_item(req->addr, req, i, &req->data);
>              }
>          } else if (req->dir == IOREQ_WRITE) {
>              for (i = 0; i < req->count; i++) {
> -                write_physical(req->addr
> -                  + (sign * i * req->size),
> -                  req->size, &req->data);
> +                write_phys_req_item(req->addr, req, i, &req->data);
>              }
>          }
>      } else {
> @@ -409,21 +418,13 @@ static void cpu_ioreq_move(CPUState *env, ioreq_t *req)
>  
>          if (req->dir == IOREQ_READ) {
>              for (i = 0; i < req->count; i++) {
> -                read_physical(req->addr
> -                  + (sign * i * req->size),
> -                  req->size, &tmp);
> -                write_physical((target_phys_addr_t )req->data
> -                  + (sign * i * req->size),
> -                  req->size, &tmp);
> +                read_phys_req_item(req->addr, req, i, &tmp);
> +                write_phys_req_item(req->data, req, i, &tmp);
>              }
>          } else if (req->dir == IOREQ_WRITE) {
>              for (i = 0; i < req->count; i++) {
> -                read_physical((target_phys_addr_t) req->data
> -                  + (sign * i * req->size),
> -                  req->size, &tmp);
> -                write_physical(req->addr
> -                  + (sign * i * req->size),
> -                  req->size, &tmp);
> +                read_phys_req_item(req->data, req, i, &tmp);
> +                write_phys_req_item(req->addr, req, i, &tmp);
>              }
>          }
>      }
>
Ian Jackson - Dec. 10, 2012, 5:01 p.m.
Stefano Stabellini writes ("Re: [Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move"):
> On Fri, 7 Dec 2012, Ian Jackson wrote:
...
> > +    if (req->df) addr -= offset;
> > +    else addr -= offset;
> 
> This can't be right, can it?

Indeed not.  v2 has this fixed.

> The search/replace changes below look correct.

Thanks.

> For the sake of consistency, could you please send a patch against
> upstream QEMU to qemu-devel? The corresponding code is in xen-all.c
> (cpu_ioreq_pio and cpu_ioreq_move).

I will do that.

Thanks,
Ian.

Patch

diff --git a/i386-dm/helper2.c b/i386-dm/helper2.c
index c6d049c..9b8552c 100644
--- a/i386-dm/helper2.c
+++ b/i386-dm/helper2.c
@@ -339,21 +339,40 @@  static void do_outp(CPUState *env, unsigned long addr,
     }
 }
 
-static inline void read_physical(uint64_t addr, unsigned long size, void *val)
+/*
+ * Helper functions which read/write an object from/to physical guest
+ * memory, as part of the implementation of an ioreq.
+ *
+ * Equivalent to
+ *   cpu_physical_memory_rw(addr + (req->df ? -1 : +1) * req->size * i,
+ *                          val, req->size, 0/1)
+ * except without the integer overflow problems.
+ */
+static void rw_phys_req_item(target_phys_addr_t addr,
+                             ioreq_t *req, uint32_t i, void *val, int rw)
 {
-    return cpu_physical_memory_rw((target_phys_addr_t)addr, val, size, 0);
+    /* Do everything unsigned so overflow just results in a truncated result
+     * and accesses to undesired parts of guest memory, which is up
+     * to the guest */
+    target_phys_addr_t offset = (target_phys_addr_t)req->size * i;
+    if (req->df) addr -= offset;
+    else addr -= offset;
+    cpu_physical_memory_rw(addr, val, req->size, rw);
 }
-
-static inline void write_physical(uint64_t addr, unsigned long size, void *val)
+static inline void read_phys_req_item(target_phys_addr_t addr,
+                                      ioreq_t *req, uint32_t i, void *val)
 {
-    return cpu_physical_memory_rw((target_phys_addr_t)addr, val, size, 1);
+    rw_phys_req_item(addr, req, i, val, 0);
+}
+static inline void write_phys_req_item(target_phys_addr_t addr,
+                                       ioreq_t *req, uint32_t i, void *val)
+{
+    rw_phys_req_item(addr, req, i, val, 1);
 }
 
 static void cpu_ioreq_pio(CPUState *env, ioreq_t *req)
 {
-    int i, sign;
-
-    sign = req->df ? -1 : 1;
+    uint32_t i;
 
     if (req->dir == IOREQ_READ) {
         if (!req->data_is_ptr) {
@@ -363,9 +382,7 @@  static void cpu_ioreq_pio(CPUState *env, ioreq_t *req)
 
             for (i = 0; i < req->count; i++) {
                 tmp = do_inp(env, req->addr, req->size);
-                write_physical((target_phys_addr_t) req->data
-                  + (sign * i * req->size),
-                  req->size, &tmp);
+                write_phys_req_item((target_phys_addr_t) req->data, req, i, &tmp);
             }
         }
     } else if (req->dir == IOREQ_WRITE) {
@@ -375,9 +392,7 @@  static void cpu_ioreq_pio(CPUState *env, ioreq_t *req)
             for (i = 0; i < req->count; i++) {
                 unsigned long tmp = 0;
 
-                read_physical((target_phys_addr_t) req->data
-                  + (sign * i * req->size),
-                  req->size, &tmp);
+                read_phys_req_item(req->data, req, i, &tmp);
                 do_outp(env, req->addr, req->size, tmp);
             }
         }
@@ -386,22 +401,16 @@  static void cpu_ioreq_pio(CPUState *env, ioreq_t *req)
 
 static void cpu_ioreq_move(CPUState *env, ioreq_t *req)
 {
-    int i, sign;
-
-    sign = req->df ? -1 : 1;
+    uint32_t i;
 
     if (!req->data_is_ptr) {
         if (req->dir == IOREQ_READ) {
             for (i = 0; i < req->count; i++) {
-                read_physical(req->addr
-                  + (sign * i * req->size),
-                  req->size, &req->data);
+                read_phys_req_item(req->addr, req, i, &req->data);
             }
         } else if (req->dir == IOREQ_WRITE) {
             for (i = 0; i < req->count; i++) {
-                write_physical(req->addr
-                  + (sign * i * req->size),
-                  req->size, &req->data);
+                write_phys_req_item(req->addr, req, i, &req->data);
             }
         }
     } else {
@@ -409,21 +418,13 @@  static void cpu_ioreq_move(CPUState *env, ioreq_t *req)
 
         if (req->dir == IOREQ_READ) {
             for (i = 0; i < req->count; i++) {
-                read_physical(req->addr
-                  + (sign * i * req->size),
-                  req->size, &tmp);
-                write_physical((target_phys_addr_t )req->data
-                  + (sign * i * req->size),
-                  req->size, &tmp);
+                read_phys_req_item(req->addr, req, i, &tmp);
+                write_phys_req_item(req->data, req, i, &tmp);
             }
         } else if (req->dir == IOREQ_WRITE) {
             for (i = 0; i < req->count; i++) {
-                read_physical((target_phys_addr_t) req->data
-                  + (sign * i * req->size),
-                  req->size, &tmp);
-                write_physical(req->addr
-                  + (sign * i * req->size),
-                  req->size, &tmp);
+                read_phys_req_item(req->data, req, i, &tmp);
+                write_phys_req_item(req->addr, req, i, &tmp);
             }
         }
     }