[Qemu-devel] [PATCH 0/3] m25p80: various fixes

[Qemu-devel] [PATCH 0/3] m25p80: various fixes

[Paolo Bonzini]

The first causes a failure in the new qtest.  The second fixes a possible
out-of-bounds access, and the third fixes again half of the same
out-of-bounds access.

Paolo Bonzini (3):
  m25p80: do not put iovec on the stack
  m25p80: avoid out of bounds accesses
  m25p80: change cur_addr to 32 bit integer

 hw/block/m25p80.c | 54 +++++++++++++++++++++++++++++-------------------------
 1 file changed, 29 insertions(+), 25 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/3] m25p80: do not put iovec on the stack

[Paolo Bonzini]

When doing a read-modify-write cycle, QEMU uses the iovec after returning
from blk_aio_pwritev.  m25p80 puts the iovec on the stack of blk_aio_pwritev's
caller, which causes trouble in this case.  This has been a problem
since commit 243e6f6 ("m25p80: Switch to byte-based block access",
2016-05-12) started doing writes at a smaller granularity than 512 bytes.
In principle however it could have broken before when using -drive
if=mtd,cache=none on a disk with 4K native sectors.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/block/m25p80.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 09b4767..dd6714d 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -446,6 +446,11 @@ static inline Manufacturer get_man(Flash *s)
 
 static void blk_sync_complete(void *opaque, int ret)
 {
+    QEMUIOVector *iov = opaque;
+
+    qemu_iovec_destroy(iov);
+    g_free(iov);
+
     /* do nothing. Masters do not directly interact with the backing store,
      * only the working copy so no mutexing required.
      */
@@ -453,31 +458,31 @@ static void blk_sync_complete(void *opaque, int ret)
 
 static void flash_sync_page(Flash *s, int page)
 {
-    QEMUIOVector iov;
+    QEMUIOVector *iov = g_new(QEMUIOVector, 1);
 
     if (!s->blk || blk_is_read_only(s->blk)) {
         return;
     }
 
-    qemu_iovec_init(&iov, 1);
-    qemu_iovec_add(&iov, s->storage + page * s->pi->page_size,
+    qemu_iovec_init(iov, 1);
+    qemu_iovec_add(iov, s->storage + page * s->pi->page_size,
                    s->pi->page_size);
-    blk_aio_pwritev(s->blk, page * s->pi->page_size, &iov, 0,
-                    blk_sync_complete, NULL);
+    blk_aio_pwritev(s->blk, page * s->pi->page_size, iov, 0,
+                    blk_sync_complete, iov);
 }
 
 static inline void flash_sync_area(Flash *s, int64_t off, int64_t len)
 {
-    QEMUIOVector iov;
+    QEMUIOVector *iov = g_new(QEMUIOVector, 1);
 
     if (!s->blk || blk_is_read_only(s->blk)) {
         return;
     }
 
     assert(!(len % BDRV_SECTOR_SIZE));
-    qemu_iovec_init(&iov, 1);
-    qemu_iovec_add(&iov, s->storage + off, len);
-    blk_aio_pwritev(s->blk, off, &iov, 0, blk_sync_complete, NULL);
+    qemu_iovec_init(iov, 1);
+    qemu_iovec_add(iov, s->storage + off, len);
+    blk_aio_pwritev(s->blk, off, iov, 0, blk_sync_complete, iov);
 }
 
 static void flash_erase(Flash *s, int offset, FlashCMD cmd)
-- 
2.7.4

[Qemu-devel] [PATCH 2/3] m25p80: avoid out of bounds accesses

[Paolo Bonzini]

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 <pbonzini@redhat.com>
---
 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:
-- 
2.7.4

[Qemu-devel] [PATCH 3/3] m25p80: change cur_addr to 32 bit integer

[Paolo Bonzini]

The maximum amount of storage that can be addressed by the m25p80 command
set is 4 GiB.  However, cur_addr is currently a 64-bit integer.  To avoid
further problems related to sign extension of signed 32-bit integer
expressions, change cur_addr to a 32 bit integer.  Preserve migration
format by adding a dummy 4-byte field in place of the (big-endian)
high four bytes in the formerly 64-bit cur_addr field.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 hw/block/m25p80.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
index 76a9bcf..7668b22 100644
--- a/hw/block/m25p80.c
+++ b/hw/block/m25p80.c
@@ -389,7 +389,7 @@ typedef struct Flash {
     uint32_t pos;
     uint8_t needed_bytes;
     uint8_t cmd_in_progress;
-    uint64_t cur_addr;
+    uint32_t cur_addr;
     uint32_t nonvolatile_cfg;
     /* Configuration register for Macronix */
     uint32_t volatile_cfg;
@@ -535,9 +535,9 @@ static inline void flash_sync_dirty(Flash *s, int64_t newpage)
 }
 
 static inline
-void flash_write8(Flash *s, uint64_t addr, uint8_t data)
+void flash_write8(Flash *s, uint32_t addr, uint8_t data)
 {
-    int64_t page = addr / s->pi->page_size;
+    uint32_t page = addr / s->pi->page_size;
     uint8_t prev = s->storage[s->cur_addr];
 
     if (!s->write_enable) {
@@ -545,7 +545,7 @@ void flash_write8(Flash *s, uint64_t addr, uint8_t data)
     }
 
     if ((prev ^ data) & data) {
-        DB_PRINT_L(1, "programming zero to one! addr=%" PRIx64 "  %" PRIx8
+        DB_PRINT_L(1, "programming zero to one! addr=%" PRIx32 "  %" PRIx8
                    " -> %" PRIx8 "\n", addr, prev, data);
     }
 
@@ -1094,7 +1094,7 @@ static uint32_t m25p80_transfer8(SSISlave *ss, uint32_t tx)
     switch (s->state) {
 
     case STATE_PAGE_PROGRAM:
-        DB_PRINT_L(1, "page program cur_addr=%#" PRIx64 " data=%" PRIx8 "\n",
+        DB_PRINT_L(1, "page program cur_addr=%#" PRIx32 " data=%" PRIx8 "\n",
                    s->cur_addr, (uint8_t)tx);
         flash_write8(s, s->cur_addr, (uint8_t)tx);
         s->cur_addr = (s->cur_addr + 1) & (s->size - 1);
@@ -1102,7 +1102,7 @@ static uint32_t m25p80_transfer8(SSISlave *ss, uint32_t tx)
 
     case STATE_READ:
         r = s->storage[s->cur_addr];
-        DB_PRINT_L(1, "READ 0x%" PRIx64 "=%" PRIx8 "\n", s->cur_addr,
+        DB_PRINT_L(1, "READ 0x%" PRIx32 "=%" PRIx8 "\n", s->cur_addr,
                    (uint8_t)r);
         s->cur_addr = (s->cur_addr + 1) & (s->size - 1);
         break;
@@ -1199,7 +1199,8 @@ static const VMStateDescription vmstate_m25p80 = {
         VMSTATE_UINT32(pos, Flash),
         VMSTATE_UINT8(needed_bytes, Flash),
         VMSTATE_UINT8(cmd_in_progress, Flash),
-        VMSTATE_UINT64(cur_addr, Flash),
+        VMSTATE_UNUSED(4),
+        VMSTATE_UINT32(cur_addr, Flash),
         VMSTATE_BOOL(write_enable, Flash),
         VMSTATE_BOOL_V(reset_enable, Flash, 2),
         VMSTATE_UINT8_V(ear, Flash, 2),
-- 
2.7.4

Re: [Qemu-devel] [PATCH 1/3] m25p80: do not put iovec on the stack

[Cédric Le Goater]

On 06/28/2016 10:39 AM, Paolo Bonzini wrote:
> When doing a read-modify-write cycle, QEMU uses the iovec after returning
> from blk_aio_pwritev.  m25p80 puts the iovec on the stack of blk_aio_pwritev's
> caller, which causes trouble in this case.  This has been a problem
> since commit 243e6f6 ("m25p80: Switch to byte-based block access",
> 2016-05-12) started doing writes at a smaller granularity than 512 bytes.
> In principle however it could have broken before when using -drive
> if=mtd,cache=none on a disk with 4K native sectors.

Ah ! Thanks. That was a problem I was seeing.   


I was thinking we could just do synchronous writes :

	https://github.com/legoater/qemu/commit/aef3fe4db3be632077c581541fe30b4e36b5a6f7

and enable snapshotting like  : 

	https://github.com/legoater/qemu/commit/b90ccab7873fd3538b47396ec7c3ae35c8e13270


Thanks, 

C. 


> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/block/m25p80.c | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 09b4767..dd6714d 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -446,6 +446,11 @@ static inline Manufacturer get_man(Flash *s)
>  
>  static void blk_sync_complete(void *opaque, int ret)
>  {
> +    QEMUIOVector *iov = opaque;
> +
> +    qemu_iovec_destroy(iov);
> +    g_free(iov);
> +
>      /* do nothing. Masters do not directly interact with the backing store,
>       * only the working copy so no mutexing required.
>       */
> @@ -453,31 +458,31 @@ static void blk_sync_complete(void *opaque, int ret)
>  
>  static void flash_sync_page(Flash *s, int page)
>  {
> -    QEMUIOVector iov;
> +    QEMUIOVector *iov = g_new(QEMUIOVector, 1);
>  
>      if (!s->blk || blk_is_read_only(s->blk)) {
>          return;
>      }
>  
> -    qemu_iovec_init(&iov, 1);
> -    qemu_iovec_add(&iov, s->storage + page * s->pi->page_size,
> +    qemu_iovec_init(iov, 1);
> +    qemu_iovec_add(iov, s->storage + page * s->pi->page_size,
>                     s->pi->page_size);
> -    blk_aio_pwritev(s->blk, page * s->pi->page_size, &iov, 0,
> -                    blk_sync_complete, NULL);
> +    blk_aio_pwritev(s->blk, page * s->pi->page_size, iov, 0,
> +                    blk_sync_complete, iov);
>  }
>  
>  static inline void flash_sync_area(Flash *s, int64_t off, int64_t len)
>  {
> -    QEMUIOVector iov;
> +    QEMUIOVector *iov = g_new(QEMUIOVector, 1);
>  
>      if (!s->blk || blk_is_read_only(s->blk)) {
>          return;
>      }
>  
>      assert(!(len % BDRV_SECTOR_SIZE));
> -    qemu_iovec_init(&iov, 1);
> -    qemu_iovec_add(&iov, s->storage + off, len);
> -    blk_aio_pwritev(s->blk, off, &iov, 0, blk_sync_complete, NULL);
> +    qemu_iovec_init(iov, 1);
> +    qemu_iovec_add(iov, s->storage + off, len);
> +    blk_aio_pwritev(s->blk, off, iov, 0, blk_sync_complete, iov);
>  }
>  
>  static void flash_erase(Flash *s, int offset, FlashCMD cmd)
> 

Re: [Qemu-devel] [PATCH 2/3] m25p80: avoid out of bounds accesses

[Cédric Le Goater]

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 <pbonzini@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 <clg@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:
> 

Re: [Qemu-devel] [PATCH 3/3] m25p80: change cur_addr to 32 bit integer

[Cédric Le Goater]

On 06/28/2016 10:39 AM, Paolo Bonzini wrote:
> The maximum amount of storage that can be addressed by the m25p80 command
> set is 4 GiB.  However, cur_addr is currently a 64-bit integer.  To avoid
> further problems related to sign extension of signed 32-bit integer
> expressions, change cur_addr to a 32 bit integer.  Preserve migration
> format by adding a dummy 4-byte field in place of the (big-endian)
> high four bytes in the formerly 64-bit cur_addr field.

I do not think that migration ever worked before. did it ? 

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

> ---
>  hw/block/m25p80.c | 15 ++++++++-------
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> index 76a9bcf..7668b22 100644
> --- a/hw/block/m25p80.c
> +++ b/hw/block/m25p80.c
> @@ -389,7 +389,7 @@ typedef struct Flash {
>      uint32_t pos;
>      uint8_t needed_bytes;
>      uint8_t cmd_in_progress;
> -    uint64_t cur_addr;
> +    uint32_t cur_addr;
>      uint32_t nonvolatile_cfg;
>      /* Configuration register for Macronix */
>      uint32_t volatile_cfg;
> @@ -535,9 +535,9 @@ static inline void flash_sync_dirty(Flash *s, int64_t newpage)
>  }
>  
>  static inline
> -void flash_write8(Flash *s, uint64_t addr, uint8_t data)
> +void flash_write8(Flash *s, uint32_t addr, uint8_t data)
>  {
> -    int64_t page = addr / s->pi->page_size;
> +    uint32_t page = addr / s->pi->page_size;
>      uint8_t prev = s->storage[s->cur_addr];

This routine needs a cleanup. It takes an 'addr' parameter (it is called 
with s->cur_addr) and uses s->cur_addr at the same time. 

C.

>      if (!s->write_enable) {
> @@ -545,7 +545,7 @@ void flash_write8(Flash *s, uint64_t addr, uint8_t data)
>      }
>  
>      if ((prev ^ data) & data) {
> -        DB_PRINT_L(1, "programming zero to one! addr=%" PRIx64 "  %" PRIx8
> +        DB_PRINT_L(1, "programming zero to one! addr=%" PRIx32 "  %" PRIx8
>                     " -> %" PRIx8 "\n", addr, prev, data);
>      }
>  
> @@ -1094,7 +1094,7 @@ static uint32_t m25p80_transfer8(SSISlave *ss, uint32_t tx)
>      switch (s->state) {
>  
>      case STATE_PAGE_PROGRAM:
> -        DB_PRINT_L(1, "page program cur_addr=%#" PRIx64 " data=%" PRIx8 "\n",
> +        DB_PRINT_L(1, "page program cur_addr=%#" PRIx32 " data=%" PRIx8 "\n",
>                     s->cur_addr, (uint8_t)tx);
>          flash_write8(s, s->cur_addr, (uint8_t)tx);
>          s->cur_addr = (s->cur_addr + 1) & (s->size - 1);
> @@ -1102,7 +1102,7 @@ static uint32_t m25p80_transfer8(SSISlave *ss, uint32_t tx)
>  
>      case STATE_READ:
>          r = s->storage[s->cur_addr];
> -        DB_PRINT_L(1, "READ 0x%" PRIx64 "=%" PRIx8 "\n", s->cur_addr,
> +        DB_PRINT_L(1, "READ 0x%" PRIx32 "=%" PRIx8 "\n", s->cur_addr,
>                     (uint8_t)r);
>          s->cur_addr = (s->cur_addr + 1) & (s->size - 1);
>          break;
> @@ -1199,7 +1199,8 @@ static const VMStateDescription vmstate_m25p80 = {
>          VMSTATE_UINT32(pos, Flash),
>          VMSTATE_UINT8(needed_bytes, Flash),
>          VMSTATE_UINT8(cmd_in_progress, Flash),
> -        VMSTATE_UINT64(cur_addr, Flash),
> +        VMSTATE_UNUSED(4),
> +        VMSTATE_UINT32(cur_addr, Flash),
>          VMSTATE_BOOL(write_enable, Flash),
>          VMSTATE_BOOL_V(reset_enable, Flash, 2),
>          VMSTATE_UINT8_V(ear, Flash, 2),
> 

Re: [Qemu-devel] [PATCH 1/3] m25p80: do not put iovec on the stack

[Cédric Le Goater]

On 06/28/2016 10:53 AM, Cédric Le Goater wrote:
> On 06/28/2016 10:39 AM, Paolo Bonzini wrote:
>> When doing a read-modify-write cycle, QEMU uses the iovec after returning
>> from blk_aio_pwritev.  m25p80 puts the iovec on the stack of blk_aio_pwritev's
>> caller, which causes trouble in this case.  This has been a problem
>> since commit 243e6f6 ("m25p80: Switch to byte-based block access",
>> 2016-05-12) started doing writes at a smaller granularity than 512 bytes.
>> In principle however it could have broken before when using -drive
>> if=mtd,cache=none on a disk with 4K native sectors.
> 
> Ah ! Thanks. That was a problem I was seeing.   
>
> 
> I was thinking we could just do synchronous writes :
> 
> 	https://github.com/legoater/qemu/commit/aef3fe4db3be632077c581541fe30b4e36b5a6f7
> 
> and enable snapshotting like  : 
> 
> 	https://github.com/legoater/qemu/commit/b90ccab7873fd3538b47396ec7c3ae35c8e13270
> 

Anyhow, I tested this patch on a P8/ppc64le system on which the 
assertion was very systematic :

	bdrv_aligned_pwritev: Assertion `!qiov || bytes == qiov->size' failed.

It is gone.

Reviewed-and-tested-by: Cédric Le Goater <clg@kaod.org>

Thanks, 

C. 
 
 
>> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>> ---
>>  hw/block/m25p80.c | 23 ++++++++++++++---------
>>  1 file changed, 14 insertions(+), 9 deletions(-)
>>
>> diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
>> index 09b4767..dd6714d 100644
>> --- a/hw/block/m25p80.c
>> +++ b/hw/block/m25p80.c
>> @@ -446,6 +446,11 @@ static inline Manufacturer get_man(Flash *s)
>>  
>>  static void blk_sync_complete(void *opaque, int ret)
>>  {
>> +    QEMUIOVector *iov = opaque;
>> +
>> +    qemu_iovec_destroy(iov);
>> +    g_free(iov);
>> +
>>      /* do nothing. Masters do not directly interact with the backing store,
>>       * only the working copy so no mutexing required.
>>       */
>> @@ -453,31 +458,31 @@ static void blk_sync_complete(void *opaque, int ret)
>>  
>>  static void flash_sync_page(Flash *s, int page)
>>  {
>> -    QEMUIOVector iov;
>> +    QEMUIOVector *iov = g_new(QEMUIOVector, 1);
>>  
>>      if (!s->blk || blk_is_read_only(s->blk)) {
>>          return;
>>      }
>>  
>> -    qemu_iovec_init(&iov, 1);
>> -    qemu_iovec_add(&iov, s->storage + page * s->pi->page_size,
>> +    qemu_iovec_init(iov, 1);
>> +    qemu_iovec_add(iov, s->storage + page * s->pi->page_size,
>>                     s->pi->page_size);
>> -    blk_aio_pwritev(s->blk, page * s->pi->page_size, &iov, 0,
>> -                    blk_sync_complete, NULL);
>> +    blk_aio_pwritev(s->blk, page * s->pi->page_size, iov, 0,
>> +                    blk_sync_complete, iov);
>>  }
>>  
>>  static inline void flash_sync_area(Flash *s, int64_t off, int64_t len)
>>  {
>> -    QEMUIOVector iov;
>> +    QEMUIOVector *iov = g_new(QEMUIOVector, 1);
>>  
>>      if (!s->blk || blk_is_read_only(s->blk)) {
>>          return;
>>      }
>>  
>>      assert(!(len % BDRV_SECTOR_SIZE));
>> -    qemu_iovec_init(&iov, 1);
>> -    qemu_iovec_add(&iov, s->storage + off, len);
>> -    blk_aio_pwritev(s->blk, off, &iov, 0, blk_sync_complete, NULL);
>> +    qemu_iovec_init(iov, 1);
>> +    qemu_iovec_add(iov, s->storage + off, len);
>> +    blk_aio_pwritev(s->blk, off, iov, 0, blk_sync_complete, iov);
>>  }
>>  
>>  static void flash_erase(Flash *s, int offset, FlashCMD cmd)
>>
> 

Re: [Qemu-devel] [PATCH 3/3] m25p80: change cur_addr to 32 bit integer

[Paolo Bonzini]

> On 06/28/2016 10:39 AM, Paolo Bonzini wrote:
> > The maximum amount of storage that can be addressed by the m25p80 command
> > set is 4 GiB.  However, cur_addr is currently a 64-bit integer.  To avoid
> > further problems related to sign extension of signed 32-bit integer
> > expressions, change cur_addr to a 32 bit integer.  Preserve migration
> > format by adding a dummy 4-byte field in place of the (big-endian)
> > high four bytes in the formerly 64-bit cur_addr field.
> 
> I do not think that migration ever worked before. did it ?

Who knows. :)  But it is pretty easy to not break it further...

Paolo

> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> Reviewed-by: Cédric Le Goater <clg@kaod.org>
> 
> > ---
> >  hw/block/m25p80.c | 15 ++++++++-------
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> > 
> > diff --git a/hw/block/m25p80.c b/hw/block/m25p80.c
> > index 76a9bcf..7668b22 100644
> > --- a/hw/block/m25p80.c
> > +++ b/hw/block/m25p80.c
> > @@ -389,7 +389,7 @@ typedef struct Flash {
> >      uint32_t pos;
> >      uint8_t needed_bytes;
> >      uint8_t cmd_in_progress;
> > -    uint64_t cur_addr;
> > +    uint32_t cur_addr;
> >      uint32_t nonvolatile_cfg;
> >      /* Configuration register for Macronix */
> >      uint32_t volatile_cfg;
> > @@ -535,9 +535,9 @@ static inline void flash_sync_dirty(Flash *s, int64_t
> > newpage)
> >  }
> >  
> >  static inline
> > -void flash_write8(Flash *s, uint64_t addr, uint8_t data)
> > +void flash_write8(Flash *s, uint32_t addr, uint8_t data)
> >  {
> > -    int64_t page = addr / s->pi->page_size;
> > +    uint32_t page = addr / s->pi->page_size;
> >      uint8_t prev = s->storage[s->cur_addr];
> 
> This routine needs a cleanup. It takes an 'addr' parameter (it is called
> with s->cur_addr) and uses s->cur_addr at the same time.
> 
> C.
> 
> >      if (!s->write_enable) {
> > @@ -545,7 +545,7 @@ void flash_write8(Flash *s, uint64_t addr, uint8_t
> > data)
> >      }
> >  
> >      if ((prev ^ data) & data) {
> > -        DB_PRINT_L(1, "programming zero to one! addr=%" PRIx64 "  %" PRIx8
> > +        DB_PRINT_L(1, "programming zero to one! addr=%" PRIx32 "  %" PRIx8
> >                     " -> %" PRIx8 "\n", addr, prev, data);
> >      }
> >  
> > @@ -1094,7 +1094,7 @@ static uint32_t m25p80_transfer8(SSISlave *ss,
> > uint32_t tx)
> >      switch (s->state) {
> >  
> >      case STATE_PAGE_PROGRAM:
> > -        DB_PRINT_L(1, "page program cur_addr=%#" PRIx64 " data=%" PRIx8
> > "\n",
> > +        DB_PRINT_L(1, "page program cur_addr=%#" PRIx32 " data=%" PRIx8
> > "\n",
> >                     s->cur_addr, (uint8_t)tx);
> >          flash_write8(s, s->cur_addr, (uint8_t)tx);
> >          s->cur_addr = (s->cur_addr + 1) & (s->size - 1);
> > @@ -1102,7 +1102,7 @@ static uint32_t m25p80_transfer8(SSISlave *ss,
> > uint32_t tx)
> >  
> >      case STATE_READ:
> >          r = s->storage[s->cur_addr];
> > -        DB_PRINT_L(1, "READ 0x%" PRIx64 "=%" PRIx8 "\n", s->cur_addr,
> > +        DB_PRINT_L(1, "READ 0x%" PRIx32 "=%" PRIx8 "\n", s->cur_addr,
> >                     (uint8_t)r);
> >          s->cur_addr = (s->cur_addr + 1) & (s->size - 1);
> >          break;
> > @@ -1199,7 +1199,8 @@ static const VMStateDescription vmstate_m25p80 = {
> >          VMSTATE_UINT32(pos, Flash),
> >          VMSTATE_UINT8(needed_bytes, Flash),
> >          VMSTATE_UINT8(cmd_in_progress, Flash),
> > -        VMSTATE_UINT64(cur_addr, Flash),
> > +        VMSTATE_UNUSED(4),
> > +        VMSTATE_UINT32(cur_addr, Flash),
> >          VMSTATE_BOOL(write_enable, Flash),
> >          VMSTATE_BOOL_V(reset_enable, Flash, 2),
> >          VMSTATE_UINT8_V(ear, Flash, 2),
> > 
> 
> 

Re: [Qemu-devel] [PATCH 2/3] m25p80: avoid out of bounds accesses

[Paolo Bonzini]

----- Original Message -----
> From: "Cédric Le Goater" <clg@kaod.org>
> To: "Paolo Bonzini" <pbonzini@redhat.com>, qemu-devel@nongnu.org
> Sent: Tuesday, June 28, 2016 11:05:24 AM
> Subject: Re: [PATCH 2/3] m25p80: avoid out of bounds accesses
> 
> 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 <pbonzini@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.

I don't know, it's quite possible that real hardware does an AND
simply because some address lines are not connected anywhere.

Paolo

Re: [Qemu-devel] m25p80: avoid out of bounds accesses

[Krzeminski, Marcin (Nokia - PL/Wroclaw)]

> -----Original Message-----
> From: Qemu-devel [mailto:qemu-devel-
> bounces+marcin.krzeminski=nokia.com@nongnu.org] On Behalf Of Paolo
> Bonzini
> Sent: Tuesday, June 28, 2016 1:42 PM
> To: Cédric Le Goater <clg@kaod.org>
> Cc: qemu-devel@nongnu.org
> Subject: Re: [Qemu-devel] [PATCH 2/3] m25p80: avoid out of bounds
> accesses
> 
> 
> 
> ----- Original Message -----
> > From: "Cédric Le Goater" <clg@kaod.org>
> > To: "Paolo Bonzini" <pbonzini@redhat.com>, qemu-devel@nongnu.org
> > Sent: Tuesday, June 28, 2016 11:05:24 AM
> > Subject: Re: [PATCH 2/3] m25p80: avoid out of bounds accesses
> >
> > 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 <pbonzini@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.
> 
> I don't know, it's quite possible that real hardware does an AND simply
> because some address lines are not connected anywhere.
> 
> Paolo
Real HW wraps when address reach end of the memory (it could be
configurable for some devices).

Reviewed by: Marcin Krzeminski <marcin.krzeminski@nokia.com>

Regards,
Marcin

Re: [Qemu-devel] [PATCH 1/3] m25p80: do not put iovec on the stack

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 920 bytes --]

On 06/28/2016 02:39 AM, Paolo Bonzini wrote:
> When doing a read-modify-write cycle, QEMU uses the iovec after returning
> from blk_aio_pwritev.  m25p80 puts the iovec on the stack of blk_aio_pwritev's
> caller, which causes trouble in this case.  This has been a problem
> since commit 243e6f6 ("m25p80: Switch to byte-based block access",
> 2016-05-12) started doing writes at a smaller granularity than 512 bytes.
> In principle however it could have broken before when using -drive
> if=mtd,cache=none on a disk with 4K native sectors.
> 
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  hw/block/m25p80.c | 23 ++++++++++++++---------
>  1 file changed, 14 insertions(+), 9 deletions(-)
> 

Reviewed-by: Eric Blake <eblake@redhat.com>

and thanks for tracking this one down

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]