On 06/28/2016 10:39 AM, Paolo Bonzini wrote:
> s->cur_addr can be made to point outside s->storage, either by
> writing a value >= 128 to s->ear (because s->ear * MAX_3BYTES_SIZE
> is a signed integer and sign-extends into the 64-bit cur_addr),
> or just by writing an address beyond the size of the flash being
> emulated. Avoid the sign extension to make the code cleaner, and
> on top of that mask s->cur_addr to s->size.
>
> Signed-off-by: Paolo Bonzini <pbonz...@redhat.com>
Yes. We can trash memory very easily with the m25p80 object so we
clearly want this fix.
Maybe, we could add a guest error when going beyond storage and
the case doing flash_write8() needs some care also. Anyhow, that
can be covered in another patch.
Reviewed-by: Cédric Le Goater <c...@kaod.org>
Thanks,
C.
> ---
> hw/block/m25p80.c | 16 +++++++---------
> 1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index dd6714d..76a9bcf 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -586,18 +586,16 @@ static inline int get_addr_length(Flash *s)
>
> static void complete_collecting_data(Flash *s)
> {
> - int i;
> -
> - s->cur_addr = 0;
> + int i, n;
>
> - for (i = 0; i < get_addr_length(s); ++i) {
> + n = get_addr_length(s);
> + s->cur_addr = (n == 3 ? s->ear : 0);
> + for (i = 0; i < n; ++i) {
> s->cur_addr <<= 8;
> s->cur_addr |= s->data[i];
> }
>
> - if (get_addr_length(s) == 3) {
> - s->cur_addr += s->ear * MAX_3BYTES_SIZE;
> - }
> + s->cur_addr &= s->size - 1;
>
> s->state = STATE_IDLE;
>
> @@ -1099,14 +1097,14 @@ static uint32_t m25p80_transfer8(SSISlave *ss,
> uint32_t tx)
> DB_PRINT_L(1, "page program cur_addr=%#" PRIx64 " data=%" PRIx8 "\n",
> s->cur_addr, (uint8_t)tx);
> flash_write8(s, s->cur_addr, (uint8_t)tx);
> - s->cur_addr++;
> + s->cur_addr = (s->cur_addr + 1) & (s->size - 1);
> break;
>
> case STATE_READ:
> r = s->storage[s->cur_addr];
> DB_PRINT_L(1, "READ 0x%" PRIx64 "=%" PRIx8 "\n", s->cur_addr,
> (uint8_t)r);
> - s->cur_addr = (s->cur_addr + 1) % s->size;
> + s->cur_addr = (s->cur_addr + 1) & (s->size - 1);
> break;
>
> case STATE_COLLECTING_DATA:
>
