KVM Archive on lore.kernel.org
 help / color / Atom feed
* [PATCH v2 0/7] exec/memory: Enforce checking MemTxResult values
@ 2020-05-18 15:53 Philippe Mathieu-Daudé
  2020-05-18 15:53 ` [PATCH v2 1/7] exec: Let address_space_read/write_cached() propagate MemTxResult Philippe Mathieu-Daudé
                   ` (7 more replies)
  0 siblings, 8 replies; 18+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-18 15:53 UTC (permalink / raw)
  To: qemu-devel
  Cc: Peter Maydell, Paolo Bonzini, kvm, qemu-arm, Richard Henderson,
	Philippe Mathieu-Daudé

Various places ignore the MemTxResult indicator of
transaction failed. Fix the easy places.
The rest are the DMA devices, which require deeper
analysis.

Since v1:
- Dropped "exec/memory: Emit warning when MemTxResult is ignored"
  https://www.mail-archive.com/qemu-devel@nongnu.org/msg704180.html

Philippe Mathieu-Daudé (7):
  exec: Let address_space_read/write_cached() propagate MemTxResult
  exec: Propagate cpu_memory_rw_debug() error
  disas: Let disas::read_memory() handler return EIO on error
  hw/elf_ops: Do not ignore write failures when loading ELF
  hw/arm/boot: Abort if set_kernel_args() fails
  accel/kvm: Let KVM_EXIT_MMIO return error
  hw/core/loader: Assert loading ROM regions succeeds at reset

 include/exec/cpu-all.h |  1 +
 include/exec/memory.h  | 19 +++++++++++--------
 include/hw/elf_ops.h   | 11 ++++++++---
 accel/kvm/kvm-all.c    | 13 +++++++------
 disas.c                | 13 ++++++++-----
 exec.c                 | 28 ++++++++++++++++------------
 hw/arm/boot.c          | 19 +++++++++++++------
 hw/core/loader.c       |  8 ++++++--
 8 files changed, 70 insertions(+), 42 deletions(-)

-- 
2.21.3


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v2 1/7] exec: Let address_space_read/write_cached() propagate MemTxResult
  2020-05-18 15:53 [PATCH v2 0/7] exec/memory: Enforce checking MemTxResult values Philippe Mathieu-Daudé
@ 2020-05-18 15:53 ` Philippe Mathieu-Daudé
  2020-05-18 16:13   ` Peter Maydell
  2020-05-18 15:53 ` [PATCH v2 2/7] exec: Propagate cpu_memory_rw_debug() error Philippe Mathieu-Daudé
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 18+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-18 15:53 UTC (permalink / raw)
  To: qemu-devel
  Cc: Peter Maydell, Paolo Bonzini, kvm, qemu-arm, Richard Henderson,
	Philippe Mathieu-Daudé

Both address_space_read_cached_slow() and
address_space_write_cached_slow() return a MemTxResult type.
Do not discard it, return it to the caller.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/exec/memory.h | 19 +++++++++++--------
 exec.c                | 16 ++++++++--------
 2 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/include/exec/memory.h b/include/exec/memory.h
index e000bd2f97..5e8c009169 100644
--- a/include/exec/memory.h
+++ b/include/exec/memory.h
@@ -2343,10 +2343,11 @@ void *qemu_map_ram_ptr(RAMBlock *ram_block, ram_addr_t addr);
 
 /* Internal functions, part of the implementation of address_space_read_cached
  * and address_space_write_cached.  */
-void address_space_read_cached_slow(MemoryRegionCache *cache,
-                                    hwaddr addr, void *buf, hwaddr len);
-void address_space_write_cached_slow(MemoryRegionCache *cache,
-                                     hwaddr addr, const void *buf, hwaddr len);
+MemTxResult address_space_read_cached_slow(MemoryRegionCache *cache,
+                                           hwaddr addr, void *buf, hwaddr len);
+MemTxResult address_space_write_cached_slow(MemoryRegionCache *cache,
+                                            hwaddr addr, const void *buf,
+                                            hwaddr len);
 
 static inline bool memory_access_is_direct(MemoryRegion *mr, bool is_write)
 {
@@ -2411,15 +2412,16 @@ MemTxResult address_space_read(AddressSpace *as, hwaddr addr,
  * @buf: buffer with the data transferred
  * @len: length of the data transferred
  */
-static inline void
+static inline MemTxResult
 address_space_read_cached(MemoryRegionCache *cache, hwaddr addr,
                           void *buf, hwaddr len)
 {
     assert(addr < cache->len && len <= cache->len - addr);
     if (likely(cache->ptr)) {
         memcpy(buf, cache->ptr + addr, len);
+        return MEMTX_OK;
     } else {
-        address_space_read_cached_slow(cache, addr, buf, len);
+        return address_space_read_cached_slow(cache, addr, buf, len);
     }
 }
 
@@ -2431,15 +2433,16 @@ address_space_read_cached(MemoryRegionCache *cache, hwaddr addr,
  * @buf: buffer with the data transferred
  * @len: length of the data transferred
  */
-static inline void
+static inline MemTxResult
 address_space_write_cached(MemoryRegionCache *cache, hwaddr addr,
                            const void *buf, hwaddr len)
 {
     assert(addr < cache->len && len <= cache->len - addr);
     if (likely(cache->ptr)) {
         memcpy(cache->ptr + addr, buf, len);
+        return MEMTX_OK;
     } else {
-        address_space_write_cached_slow(cache, addr, buf, len);
+        return address_space_write_cached_slow(cache, addr, buf, len);
     }
 }
 
diff --git a/exec.c b/exec.c
index 5162f0d12f..877b51cc5c 100644
--- a/exec.c
+++ b/exec.c
@@ -3716,7 +3716,7 @@ static inline MemoryRegion *address_space_translate_cached(
 /* Called from RCU critical section. address_space_read_cached uses this
  * out of line function when the target is an MMIO or IOMMU region.
  */
-void
+MemTxResult
 address_space_read_cached_slow(MemoryRegionCache *cache, hwaddr addr,
                                    void *buf, hwaddr len)
 {
@@ -3726,15 +3726,15 @@ address_space_read_cached_slow(MemoryRegionCache *cache, hwaddr addr,
     l = len;
     mr = address_space_translate_cached(cache, addr, &addr1, &l, false,
                                         MEMTXATTRS_UNSPECIFIED);
-    flatview_read_continue(cache->fv,
-                           addr, MEMTXATTRS_UNSPECIFIED, buf, len,
-                           addr1, l, mr);
+    return flatview_read_continue(cache->fv,
+                                  addr, MEMTXATTRS_UNSPECIFIED, buf, len,
+                                  addr1, l, mr);
 }
 
 /* Called from RCU critical section. address_space_write_cached uses this
  * out of line function when the target is an MMIO or IOMMU region.
  */
-void
+MemTxResult
 address_space_write_cached_slow(MemoryRegionCache *cache, hwaddr addr,
                                     const void *buf, hwaddr len)
 {
@@ -3744,9 +3744,9 @@ address_space_write_cached_slow(MemoryRegionCache *cache, hwaddr addr,
     l = len;
     mr = address_space_translate_cached(cache, addr, &addr1, &l, true,
                                         MEMTXATTRS_UNSPECIFIED);
-    flatview_write_continue(cache->fv,
-                            addr, MEMTXATTRS_UNSPECIFIED, buf, len,
-                            addr1, l, mr);
+    return flatview_write_continue(cache->fv,
+                                   addr, MEMTXATTRS_UNSPECIFIED, buf, len,
+                                   addr1, l, mr);
 }
 
 #define ARG1_DECL                MemoryRegionCache *cache
-- 
2.21.3


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v2 2/7] exec: Propagate cpu_memory_rw_debug() error
  2020-05-18 15:53 [PATCH v2 0/7] exec/memory: Enforce checking MemTxResult values Philippe Mathieu-Daudé
  2020-05-18 15:53 ` [PATCH v2 1/7] exec: Let address_space_read/write_cached() propagate MemTxResult Philippe Mathieu-Daudé
@ 2020-05-18 15:53 ` Philippe Mathieu-Daudé
  2020-05-18 15:53 ` [PATCH v2 3/7] disas: Let disas::read_memory() handler return EIO on error Philippe Mathieu-Daudé
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 18+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-18 15:53 UTC (permalink / raw)
  To: qemu-devel
  Cc: Peter Maydell, Paolo Bonzini, kvm, qemu-arm, Richard Henderson,
	Philippe Mathieu-Daudé

Do not ignore the MemTxResult error type returned by
the address_space_rw() API.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/exec/cpu-all.h |  1 +
 exec.c                 | 12 ++++++++----
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/include/exec/cpu-all.h b/include/exec/cpu-all.h
index d14374bdd4..fb4e8a8e29 100644
--- a/include/exec/cpu-all.h
+++ b/include/exec/cpu-all.h
@@ -413,6 +413,7 @@ void dump_exec_info(void);
 void dump_opcount_info(void);
 #endif /* !CONFIG_USER_ONLY */
 
+/* Returns: 0 on success, -1 on error */
 int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
                         void *ptr, target_ulong len, bool is_write);
 
diff --git a/exec.c b/exec.c
index 877b51cc5c..ae5a6944ef 100644
--- a/exec.c
+++ b/exec.c
@@ -3769,6 +3769,7 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
     while (len > 0) {
         int asidx;
         MemTxAttrs attrs;
+        MemTxResult res;
 
         page = addr & TARGET_PAGE_MASK;
         phys_addr = cpu_get_phys_page_attrs_debug(cpu, page, &attrs);
@@ -3781,11 +3782,14 @@ int cpu_memory_rw_debug(CPUState *cpu, target_ulong addr,
             l = len;
         phys_addr += (addr & ~TARGET_PAGE_MASK);
         if (is_write) {
-            address_space_write_rom(cpu->cpu_ases[asidx].as, phys_addr,
-                                    attrs, buf, l);
+            res = address_space_write_rom(cpu->cpu_ases[asidx].as, phys_addr,
+                                          attrs, buf, l);
         } else {
-            address_space_read(cpu->cpu_ases[asidx].as, phys_addr, attrs, buf,
-                               l);
+            res = address_space_read(cpu->cpu_ases[asidx].as, phys_addr,
+                                     attrs, buf, l);
+        }
+        if (res != MEMTX_OK) {
+            return -1;
         }
         len -= l;
         buf += l;
-- 
2.21.3


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v2 3/7] disas: Let disas::read_memory() handler return EIO on error
  2020-05-18 15:53 [PATCH v2 0/7] exec/memory: Enforce checking MemTxResult values Philippe Mathieu-Daudé
  2020-05-18 15:53 ` [PATCH v2 1/7] exec: Let address_space_read/write_cached() propagate MemTxResult Philippe Mathieu-Daudé
  2020-05-18 15:53 ` [PATCH v2 2/7] exec: Propagate cpu_memory_rw_debug() error Philippe Mathieu-Daudé
@ 2020-05-18 15:53 ` Philippe Mathieu-Daudé
  2020-05-18 15:53 ` [PATCH v2 4/7] hw/elf_ops: Do not ignore write failures when loading ELF Philippe Mathieu-Daudé
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 18+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-18 15:53 UTC (permalink / raw)
  To: qemu-devel
  Cc: Peter Maydell, Paolo Bonzini, kvm, qemu-arm, Richard Henderson,
	Philippe Mathieu-Daudé

Both cpu_memory_rw_debug() and address_space_read() return
an error on failed transaction. Check the returned value,
and return EIO in case of error.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 disas.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/disas.c b/disas.c
index 45285d3f63..c1397d3933 100644
--- a/disas.c
+++ b/disas.c
@@ -39,9 +39,11 @@ target_read_memory (bfd_vma memaddr,
                     struct disassemble_info *info)
 {
     CPUDebug *s = container_of(info, CPUDebug, info);
+    int r;
 
-    cpu_memory_rw_debug(s->cpu, memaddr, myaddr, length, 0);
-    return 0;
+    r = cpu_memory_rw_debug(s->cpu, memaddr, myaddr, length, 0);
+
+    return r ? EIO : 0;
 }
 
 /* Print an error message.  We can assume that this is in response to
@@ -718,10 +720,11 @@ physical_read_memory(bfd_vma memaddr, bfd_byte *myaddr, int length,
                      struct disassemble_info *info)
 {
     CPUDebug *s = container_of(info, CPUDebug, info);
+    MemTxResult res;
 
-    address_space_read(s->cpu->as, memaddr, MEMTXATTRS_UNSPECIFIED,
-                       myaddr, length);
-    return 0;
+    res = address_space_read(s->cpu->as, memaddr, MEMTXATTRS_UNSPECIFIED,
+                             myaddr, length);
+    return res == MEMTX_OK ? 0 : EIO;
 }
 
 /* Disassembler for the monitor.  */
-- 
2.21.3


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v2 4/7] hw/elf_ops: Do not ignore write failures when loading ELF
  2020-05-18 15:53 [PATCH v2 0/7] exec/memory: Enforce checking MemTxResult values Philippe Mathieu-Daudé
                   ` (2 preceding siblings ...)
  2020-05-18 15:53 ` [PATCH v2 3/7] disas: Let disas::read_memory() handler return EIO on error Philippe Mathieu-Daudé
@ 2020-05-18 15:53 ` Philippe Mathieu-Daudé
  2020-05-19  7:58   ` Stefano Garzarella
  2020-05-18 15:53 ` [PATCH v2 5/7] hw/arm/boot: Abort if set_kernel_args() fails Philippe Mathieu-Daudé
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 18+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-18 15:53 UTC (permalink / raw)
  To: qemu-devel
  Cc: Peter Maydell, Paolo Bonzini, kvm, qemu-arm, Richard Henderson,
	Philippe Mathieu-Daudé

Do not ignore the MemTxResult error type returned by
address_space_write().

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/hw/elf_ops.h | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
index 398a4a2c85..6fdff3dced 100644
--- a/include/hw/elf_ops.h
+++ b/include/hw/elf_ops.h
@@ -553,9 +553,14 @@ static int glue(load_elf, SZ)(const char *name, int fd,
                     rom_add_elf_program(label, mapped_file, data, file_size,
                                         mem_size, addr, as);
                 } else {
-                    address_space_write(as ? as : &address_space_memory,
-                                        addr, MEMTXATTRS_UNSPECIFIED,
-                                        data, file_size);
+                    MemTxResult res;
+
+                    res = address_space_write(as ? as : &address_space_memory,
+                                              addr, MEMTXATTRS_UNSPECIFIED,
+                                              data, file_size);
+                    if (res != MEMTX_OK) {
+                        goto fail;
+                    }
                 }
             }
 
-- 
2.21.3


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [PATCH v2 5/7] hw/arm/boot: Abort if set_kernel_args() fails
  2020-05-18 15:53 [PATCH v2 0/7] exec/memory: Enforce checking MemTxResult values Philippe Mathieu-Daudé
                   ` (3 preceding siblings ...)
  2020-05-18 15:53 ` [PATCH v2 4/7] hw/elf_ops: Do not ignore write failures when loading ELF Philippe Mathieu-Daudé
@ 2020-05-18 15:53 ` Philippe Mathieu-Daudé
  2020-05-18 16:08   ` Peter Maydell
  2020-05-18 15:53 ` [RFC PATCH v2 6/7] accel/kvm: Let KVM_EXIT_MMIO return error Philippe Mathieu-Daudé
                   ` (2 subsequent siblings)
  7 siblings, 1 reply; 18+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-18 15:53 UTC (permalink / raw)
  To: qemu-devel
  Cc: Peter Maydell, Paolo Bonzini, kvm, qemu-arm, Richard Henderson,
	Philippe Mathieu-Daudé

If a address_space_write() fails while calling
set_kernel_args(), the guest kernel will boot
using crap data. Avoid that by aborting if this
ever occurs.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/arm/boot.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index fef4072db1..7cc271034c 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -291,7 +291,8 @@ static inline bool have_dtb(const struct arm_boot_info *info)
 
 #define WRITE_WORD(p, value) do { \
     address_space_stl_notdirty(as, p, value, \
-                               MEMTXATTRS_UNSPECIFIED, NULL);  \
+                               MEMTXATTRS_UNSPECIFIED, &result); \
+    assert(result == MEMTX_OK); \
     p += 4;                       \
 } while (0)
 
@@ -300,6 +301,7 @@ static void set_kernel_args(const struct arm_boot_info *info, AddressSpace *as)
     int initrd_size = info->initrd_size;
     hwaddr base = info->loader_start;
     hwaddr p;
+    MemTxResult result;
 
     p = base + KERNEL_ARGS_ADDR;
     /* ATAG_CORE */
@@ -326,8 +328,9 @@ static void set_kernel_args(const struct arm_boot_info *info, AddressSpace *as)
         int cmdline_size;
 
         cmdline_size = strlen(info->kernel_cmdline);
-        address_space_write(as, p + 8, MEMTXATTRS_UNSPECIFIED,
-                            info->kernel_cmdline, cmdline_size + 1);
+        result = address_space_write(as, p + 8, MEMTXATTRS_UNSPECIFIED,
+                                     info->kernel_cmdline, cmdline_size + 1);
+        assert(result == MEMTX_OK);
         cmdline_size = (cmdline_size >> 2) + 1;
         WRITE_WORD(p, cmdline_size + 2);
         WRITE_WORD(p, 0x54410009);
@@ -341,8 +344,9 @@ static void set_kernel_args(const struct arm_boot_info *info, AddressSpace *as)
         atag_board_len = (info->atag_board(info, atag_board_buf) + 3) & ~3;
         WRITE_WORD(p, (atag_board_len + 8) >> 2);
         WRITE_WORD(p, 0x414f4d50);
-        address_space_write(as, p, MEMTXATTRS_UNSPECIFIED,
-                            atag_board_buf, atag_board_len);
+        result = address_space_write(as, p, MEMTXATTRS_UNSPECIFIED,
+                                     atag_board_buf, atag_board_len);
+        assert(result == MEMTX_OK);
         p += atag_board_len;
     }
     /* ATAG_END */
@@ -357,6 +361,7 @@ static void set_kernel_args_old(const struct arm_boot_info *info,
     const char *s;
     int initrd_size = info->initrd_size;
     hwaddr base = info->loader_start;
+    MemTxResult result;
 
     /* see linux/include/asm-arm/setup.h */
     p = base + KERNEL_ARGS_ADDR;
@@ -419,7 +424,9 @@ static void set_kernel_args_old(const struct arm_boot_info *info,
     }
     s = info->kernel_cmdline;
     if (s) {
-        address_space_write(as, p, MEMTXATTRS_UNSPECIFIED, s, strlen(s) + 1);
+        result = address_space_write(as, p, MEMTXATTRS_UNSPECIFIED,
+                                     s, strlen(s) + 1);
+        assert(result == MEMTX_OK);
     } else {
         WRITE_WORD(p, 0);
     }
-- 
2.21.3


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [RFC PATCH v2 6/7] accel/kvm: Let KVM_EXIT_MMIO return error
  2020-05-18 15:53 [PATCH v2 0/7] exec/memory: Enforce checking MemTxResult values Philippe Mathieu-Daudé
                   ` (4 preceding siblings ...)
  2020-05-18 15:53 ` [PATCH v2 5/7] hw/arm/boot: Abort if set_kernel_args() fails Philippe Mathieu-Daudé
@ 2020-05-18 15:53 ` Philippe Mathieu-Daudé
  2020-05-18 16:01   ` Peter Maydell
  2020-05-18 15:53 ` [RFC PATCH v2 7/7] hw/core/loader: Assert loading ROM regions succeeds at reset Philippe Mathieu-Daudé
  2020-05-21 15:40 ` [PATCH v2 0/7] exec/memory: Enforce checking MemTxResult values Paolo Bonzini
  7 siblings, 1 reply; 18+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-18 15:53 UTC (permalink / raw)
  To: qemu-devel
  Cc: Peter Maydell, Paolo Bonzini, kvm, qemu-arm, Richard Henderson,
	Philippe Mathieu-Daudé

Give the hypervisor a possibility to catch any error
occuring during KVM_EXIT_MMIO.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
RFC because maybe we simply want to ignore this error instead
---
 accel/kvm/kvm-all.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/accel/kvm/kvm-all.c b/accel/kvm/kvm-all.c
index d06cc04079..8dbcb8fda3 100644
--- a/accel/kvm/kvm-all.c
+++ b/accel/kvm/kvm-all.c
@@ -2357,6 +2357,7 @@ int kvm_cpu_exec(CPUState *cpu)
 
     do {
         MemTxAttrs attrs;
+        MemTxResult res;
 
         if (cpu->vcpu_dirty) {
             kvm_arch_put_registers(cpu, KVM_PUT_RUNTIME_STATE);
@@ -2429,12 +2430,12 @@ int kvm_cpu_exec(CPUState *cpu)
         case KVM_EXIT_MMIO:
             DPRINTF("handle_mmio\n");
             /* Called outside BQL */
-            address_space_rw(&address_space_memory,
-                             run->mmio.phys_addr, attrs,
-                             run->mmio.data,
-                             run->mmio.len,
-                             run->mmio.is_write);
-            ret = 0;
+            res = address_space_rw(&address_space_memory,
+                                   run->mmio.phys_addr, attrs,
+                                   run->mmio.data,
+                                   run->mmio.len,
+                                   run->mmio.is_write);
+            ret = res == MEMTX_OK ? 0 : -1;
             break;
         case KVM_EXIT_IRQ_WINDOW_OPEN:
             DPRINTF("irq_window_open\n");
-- 
2.21.3


^ permalink raw reply	[flat|nested] 18+ messages in thread

* [RFC PATCH v2 7/7] hw/core/loader: Assert loading ROM regions succeeds at reset
  2020-05-18 15:53 [PATCH v2 0/7] exec/memory: Enforce checking MemTxResult values Philippe Mathieu-Daudé
                   ` (5 preceding siblings ...)
  2020-05-18 15:53 ` [RFC PATCH v2 6/7] accel/kvm: Let KVM_EXIT_MMIO return error Philippe Mathieu-Daudé
@ 2020-05-18 15:53 ` Philippe Mathieu-Daudé
  2020-05-18 16:12   ` Peter Maydell
  2020-05-21 15:40 ` [PATCH v2 0/7] exec/memory: Enforce checking MemTxResult values Paolo Bonzini
  7 siblings, 1 reply; 18+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-18 15:53 UTC (permalink / raw)
  To: qemu-devel
  Cc: Peter Maydell, Paolo Bonzini, kvm, qemu-arm, Richard Henderson,
	Philippe Mathieu-Daudé

If we are unable to load a blob in a ROM region, we should not
ignore it and let the machine boot.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
RFC: Maybe more polite with user to use hw_error()?
---
 hw/core/loader.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/hw/core/loader.c b/hw/core/loader.c
index 8bbb1797a4..4e046388b4 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -1146,8 +1146,12 @@ static void rom_reset(void *unused)
             void *host = memory_region_get_ram_ptr(rom->mr);
             memcpy(host, rom->data, rom->datasize);
         } else {
-            address_space_write_rom(rom->as, rom->addr, MEMTXATTRS_UNSPECIFIED,
-                                    rom->data, rom->datasize);
+            MemTxResult res;
+
+            res = address_space_write_rom(rom->as, rom->addr,
+                                          MEMTXATTRS_UNSPECIFIED,
+                                          rom->data, rom->datasize);
+            assert(res == MEMTX_OK);
         }
         if (rom->isrom) {
             /* rom needs to be written only once */
-- 
2.21.3


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC PATCH v2 6/7] accel/kvm: Let KVM_EXIT_MMIO return error
  2020-05-18 15:53 ` [RFC PATCH v2 6/7] accel/kvm: Let KVM_EXIT_MMIO return error Philippe Mathieu-Daudé
@ 2020-05-18 16:01   ` Peter Maydell
  2020-05-18 16:06     ` Philippe Mathieu-Daudé
  2020-05-21 15:39     ` Paolo Bonzini
  0 siblings, 2 replies; 18+ messages in thread
From: Peter Maydell @ 2020-05-18 16:01 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: QEMU Developers, Paolo Bonzini, kvm-devel, qemu-arm, Richard Henderson

On Mon, 18 May 2020 at 16:53, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Give the hypervisor a possibility to catch any error
> occuring during KVM_EXIT_MMIO.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> RFC because maybe we simply want to ignore this error instead

The "right" answer is that the kernel should enhance the KVM_EXIT_MMIO
API to allow userspace to say "sorry, you got a bus error on that
memory access the guest just tried" (which the kernel then has to
turn into an appropriate guest exception, or ignore, depending on
what the architecture requires.) You don't want to set ret to
non-zero here, because that will cause us to VM_STOP, and I
suspect that x86 at least is relying on the implict RAZ/WI
behaviour it currently gets.

thanks
-- PMM

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC PATCH v2 6/7] accel/kvm: Let KVM_EXIT_MMIO return error
  2020-05-18 16:01   ` Peter Maydell
@ 2020-05-18 16:06     ` Philippe Mathieu-Daudé
  2020-05-21 15:39     ` Paolo Bonzini
  1 sibling, 0 replies; 18+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-18 16:06 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Paolo Bonzini, qemu-arm, QEMU Developers, kvm-devel, Richard Henderson

On 5/18/20 6:01 PM, Peter Maydell wrote:
> On Mon, 18 May 2020 at 16:53, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> Give the hypervisor a possibility to catch any error
>> occuring during KVM_EXIT_MMIO.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> RFC because maybe we simply want to ignore this error instead
> 
> The "right" answer is that the kernel should enhance the KVM_EXIT_MMIO
> API to allow userspace to say "sorry, you got a bus error on that
> memory access the guest just tried" (which the kernel then has to
> turn into an appropriate guest exception, or ignore, depending on
> what the architecture requires.) You don't want to set ret to
> non-zero here, because that will cause us to VM_STOP, and I
> suspect that x86 at least is relying on the implict RAZ/WI
> behaviour it currently gets.

OK, similar to the worst case I expected here.
Thank you for the clear explanation :)

> 
> thanks
> -- PMM
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 5/7] hw/arm/boot: Abort if set_kernel_args() fails
  2020-05-18 15:53 ` [PATCH v2 5/7] hw/arm/boot: Abort if set_kernel_args() fails Philippe Mathieu-Daudé
@ 2020-05-18 16:08   ` Peter Maydell
  0 siblings, 0 replies; 18+ messages in thread
From: Peter Maydell @ 2020-05-18 16:08 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: QEMU Developers, Paolo Bonzini, kvm-devel, qemu-arm, Richard Henderson

On Mon, 18 May 2020 at 16:53, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> If a address_space_write() fails while calling
> set_kernel_args(), the guest kernel will boot
> using crap data. Avoid that by aborting if this
> ever occurs.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

I think it's reasonable to make these be fatal, but I think we
shouldn't just assert() but instead make set_kernel_arg()
and set_kernel_args_old() return a success/failure indication,
and report failures to the user as fatal errors.

thanks
-- PMM

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC PATCH v2 7/7] hw/core/loader: Assert loading ROM regions succeeds at reset
  2020-05-18 15:53 ` [RFC PATCH v2 7/7] hw/core/loader: Assert loading ROM regions succeeds at reset Philippe Mathieu-Daudé
@ 2020-05-18 16:12   ` Peter Maydell
  2020-05-18 16:25     ` Philippe Mathieu-Daudé
  0 siblings, 1 reply; 18+ messages in thread
From: Peter Maydell @ 2020-05-18 16:12 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: QEMU Developers, Paolo Bonzini, kvm-devel, qemu-arm, Richard Henderson

On Mon, 18 May 2020 at 16:53, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> If we are unable to load a blob in a ROM region, we should not
> ignore it and let the machine boot.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> RFC: Maybe more polite with user to use hw_error()?
> ---
>  hw/core/loader.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index 8bbb1797a4..4e046388b4 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -1146,8 +1146,12 @@ static void rom_reset(void *unused)
>              void *host = memory_region_get_ram_ptr(rom->mr);
>              memcpy(host, rom->data, rom->datasize);
>          } else {
> -            address_space_write_rom(rom->as, rom->addr, MEMTXATTRS_UNSPECIFIED,
> -                                    rom->data, rom->datasize);
> +            MemTxResult res;
> +
> +            res = address_space_write_rom(rom->as, rom->addr,
> +                                          MEMTXATTRS_UNSPECIFIED,
> +                                          rom->data, rom->datasize);
> +            assert(res == MEMTX_OK);

We shouln't assert(), because this is easy for a user to trigger
by loading an ELF file that's been linked to the wrong address.
Something helpful that ideally includes the name of the ELF file
and perhaps the address might be nice.

(But overall I'm a bit wary of this and other patches in the series
just because they add checks that were previously not there, and
I'm not sure whether users might be accidentally relying on
the continues-anyway behaviour.)

thanks
-- PMM

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 1/7] exec: Let address_space_read/write_cached() propagate MemTxResult
  2020-05-18 15:53 ` [PATCH v2 1/7] exec: Let address_space_read/write_cached() propagate MemTxResult Philippe Mathieu-Daudé
@ 2020-05-18 16:13   ` Peter Maydell
  0 siblings, 0 replies; 18+ messages in thread
From: Peter Maydell @ 2020-05-18 16:13 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: QEMU Developers, Paolo Bonzini, kvm-devel, qemu-arm, Richard Henderson

On Mon, 18 May 2020 at 16:53, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Both address_space_read_cached_slow() and
> address_space_write_cached_slow() return a MemTxResult type.
> Do not discard it, return it to the caller.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/exec/memory.h | 19 +++++++++++--------
>  exec.c                | 16 ++++++++--------
>  2 files changed, 19 insertions(+), 16 deletions(-)

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC PATCH v2 7/7] hw/core/loader: Assert loading ROM regions succeeds at reset
  2020-05-18 16:12   ` Peter Maydell
@ 2020-05-18 16:25     ` Philippe Mathieu-Daudé
  0 siblings, 0 replies; 18+ messages in thread
From: Philippe Mathieu-Daudé @ 2020-05-18 16:25 UTC (permalink / raw)
  To: Peter Maydell
  Cc: Paolo Bonzini, qemu-arm, QEMU Developers, kvm-devel, Richard Henderson

On 5/18/20 6:12 PM, Peter Maydell wrote:
> On Mon, 18 May 2020 at 16:53, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> If we are unable to load a blob in a ROM region, we should not
>> ignore it and let the machine boot.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>> RFC: Maybe more polite with user to use hw_error()?
>> ---
>>   hw/core/loader.c | 8 ++++++--
>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/hw/core/loader.c b/hw/core/loader.c
>> index 8bbb1797a4..4e046388b4 100644
>> --- a/hw/core/loader.c
>> +++ b/hw/core/loader.c
>> @@ -1146,8 +1146,12 @@ static void rom_reset(void *unused)
>>               void *host = memory_region_get_ram_ptr(rom->mr);
>>               memcpy(host, rom->data, rom->datasize);
>>           } else {
>> -            address_space_write_rom(rom->as, rom->addr, MEMTXATTRS_UNSPECIFIED,
>> -                                    rom->data, rom->datasize);
>> +            MemTxResult res;
>> +
>> +            res = address_space_write_rom(rom->as, rom->addr,
>> +                                          MEMTXATTRS_UNSPECIFIED,
>> +                                          rom->data, rom->datasize);
>> +            assert(res == MEMTX_OK);
> 
> We shouln't assert(), because this is easy for a user to trigger
> by loading an ELF file that's been linked to the wrong address.
> Something helpful that ideally includes the name of the ELF file
> and perhaps the address might be nice.
> 
> (But overall I'm a bit wary of this and other patches in the series
> just because they add checks that were previously not there, and
> I'm not sure whether users might be accidentally relying on
> the continues-anyway behaviour.)

I understand. Thanks for reviewing, I'll rework this one and the 
previous set_kernel_args().

> 
> thanks
> -- PMM
> 

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 4/7] hw/elf_ops: Do not ignore write failures when loading ELF
  2020-05-18 15:53 ` [PATCH v2 4/7] hw/elf_ops: Do not ignore write failures when loading ELF Philippe Mathieu-Daudé
@ 2020-05-19  7:58   ` Stefano Garzarella
  0 siblings, 0 replies; 18+ messages in thread
From: Stefano Garzarella @ 2020-05-19  7:58 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé
  Cc: qemu-devel, Peter Maydell, Paolo Bonzini, kvm, qemu-arm,
	Richard Henderson

On Mon, May 18, 2020 at 05:53:05PM +0200, Philippe Mathieu-Daudé wrote:
> Do not ignore the MemTxResult error type returned by
> address_space_write().
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/hw/elf_ops.h | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)

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

> 
> diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
> index 398a4a2c85..6fdff3dced 100644
> --- a/include/hw/elf_ops.h
> +++ b/include/hw/elf_ops.h
> @@ -553,9 +553,14 @@ static int glue(load_elf, SZ)(const char *name, int fd,
>                      rom_add_elf_program(label, mapped_file, data, file_size,
>                                          mem_size, addr, as);
>                  } else {
> -                    address_space_write(as ? as : &address_space_memory,
> -                                        addr, MEMTXATTRS_UNSPECIFIED,
> -                                        data, file_size);
> +                    MemTxResult res;
> +
> +                    res = address_space_write(as ? as : &address_space_memory,
> +                                              addr, MEMTXATTRS_UNSPECIFIED,
> +                                              data, file_size);
> +                    if (res != MEMTX_OK) {
> +                        goto fail;
> +                    }
>                  }
>              }
>  
> -- 
> 2.21.3
> 


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC PATCH v2 6/7] accel/kvm: Let KVM_EXIT_MMIO return error
  2020-05-18 16:01   ` Peter Maydell
  2020-05-18 16:06     ` Philippe Mathieu-Daudé
@ 2020-05-21 15:39     ` Paolo Bonzini
  2020-05-21 15:45       ` Peter Maydell
  1 sibling, 1 reply; 18+ messages in thread
From: Paolo Bonzini @ 2020-05-21 15:39 UTC (permalink / raw)
  To: Peter Maydell, Philippe Mathieu-Daudé
  Cc: QEMU Developers, kvm-devel, qemu-arm, Richard Henderson

On 18/05/20 18:01, Peter Maydell wrote:
> The "right" answer is that the kernel should enhance the KVM_EXIT_MMIO
> API to allow userspace to say "sorry, you got a bus error on that
> memory access the guest just tried" (which the kernel then has to
> turn into an appropriate guest exception, or ignore, depending on
> what the architecture requires.) You don't want to set ret to
> non-zero here, because that will cause us to VM_STOP, and I
> suspect that x86 at least is relying on the implict RAZ/WI
> behaviour it currently gets.

Yes, it is.  It may even be already possible to inject the right
exception (on ARM) through KVM_SET_VCPU_EVENTS or something like that, too.

Paolo


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH v2 0/7] exec/memory: Enforce checking MemTxResult values
  2020-05-18 15:53 [PATCH v2 0/7] exec/memory: Enforce checking MemTxResult values Philippe Mathieu-Daudé
                   ` (6 preceding siblings ...)
  2020-05-18 15:53 ` [RFC PATCH v2 7/7] hw/core/loader: Assert loading ROM regions succeeds at reset Philippe Mathieu-Daudé
@ 2020-05-21 15:40 ` Paolo Bonzini
  7 siblings, 0 replies; 18+ messages in thread
From: Paolo Bonzini @ 2020-05-21 15:40 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé, qemu-devel
  Cc: Peter Maydell, kvm, qemu-arm, Richard Henderson

On 18/05/20 17:53, Philippe Mathieu-Daudé wrote:
> Various places ignore the MemTxResult indicator of
> transaction failed. Fix the easy places.
> The rest are the DMA devices, which require deeper
> analysis.
> 
> Since v1:
> - Dropped "exec/memory: Emit warning when MemTxResult is ignored"
>   https://www.mail-archive.com/qemu-devel@nongnu.org/msg704180.html
> 
> Philippe Mathieu-Daudé (7):
>   exec: Let address_space_read/write_cached() propagate MemTxResult
>   exec: Propagate cpu_memory_rw_debug() error
>   disas: Let disas::read_memory() handler return EIO on error
>   hw/elf_ops: Do not ignore write failures when loading ELF
>   hw/arm/boot: Abort if set_kernel_args() fails
>   accel/kvm: Let KVM_EXIT_MMIO return error
>   hw/core/loader: Assert loading ROM regions succeeds at reset
> 
>  include/exec/cpu-all.h |  1 +
>  include/exec/memory.h  | 19 +++++++++++--------
>  include/hw/elf_ops.h   | 11 ++++++++---
>  accel/kvm/kvm-all.c    | 13 +++++++------
>  disas.c                | 13 ++++++++-----
>  exec.c                 | 28 ++++++++++++++++------------
>  hw/arm/boot.c          | 19 +++++++++++++------
>  hw/core/loader.c       |  8 ++++++--
>  8 files changed, 70 insertions(+), 42 deletions(-)
> 

Queued patches 1-4, thanks.

Paolo


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [RFC PATCH v2 6/7] accel/kvm: Let KVM_EXIT_MMIO return error
  2020-05-21 15:39     ` Paolo Bonzini
@ 2020-05-21 15:45       ` Peter Maydell
  0 siblings, 0 replies; 18+ messages in thread
From: Peter Maydell @ 2020-05-21 15:45 UTC (permalink / raw)
  To: Paolo Bonzini
  Cc: Philippe Mathieu-Daudé,
	QEMU Developers, kvm-devel, qemu-arm, Richard Henderson

On Thu, 21 May 2020 at 16:39, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
> On 18/05/20 18:01, Peter Maydell wrote:
> > The "right" answer is that the kernel should enhance the KVM_EXIT_MMIO
> > API to allow userspace to say "sorry, you got a bus error on that
> > memory access the guest just tried" (which the kernel then has to
> > turn into an appropriate guest exception, or ignore, depending on
> > what the architecture requires.) You don't want to set ret to
> > non-zero here, because that will cause us to VM_STOP, and I
> > suspect that x86 at least is relying on the implict RAZ/WI
> > behaviour it currently gets.
>
> Yes, it is.  It may even be already possible to inject the right
> exception (on ARM) through KVM_SET_VCPU_EVENTS or something like that, too.

Yeah, in theory we could deliver an exception from userspace
by updating all the register state, but I think the kernel really
ought to do it both (a) because it's just a neater API to do it
that way round and (b) because the kernel is the one that has
the info about the faulting insn that it might need for things
like setting up a syndrome register value.

thanks
-- PMM

^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, back to index

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-18 15:53 [PATCH v2 0/7] exec/memory: Enforce checking MemTxResult values Philippe Mathieu-Daudé
2020-05-18 15:53 ` [PATCH v2 1/7] exec: Let address_space_read/write_cached() propagate MemTxResult Philippe Mathieu-Daudé
2020-05-18 16:13   ` Peter Maydell
2020-05-18 15:53 ` [PATCH v2 2/7] exec: Propagate cpu_memory_rw_debug() error Philippe Mathieu-Daudé
2020-05-18 15:53 ` [PATCH v2 3/7] disas: Let disas::read_memory() handler return EIO on error Philippe Mathieu-Daudé
2020-05-18 15:53 ` [PATCH v2 4/7] hw/elf_ops: Do not ignore write failures when loading ELF Philippe Mathieu-Daudé
2020-05-19  7:58   ` Stefano Garzarella
2020-05-18 15:53 ` [PATCH v2 5/7] hw/arm/boot: Abort if set_kernel_args() fails Philippe Mathieu-Daudé
2020-05-18 16:08   ` Peter Maydell
2020-05-18 15:53 ` [RFC PATCH v2 6/7] accel/kvm: Let KVM_EXIT_MMIO return error Philippe Mathieu-Daudé
2020-05-18 16:01   ` Peter Maydell
2020-05-18 16:06     ` Philippe Mathieu-Daudé
2020-05-21 15:39     ` Paolo Bonzini
2020-05-21 15:45       ` Peter Maydell
2020-05-18 15:53 ` [RFC PATCH v2 7/7] hw/core/loader: Assert loading ROM regions succeeds at reset Philippe Mathieu-Daudé
2020-05-18 16:12   ` Peter Maydell
2020-05-18 16:25     ` Philippe Mathieu-Daudé
2020-05-21 15:40 ` [PATCH v2 0/7] exec/memory: Enforce checking MemTxResult values Paolo Bonzini

KVM Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/kvm/0 kvm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 kvm kvm/ https://lore.kernel.org/kvm \
		kvm@vger.kernel.org
	public-inbox-index kvm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.kvm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git