[PATCH v3 0/3] hw/elf_ops: clear uninitialized segment space

[PATCH v3 0/3] hw/elf_ops: clear uninitialized segment space

[Laurent Vivier]

When the mem_size of the segment is bigger than the file_size,
and if this space doesn't overlap another segment, it needs
to be cleared.

When the file is loaded in RAM, it is cleared by the loader (PATCH 2),
when the file is loaded in a ROM, the space is cleared on reset,
when the data of the file is copied from the data buffer to
the machine memory space (PATCH 3).

This series a new function address_space_set() to clear the memory.

v3: add a patch to clear the uninitialized space of the ROM
v2: PMD introduces address_space_set() function

Laurent Vivier (2):
  hw/elf_ops: clear uninitialized segment space
  hw/core/loader: clear uninitialized ROM space

Philippe Mathieu-Daudé (1):
  exec/memory: Extract address_space_set() from dma_memory_set()

 include/exec/memory.h | 16 ++++++++++++++++
 include/hw/elf_ops.h  | 13 +++++++++++++
 hw/core/loader.c      |  4 ++++
 softmmu/dma-helpers.c | 16 +---------------
 softmmu/physmem.c     | 19 +++++++++++++++++++
 5 files changed, 53 insertions(+), 15 deletions(-)

-- 
2.31.1

[PATCH v3 1/3] exec/memory: Extract address_space_set() from dma_memory_set()

[Laurent Vivier]

From: Philippe Mathieu-Daudé <philmd@redhat.com>

dma_memory_set() does a DMA barrier, set the address space with
a constant value. The constant value filling code is not specific
to DMA and can be used for AddressSpace. Extract it as a new
helper: address_space_set().

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Laurent Vivier <laurent@vivier.eu>
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
 include/exec/memory.h | 16 ++++++++++++++++
 softmmu/dma-helpers.c | 16 +---------------
 softmmu/physmem.c     | 19 +++++++++++++++++++
 3 files changed, 36 insertions(+), 15 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index 5728a681b27d..192139af58e9 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -2568,6 +2568,22 @@ address_space_write_cached(MemoryRegionCache *cache, hwaddr addr,
     }
 }
 
+/**
+ * address_space_set: Fill address space with a constant byte.
+ *
+ * Return a MemTxResult indicating whether the operation succeeded
+ * or failed (eg unassigned memory, device rejected the transaction,
+ * IOMMU fault).
+ *
+ * @as: #AddressSpace to be accessed
+ * @addr: address within that address space
+ * @c: constant byte to fill the memory
+ * @len: the number of bytes to fill with the constant byte
+ * @attrs: memory transaction attributes
+ */
+MemTxResult address_space_set(AddressSpace *as, hwaddr addr,
+                              uint8_t c, hwaddr len, MemTxAttrs attrs);
+
 #ifdef NEED_CPU_H
 /* enum device_endian to MemOp.  */
 static inline MemOp devend_memop(enum device_endian end)
diff --git a/softmmu/dma-helpers.c b/softmmu/dma-helpers.c
index 7d766a5e89a3..8e1e7ad53206 100644
--- a/softmmu/dma-helpers.c
+++ b/softmmu/dma-helpers.c
@@ -23,21 +23,7 @@ MemTxResult dma_memory_set(AddressSpace *as, dma_addr_t addr,
 {
     dma_barrier(as, DMA_DIRECTION_FROM_DEVICE);
 
-#define FILLBUF_SIZE 512
-    uint8_t fillbuf[FILLBUF_SIZE];
-    int l;
-    MemTxResult error = MEMTX_OK;
-
-    memset(fillbuf, c, FILLBUF_SIZE);
-    while (len > 0) {
-        l = len < FILLBUF_SIZE ? len : FILLBUF_SIZE;
-        error |= address_space_write(as, addr, MEMTXATTRS_UNSPECIFIED,
-                                     fillbuf, l);
-        len -= l;
-        addr += l;
-    }
-
-    return error;
+    return address_space_set(as, addr, c, len, MEMTXATTRS_UNSPECIFIED);
 }
 
 void qemu_sglist_init(QEMUSGList *qsg, DeviceState *dev, int alloc_hint,
diff --git a/softmmu/physmem.c b/softmmu/physmem.c
index 85034d9c11e3..c9117527ae71 100644
--- a/softmmu/physmem.c
+++ b/softmmu/physmem.c
@@ -2891,6 +2891,25 @@ MemTxResult address_space_rw(AddressSpace *as, hwaddr addr, MemTxAttrs attrs,
     }
 }
 
+MemTxResult address_space_set(AddressSpace *as, hwaddr addr,
+                              uint8_t c, hwaddr len, MemTxAttrs attrs)
+{
+#define FILLBUF_SIZE 512
+    uint8_t fillbuf[FILLBUF_SIZE];
+    int l;
+    MemTxResult error = MEMTX_OK;
+
+    memset(fillbuf, c, FILLBUF_SIZE);
+    while (len > 0) {
+        l = len < FILLBUF_SIZE ? len : FILLBUF_SIZE;
+        error |= address_space_write(as, addr, attrs, fillbuf, l);
+        len -= l;
+        addr += l;
+    }
+
+    return error;
+}
+
 void cpu_physical_memory_rw(hwaddr addr, void *buf,
                             hwaddr len, bool is_write)
 {
-- 
2.31.1

[PATCH v3 2/3] hw/elf_ops: clear uninitialized segment space

[Laurent Vivier]

When the mem_size of the segment is bigger than the file_size,
and if this space doesn't overlap another segment, it needs
to be cleared.

This bug is very similar to the one we had for linux-user,
22d113b52f41 ("linux-user: Fix loading of BSS segments"),
where .bss section is encoded as an extension of the the data
one by setting the segment p_memsz > p_filesz.

Signed-off-by: Laurent Vivier <laurent@vivier.eu>
[PMD: Use recently added address_space_set()]
Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/hw/elf_ops.h | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
index 6ee458e7bc3c..29f4c43e231d 100644
--- a/include/hw/elf_ops.h
+++ b/include/hw/elf_ops.h
@@ -562,6 +562,19 @@ static int glue(load_elf, SZ)(const char *name, int fd,
                     if (res != MEMTX_OK) {
                         goto fail;
                     }
+                    /*
+                     * We need to zero'ify the space that is not copied
+                     * from file
+                     */
+                    if (file_size < mem_size) {
+                        res = address_space_set(as ? as : &address_space_memory,
+                                                addr + file_size, 0,
+                                                mem_size - file_size,
+                                                MEMTXATTRS_UNSPECIFIED);
+                        if (res != MEMTX_OK) {
+                            goto fail;
+                        }
+                    }
                 }
             }
 
-- 
2.31.1

[PATCH v3 3/3] hw/core/loader: clear uninitialized ROM space

[Laurent Vivier]

As for "hw/elf_ops: clear uninitialized segment space" we need to
clear the uninitialized space when the ELF is set in ROM.

Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
 hw/core/loader.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/hw/core/loader.c b/hw/core/loader.c
index d3e5f3b423f6..8146fdcbb7a0 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -1146,9 +1146,13 @@ static void rom_reset(void *unused)
         if (rom->mr) {
             void *host = memory_region_get_ram_ptr(rom->mr);
             memcpy(host, rom->data, rom->datasize);
+            memset(host + rom->datasize, 0, rom->romsize - rom->datasize);
         } else {
             address_space_write_rom(rom->as, rom->addr, MEMTXATTRS_UNSPECIFIED,
                                     rom->data, rom->datasize);
+            address_space_set(rom->as, rom->addr + rom->datasize, 0,
+                              rom->romsize - rom->datasize,
+                              MEMTXATTRS_UNSPECIFIED);
         }
         if (rom->isrom) {
             /* rom needs to be written only once */
-- 
2.31.1

Re: [PATCH v3 3/3] hw/core/loader: clear uninitialized ROM space

[Philippe Mathieu-Daudé]

On 4/29/21 4:13 PM, Laurent Vivier wrote:
> As for "hw/elf_ops: clear uninitialized segment space" we need to
> clear the uninitialized space when the ELF is set in ROM.
> 
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
>  hw/core/loader.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index d3e5f3b423f6..8146fdcbb7a0 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -1146,9 +1146,13 @@ static void rom_reset(void *unused)
>          if (rom->mr) {
>              void *host = memory_region_get_ram_ptr(rom->mr);
>              memcpy(host, rom->data, rom->datasize);
> +            memset(host + rom->datasize, 0, rom->romsize - rom->datasize);
>          } else {
>              address_space_write_rom(rom->as, rom->addr, MEMTXATTRS_UNSPECIFIED,
>                                      rom->data, rom->datasize);
> +            address_space_set(rom->as, rom->addr + rom->datasize, 0,
> +                              rom->romsize - rom->datasize,
> +                              MEMTXATTRS_UNSPECIFIED);
>          }
>          if (rom->isrom) {
>              /* rom needs to be written only once */
> 

This is consistent with the comment from commit d60fa42e8ba
("Save memory allocation in the elf loader"):

    /* datasize is the amount of memory allocated in "data". If datasize
is less
     * than romsize, it means that the area from datasize to romsize is
filled
     * with zeros.
     */

Therefore:

Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>

However depending on the underlying media, there might be cases
where we want to fill with -1 instead. Just to keep in mind, if
one day it bites us.

Regards,

Phil.

Re: [PATCH v3 3/3] hw/core/loader: clear uninitialized ROM space

[Stefano Garzarella]

On Thu, Apr 29, 2021 at 04:13:26PM +0200, Laurent Vivier wrote:
>As for "hw/elf_ops: clear uninitialized segment space" we need to
>clear the uninitialized space when the ELF is set in ROM.
>
>Signed-off-by: Laurent Vivier <laurent@vivier.eu>

Reviewed-by: Stefano Garzarella <sgarzare@redhat.com>

Re: [PATCH v3 0/3] hw/elf_ops: clear uninitialized segment space

[Laurent Vivier]

Le 29/04/2021 à 16:13, Laurent Vivier a écrit :
> When the mem_size of the segment is bigger than the file_size,
> and if this space doesn't overlap another segment, it needs
> to be cleared.
> 
> When the file is loaded in RAM, it is cleared by the loader (PATCH 2),
> when the file is loaded in a ROM, the space is cleared on reset,
> when the data of the file is copied from the data buffer to
> the machine memory space (PATCH 3).
> 
> This series a new function address_space_set() to clear the memory.
> 
> v3: add a patch to clear the uninitialized space of the ROM
> v2: PMD introduces address_space_set() function
> 
> Laurent Vivier (2):
>   hw/elf_ops: clear uninitialized segment space
>   hw/core/loader: clear uninitialized ROM space
> 
> Philippe Mathieu-Daudé (1):
>   exec/memory: Extract address_space_set() from dma_memory_set()
> 
>  include/exec/memory.h | 16 ++++++++++++++++
>  include/hw/elf_ops.h  | 13 +++++++++++++
>  hw/core/loader.c      |  4 ++++
>  softmmu/dma-helpers.c | 16 +---------------
>  softmmu/physmem.c     | 19 +++++++++++++++++++
>  5 files changed, 53 insertions(+), 15 deletions(-)
> 

Anyone to merge the series?

Thanks,
Laurent