* [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
* 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
* [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
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).