* [Qemu-devel] [PATCH 0/2] cpu_ioreq_pio, cpu_ioreq_move: simply and fix
@ 2012-12-10 18:04 Ian Jackson
2012-12-10 18:04 ` [Qemu-devel] [PATCH 1/2] cpu_ioreq_pio, cpu_ioreq_move: introduce read_phys_req_item, write_phys_req_item Ian Jackson
2012-12-10 18:04 ` [Qemu-devel] [PATCH 2/2] cpu_ioreq_pio, cpu_ioreq_move: i should be uint32_t rather than int Ian Jackson
0 siblings, 2 replies; 4+ messages in thread
From: Ian Jackson @ 2012-12-10 18:04 UTC (permalink / raw)
To: qemu-devel, xen-devel
These two patches fix a potential infinite loop if an ioreq has a very
large count, and also simplifies the code.
^ permalink raw reply [flat|nested] 4+ messages in thread
* [Qemu-devel] [PATCH 1/2] cpu_ioreq_pio, cpu_ioreq_move: introduce read_phys_req_item, write_phys_req_item
2012-12-10 18:04 [Qemu-devel] [PATCH 0/2] cpu_ioreq_pio, cpu_ioreq_move: simply and fix Ian Jackson
@ 2012-12-10 18:04 ` Ian Jackson
2012-12-10 19:29 ` Stefano Stabellini
2012-12-10 18:04 ` [Qemu-devel] [PATCH 2/2] cpu_ioreq_pio, cpu_ioreq_move: i should be uint32_t rather than int Ian Jackson
1 sibling, 1 reply; 4+ messages in thread
From: Ian Jackson @ 2012-12-10 18:04 UTC (permalink / raw)
To: qemu-devel, xen-devel; +Cc: Dongxiao Xu, Ian Jackson, Stefano Stabellini
Replace a lot of formulaic multiplications (containing casts, no less)
with calls to a pair of functions. This encapsulates in a single
place the operations which require care relating to integer overflow.
Cc: Dongxiao Xu <dongxiao.xu@intel.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
xen-all.c | 73 ++++++++++++++++++++++++++++++++++++-------------------------
1 files changed, 43 insertions(+), 30 deletions(-)
diff --git a/xen-all.c b/xen-all.c
index 046cc2a..97c8ef4 100644
--- a/xen-all.c
+++ b/xen-all.c
@@ -682,11 +682,42 @@ static void do_outp(pio_addr_t addr,
}
}
-static void cpu_ioreq_pio(ioreq_t *req)
+/*
+ * 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(hwaddr addr,
+ ioreq_t *req, uint32_t i, void *val, int rw)
+{
+ /* 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 */
+ hwaddr offset = (hwaddr)req->size * i;
+ if (req->df) addr -= offset;
+ else addr += offset;
+ cpu_physical_memory_rw(addr, val, req->size, rw);
+}
+
+static inline void read_phys_req_item(hwaddr addr,
+ ioreq_t *req, uint32_t i, void *val)
+{
+ rw_phys_req_item(addr, req, i, val, 0);
+}
+static inline void write_phys_req_item(hwaddr addr,
+ ioreq_t *req, uint32_t i, void *val)
{
- int i, sign;
+ rw_phys_req_item(addr, req, i, val, 1);
+}
- sign = req->df ? -1 : 1;
+
+static void cpu_ioreq_pio(ioreq_t *req)
+{
+ int i;
if (req->dir == IOREQ_READ) {
if (!req->data_is_ptr) {
@@ -696,9 +727,7 @@ static void cpu_ioreq_pio(ioreq_t *req)
for (i = 0; i < req->count; i++) {
tmp = do_inp(req->addr, req->size);
- cpu_physical_memory_write(
- req->data + (sign * i * (int64_t)req->size),
- (uint8_t *) &tmp, req->size);
+ write_phys_req_item(req->data, req, i, &tmp);
}
}
} else if (req->dir == IOREQ_WRITE) {
@@ -708,9 +737,7 @@ static void cpu_ioreq_pio(ioreq_t *req)
for (i = 0; i < req->count; i++) {
uint32_t tmp = 0;
- cpu_physical_memory_read(
- req->data + (sign * i * (int64_t)req->size),
- (uint8_t*) &tmp, req->size);
+ read_phys_req_item(req->data, req, i, &tmp);
do_outp(req->addr, req->size, tmp);
}
}
@@ -719,22 +746,16 @@ static void cpu_ioreq_pio(ioreq_t *req)
static void cpu_ioreq_move(ioreq_t *req)
{
- int i, sign;
-
- sign = req->df ? -1 : 1;
+ int i;
if (!req->data_is_ptr) {
if (req->dir == IOREQ_READ) {
for (i = 0; i < req->count; i++) {
- cpu_physical_memory_read(
- req->addr + (sign * i * (int64_t)req->size),
- (uint8_t *) &req->data, req->size);
+ read_phys_req_item(req->addr, req, i, &req->data);
}
} else if (req->dir == IOREQ_WRITE) {
for (i = 0; i < req->count; i++) {
- cpu_physical_memory_write(
- req->addr + (sign * i * (int64_t)req->size),
- (uint8_t *) &req->data, req->size);
+ write_phys_req_item(req->addr, req, i, &req->data);
}
}
} else {
@@ -742,21 +763,13 @@ static void cpu_ioreq_move(ioreq_t *req)
if (req->dir == IOREQ_READ) {
for (i = 0; i < req->count; i++) {
- cpu_physical_memory_read(
- req->addr + (sign * i * (int64_t)req->size),
- (uint8_t*) &tmp, req->size);
- cpu_physical_memory_write(
- req->data + (sign * i * (int64_t)req->size),
- (uint8_t*) &tmp, req->size);
+ 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++) {
- cpu_physical_memory_read(
- req->data + (sign * i * (int64_t)req->size),
- (uint8_t*) &tmp, req->size);
- cpu_physical_memory_write(
- req->addr + (sign * i * (int64_t)req->size),
- (uint8_t*) &tmp, req->size);
+ read_phys_req_item(req->data, req, i, &tmp);
+ write_phys_req_item(req->addr, req, i, &tmp);
}
}
}
--
1.7.2.5
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [Qemu-devel] [PATCH 2/2] cpu_ioreq_pio, cpu_ioreq_move: i should be uint32_t rather than int
2012-12-10 18:04 [Qemu-devel] [PATCH 0/2] cpu_ioreq_pio, cpu_ioreq_move: simply and fix Ian Jackson
2012-12-10 18:04 ` [Qemu-devel] [PATCH 1/2] cpu_ioreq_pio, cpu_ioreq_move: introduce read_phys_req_item, write_phys_req_item Ian Jackson
@ 2012-12-10 18:04 ` Ian Jackson
1 sibling, 0 replies; 4+ messages in thread
From: Ian Jackson @ 2012-12-10 18:04 UTC (permalink / raw)
To: qemu-devel, xen-devel; +Cc: Dongxiao Xu, Ian Jackson, Stefano Stabellini
The current code compare i (int) with req->count (uint32_t) in a for
loop, risking an infinite loop if req->count is equal to UINT_MAX.
Also i is only used in comparisons or multiplications with unsigned
integers.
Signed-off-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Dongxiao Xu <dongxiao.xu@intel.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
xen-all.c | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/xen-all.c b/xen-all.c
index 97c8ef4..aabbb80 100644
--- a/xen-all.c
+++ b/xen-all.c
@@ -717,7 +717,7 @@ static inline void write_phys_req_item(hwaddr addr,
static void cpu_ioreq_pio(ioreq_t *req)
{
- int i;
+ uint32_t i;
if (req->dir == IOREQ_READ) {
if (!req->data_is_ptr) {
@@ -746,7 +746,7 @@ static void cpu_ioreq_pio(ioreq_t *req)
static void cpu_ioreq_move(ioreq_t *req)
{
- int i;
+ uint32_t i;
if (!req->data_is_ptr) {
if (req->dir == IOREQ_READ) {
--
1.7.2.5
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [Qemu-devel] [PATCH 1/2] cpu_ioreq_pio, cpu_ioreq_move: introduce read_phys_req_item, write_phys_req_item
2012-12-10 18:04 ` [Qemu-devel] [PATCH 1/2] cpu_ioreq_pio, cpu_ioreq_move: introduce read_phys_req_item, write_phys_req_item Ian Jackson
@ 2012-12-10 19:29 ` Stefano Stabellini
0 siblings, 0 replies; 4+ messages in thread
From: Stefano Stabellini @ 2012-12-10 19:29 UTC (permalink / raw)
To: Ian Jackson
Cc: Dongxiao Xu, xen-devel@lists.xensource.com, qemu-devel@nongnu.org,
Stefano Stabellini
On Mon, 10 Dec 2012, Ian Jackson wrote:
> Replace a lot of formulaic multiplications (containing casts, no less)
> with calls to a pair of functions. This encapsulates in a single
> place the operations which require care relating to integer overflow.
>
> Cc: Dongxiao Xu <dongxiao.xu@intel.com>
> Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
> ---
> xen-all.c | 73 ++++++++++++++++++++++++++++++++++++-------------------------
> 1 files changed, 43 insertions(+), 30 deletions(-)
>
> diff --git a/xen-all.c b/xen-all.c
> index 046cc2a..97c8ef4 100644
> --- a/xen-all.c
> +++ b/xen-all.c
> @@ -682,11 +682,42 @@ static void do_outp(pio_addr_t addr,
> }
> }
>
> -static void cpu_ioreq_pio(ioreq_t *req)
> +/*
> + * 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(hwaddr addr,
> + ioreq_t *req, uint32_t i, void *val, int rw)
> +{
> + /* 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 */
> + hwaddr offset = (hwaddr)req->size * i;
> + if (req->df) addr -= offset;
> + else addr += offset;
> + cpu_physical_memory_rw(addr, val, req->size, rw);
> +}
QEMU's code style is
if (something) {
you can also run the patch through scripts/checkpatch.pl.
Aside from the code style issue:
Acked-by: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
> +static inline void read_phys_req_item(hwaddr addr,
> + ioreq_t *req, uint32_t i, void *val)
> +{
> + rw_phys_req_item(addr, req, i, val, 0);
> +}
> +static inline void write_phys_req_item(hwaddr addr,
> + ioreq_t *req, uint32_t i, void *val)
> {
> - int i, sign;
> + rw_phys_req_item(addr, req, i, val, 1);
> +}
>
> - sign = req->df ? -1 : 1;
> +
> +static void cpu_ioreq_pio(ioreq_t *req)
> +{
> + int i;
>
> if (req->dir == IOREQ_READ) {
> if (!req->data_is_ptr) {
> @@ -696,9 +727,7 @@ static void cpu_ioreq_pio(ioreq_t *req)
>
> for (i = 0; i < req->count; i++) {
> tmp = do_inp(req->addr, req->size);
> - cpu_physical_memory_write(
> - req->data + (sign * i * (int64_t)req->size),
> - (uint8_t *) &tmp, req->size);
> + write_phys_req_item(req->data, req, i, &tmp);
> }
> }
> } else if (req->dir == IOREQ_WRITE) {
> @@ -708,9 +737,7 @@ static void cpu_ioreq_pio(ioreq_t *req)
> for (i = 0; i < req->count; i++) {
> uint32_t tmp = 0;
>
> - cpu_physical_memory_read(
> - req->data + (sign * i * (int64_t)req->size),
> - (uint8_t*) &tmp, req->size);
> + read_phys_req_item(req->data, req, i, &tmp);
> do_outp(req->addr, req->size, tmp);
> }
> }
> @@ -719,22 +746,16 @@ static void cpu_ioreq_pio(ioreq_t *req)
>
> static void cpu_ioreq_move(ioreq_t *req)
> {
> - int i, sign;
> -
> - sign = req->df ? -1 : 1;
> + int i;
>
> if (!req->data_is_ptr) {
> if (req->dir == IOREQ_READ) {
> for (i = 0; i < req->count; i++) {
> - cpu_physical_memory_read(
> - req->addr + (sign * i * (int64_t)req->size),
> - (uint8_t *) &req->data, req->size);
> + read_phys_req_item(req->addr, req, i, &req->data);
> }
> } else if (req->dir == IOREQ_WRITE) {
> for (i = 0; i < req->count; i++) {
> - cpu_physical_memory_write(
> - req->addr + (sign * i * (int64_t)req->size),
> - (uint8_t *) &req->data, req->size);
> + write_phys_req_item(req->addr, req, i, &req->data);
> }
> }
> } else {
> @@ -742,21 +763,13 @@ static void cpu_ioreq_move(ioreq_t *req)
>
> if (req->dir == IOREQ_READ) {
> for (i = 0; i < req->count; i++) {
> - cpu_physical_memory_read(
> - req->addr + (sign * i * (int64_t)req->size),
> - (uint8_t*) &tmp, req->size);
> - cpu_physical_memory_write(
> - req->data + (sign * i * (int64_t)req->size),
> - (uint8_t*) &tmp, req->size);
> + 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++) {
> - cpu_physical_memory_read(
> - req->data + (sign * i * (int64_t)req->size),
> - (uint8_t*) &tmp, req->size);
> - cpu_physical_memory_write(
> - req->addr + (sign * i * (int64_t)req->size),
> - (uint8_t*) &tmp, req->size);
> + read_phys_req_item(req->data, req, i, &tmp);
> + write_phys_req_item(req->addr, req, i, &tmp);
> }
> }
> }
> --
> 1.7.2.5
>
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-12-10 19:30 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-12-10 18:04 [Qemu-devel] [PATCH 0/2] cpu_ioreq_pio, cpu_ioreq_move: simply and fix Ian Jackson
2012-12-10 18:04 ` [Qemu-devel] [PATCH 1/2] cpu_ioreq_pio, cpu_ioreq_move: introduce read_phys_req_item, write_phys_req_item Ian Jackson
2012-12-10 19:29 ` Stefano Stabellini
2012-12-10 18:04 ` [Qemu-devel] [PATCH 2/2] cpu_ioreq_pio, cpu_ioreq_move: i should be uint32_t rather than int Ian Jackson
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).