|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move
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@xxxxxxxxxxxxx>
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@xxxxxxxxx>
Signed-off-by: Stefano Stabellini <stefano.stabellini@xxxxxxxxxxxxx>
Signed-off-by: Ian Jackson <ian.jackson@xxxxxxxxxxxxx>
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);
}
}
}
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |