[PATCH v3 0/3] exec/memory: Enforce checking MemTxResult values

All of lore.kernel.org
 help / color / mirror / Atom feed

* [PATCH v3 0/3] exec/memory: Enforce checking MemTxResult values
@ 2021-05-20  5:15 Philippe Mathieu-Daudé
  2021-05-20  5:15 ` [PATCH v3 1/3] hw/arm/boot: Abort if set_kernel_args() fails Philippe Mathieu-Daudé
                   ` (2 more replies)
  0 siblings, 3 replies; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-20  5:15 UTC (permalink / raw)
  To: qemu-devel; +Cc: Peter Maydell, qemu-arm, 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 v2:
- Dropped "accel/kvm: Let KVM_EXIT_MMIO return error"
- Addressed Peter's review comments

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é (3):
  hw/arm/boot: Abort if set_kernel_args() fails
  hw/core/loader: Move 'write_rom' trace event earlier
  hw/core/loader: Warn if we fail to load ROM regions at reset

 hw/arm/boot.c    | 53 ++++++++++++++++++++++++++++++++++++++----------
 hw/core/loader.c | 15 ++++++++++----
 2 files changed, 53 insertions(+), 15 deletions(-)

-- 
2.26.3



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

* [PATCH v3 1/3] hw/arm/boot: Abort if set_kernel_args() fails
  2021-05-20  5:15 [PATCH v3 0/3] exec/memory: Enforce checking MemTxResult values Philippe Mathieu-Daudé
@ 2021-05-20  5:15 ` Philippe Mathieu-Daudé
  2021-05-27  9:04   ` Peter Maydell
  2021-05-20  5:15 ` [PATCH v3 2/3] hw/core/loader: Move 'write_rom' trace event earlier Philippe Mathieu-Daudé
  2021-05-20  5:15 ` [PATCH v3 3/3] hw/core/loader: Warn if we fail to load ROM regions at reset Philippe Mathieu-Daudé
  2 siblings, 1 reply; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-20  5:15 UTC (permalink / raw)
  To: qemu-devel; +Cc: Peter Maydell, qemu-arm, Philippe Mathieu-Daudé

If a address_space_write() call 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 | 53 ++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 42 insertions(+), 11 deletions(-)

diff --git a/hw/arm/boot.c b/hw/arm/boot.c
index d7b059225e6..0c1346d5842 100644
--- a/hw/arm/boot.c
+++ b/hw/arm/boot.c
@@ -291,15 +291,20 @@ 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); \
+    if (result != MEMTX_OK) { \
+        goto fail; \
+    } \
     p += 4;                       \
 } while (0)
 
-static void set_kernel_args(const struct arm_boot_info *info, AddressSpace *as)
+/* Returns: 0 on success, -1 on error */
+static int 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 +331,11 @@ 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);
+        if (result != MEMTX_OK) {
+            goto fail;
+        }
         cmdline_size = (cmdline_size >> 2) + 1;
         WRITE_WORD(p, cmdline_size + 2);
         WRITE_WORD(p, 0x54410009);
@@ -341,22 +349,31 @@ 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);
+        if (result != MEMTX_OK) {
+            goto fail;
+        }
         p += atag_board_len;
     }
     /* ATAG_END */
     WRITE_WORD(p, 0);
     WRITE_WORD(p, 0);
+
+    return 0;
+fail:
+    return -1;
 }
 
-static void set_kernel_args_old(const struct arm_boot_info *info,
-                                AddressSpace *as)
+/* Returns: 0 on success, -1 on error */
+static int set_kernel_args_old(const struct arm_boot_info *info,
+                               AddressSpace *as)
 {
     hwaddr p;
     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,10 +436,18 @@ 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);
+        if (result != MEMTX_OK) {
+            goto fail;
+        }
     } else {
         WRITE_WORD(p, 0);
     }
+
+    return 0;
+fail:
+    return -1;
 }
 
 static int fdt_add_memory_node(void *fdt, uint32_t acells, hwaddr mem_base,
@@ -786,10 +811,16 @@ static void do_cpu_reset(void *opaque)
                 cpu_set_pc(cs, info->loader_start);
 
                 if (!have_dtb(info)) {
+                    int err;
+
                     if (old_param) {
-                        set_kernel_args_old(info, as);
+                        err = set_kernel_args_old(info, as);
                     } else {
-                        set_kernel_args(info, as);
+                        err = set_kernel_args(info, as);
+                    }
+                    if (err) {
+                        error_report("could not set kernel arguments");
+                        exit(1);
                     }
                 }
             } else {
-- 
2.26.3



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

* Re: [PATCH v3 1/3] hw/arm/boot: Abort if set_kernel_args() fails
  2021-05-20  5:15 ` [PATCH v3 1/3] hw/arm/boot: Abort if set_kernel_args() fails Philippe Mathieu-Daudé
@ 2021-05-27  9:04   ` Peter Maydell
  0 siblings, 0 replies; 6+ messages in thread
From: Peter Maydell @ 2021-05-27  9:04 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé; +Cc: qemu-arm, QEMU Developers

On Thu, 20 May 2021 at 06:15, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> If a address_space_write() call 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>

> @@ -786,10 +811,16 @@ static void do_cpu_reset(void *opaque)
>                  cpu_set_pc(cs, info->loader_start);
>
>                  if (!have_dtb(info)) {
> +                    int err;
> +
>                      if (old_param) {
> -                        set_kernel_args_old(info, as);
> +                        err = set_kernel_args_old(info, as);
>                      } else {
> -                        set_kernel_args(info, as);
> +                        err = set_kernel_args(info, as);
> +                    }
> +                    if (err) {
> +                        error_report("could not set kernel arguments");
> +                        exit(1);
>                      }
>                  }
>              } else {

Since this is in the 'reset' method it's in theory possible that
we might end up exit()ing here in mid-run if the simulation
does a reset and the second reset fails but the one on bootup
didn't. But that seems pretty unlikely, and in any case this
code is all in the "booting Linux, but no DTB" codepath, which
is nowadays a pretty rare case.

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

thanks
-- PMM


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

* [PATCH v3 2/3] hw/core/loader: Move 'write_rom' trace event earlier
  2021-05-20  5:15 [PATCH v3 0/3] exec/memory: Enforce checking MemTxResult values Philippe Mathieu-Daudé
  2021-05-20  5:15 ` [PATCH v3 1/3] hw/arm/boot: Abort if set_kernel_args() fails Philippe Mathieu-Daudé
@ 2021-05-20  5:15 ` Philippe Mathieu-Daudé
  2021-05-20  5:15 ` [PATCH v3 3/3] hw/core/loader: Warn if we fail to load ROM regions at reset Philippe Mathieu-Daudé
  2 siblings, 0 replies; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-20  5:15 UTC (permalink / raw)
  To: qemu-devel; +Cc: Peter Maydell, qemu-arm, Philippe Mathieu-Daudé

It is more useful to trace the event which will happen,
rather than missing an event that failed. So move the
'write_rom' trace event earlier.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/core/loader.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/hw/core/loader.c b/hw/core/loader.c
index 5b34869a541..b3c4a654b45 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -1142,6 +1142,7 @@ static void rom_reset(void *unused)
         if (rom->data == NULL) {
             continue;
         }
+        trace_loader_write_rom(rom->name, rom->addr, rom->datasize, rom->isrom);
         if (rom->mr) {
             void *host = memory_region_get_ram_ptr(rom->mr);
             memcpy(host, rom->data, rom->datasize);
@@ -1160,8 +1161,6 @@ static void rom_reset(void *unused)
          * CPU definitely fetches its instructions from the just written data.
          */
         cpu_flush_icache_range(rom->addr, rom->datasize);
-
-        trace_loader_write_rom(rom->name, rom->addr, rom->datasize, rom->isrom);
     }
 }
 
-- 
2.26.3



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

* [PATCH v3 3/3] hw/core/loader: Warn if we fail to load ROM regions at reset
  2021-05-20  5:15 [PATCH v3 0/3] exec/memory: Enforce checking MemTxResult values Philippe Mathieu-Daudé
  2021-05-20  5:15 ` [PATCH v3 1/3] hw/arm/boot: Abort if set_kernel_args() fails Philippe Mathieu-Daudé
  2021-05-20  5:15 ` [PATCH v3 2/3] hw/core/loader: Move 'write_rom' trace event earlier Philippe Mathieu-Daudé
@ 2021-05-20  5:15 ` Philippe Mathieu-Daudé
  2021-05-27  9:07   ` Peter Maydell
  2 siblings, 1 reply; 6+ messages in thread
From: Philippe Mathieu-Daudé @ 2021-05-20  5:15 UTC (permalink / raw)
  To: qemu-devel; +Cc: Peter Maydell, qemu-arm, Philippe Mathieu-Daudé

If the user provides an ELF file that's been linked to a wrong
address, we try to load it, fails, and keep going silently.
Instead,
Display a warning instead, but keep going to not disrupt users
accidentally relying on this 'continues-anyway' behaviour.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 hw/core/loader.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/hw/core/loader.c b/hw/core/loader.c
index b3c4a654b45..37a2f2c4959 100644
--- a/hw/core/loader.c
+++ b/hw/core/loader.c
@@ -1147,8 +1147,16 @@ 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);
+            if (res != MEMTX_OK) {
+                warn_report("rom: unable to write data (file '%s', "
+                            "addr=0x" TARGET_FMT_plx ", size=0x%zu)",
+                            rom->name, rom->addr, rom->datasize);
+            }
         }
         if (rom->isrom) {
             /* rom needs to be written only once */
-- 
2.26.3



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

* Re: [PATCH v3 3/3] hw/core/loader: Warn if we fail to load ROM regions at reset
  2021-05-20  5:15 ` [PATCH v3 3/3] hw/core/loader: Warn if we fail to load ROM regions at reset Philippe Mathieu-Daudé
@ 2021-05-27  9:07   ` Peter Maydell
  0 siblings, 0 replies; 6+ messages in thread
From: Peter Maydell @ 2021-05-27  9:07 UTC (permalink / raw)
  To: Philippe Mathieu-Daudé; +Cc: qemu-arm, QEMU Developers

On Thu, 20 May 2021 at 06:15, Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> If the user provides an ELF file that's been linked to a wrong
> address, we try to load it, fails, and keep going silently.
> Instead,
> Display a warning instead, but keep going to not disrupt users
> accidentally relying on this 'continues-anyway' behaviour.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  hw/core/loader.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/hw/core/loader.c b/hw/core/loader.c
> index b3c4a654b45..37a2f2c4959 100644
> --- a/hw/core/loader.c
> +++ b/hw/core/loader.c
> @@ -1147,8 +1147,16 @@ 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);
> +            if (res != MEMTX_OK) {
> +                warn_report("rom: unable to write data (file '%s', "
> +                            "addr=0x" TARGET_FMT_plx ", size=0x%zu)",
> +                            rom->name, rom->addr, rom->datasize);
> +            }
>          }
>          if (rom->isrom) {
>              /* rom needs to be written only once */

address_space_write_rom() as currently implemented cannot ever
fail: it always returns MEMTX_OK. (This is because it completely
ignores attempts to write to devices, and writing to devices backed
by host RAM always works.)

But perhaps this change is reasonable enough as future-proofing
in case we decide to allow address_space_write_rom() to write
rom blobs to devices in future?

-- PMM


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

end of thread, other threads:[~2021-05-27  9:09 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-05-20  5:15 [PATCH v3 0/3] exec/memory: Enforce checking MemTxResult values Philippe Mathieu-Daudé
2021-05-20  5:15 ` [PATCH v3 1/3] hw/arm/boot: Abort if set_kernel_args() fails Philippe Mathieu-Daudé
2021-05-27  9:04   ` Peter Maydell
2021-05-20  5:15 ` [PATCH v3 2/3] hw/core/loader: Move 'write_rom' trace event earlier Philippe Mathieu-Daudé
2021-05-20  5:15 ` [PATCH v3 3/3] hw/core/loader: Warn if we fail to load ROM regions at reset Philippe Mathieu-Daudé
2021-05-27  9:07   ` Peter Maydell

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.