[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pi
From: |
Stefano Stabellini |
Subject: |
Re: [Qemu-devel] [Xen-devel] [PATCH 0/2] QEMU/xen: simplify cpu_ioreq_pio and cpu_ioreq_move |
Date: |
Mon, 10 Dec 2012 16:08:22 +0000 |
User-agent: |
Alpine 2.02 (DEB 1266 2009-07-14) |
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 <address@hidden>
> 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 <address@hidden>
> Signed-off-by: Stefano Stabellini <address@hidden>
> Signed-off-by: Ian Jackson <address@hidden>
>
> 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);
> }
> }
> }
>