* [PATCH v2] generic-loader: check that binary file target location exists
@ 2021-10-26 14:03 Damien Hedde
2021-10-27 22:43 ` Alistair Francis
2021-11-01 10:53 ` Peter Maydell
0 siblings, 2 replies; 6+ messages in thread
From: Damien Hedde @ 2021-10-26 14:03 UTC (permalink / raw)
Cc: Damien Hedde, Alistair Francis, Philippe Mathieu-Daudé, qemu-devel
When loading a binary file, we only check if it is smaller than the
ram_size. It does not really check if the file will be loaded at an
existing location (if there is nothing at the target address, it will
"fail" silently later). It prevents loading a binary blob bigger than
ram_size too even if the target location is big enough.
Replace this check by looking for the target memory region size and
prevent loading a bigger file than the available space.
Get rid of "hw/boards.h" include, since we needed it only to access
`current_machine`.
Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
---
Hi,
This is an updated version implementing what we discussed in v1.
This can be tested easily, eg, using opentitan machine which has a 64K ram
located at 0x10000000.
the following works (we a blob corresponding to the whole ram)
| $ dd bs=1K count=64 if=/dev/zero of=blob.bin
| $ qemu-system-riscv32 -display none -M opentitan -device loader,addr=0x10000000,file=blob.bin
but this command fails because we load a blob which is too big
| $ dd bs=1K count=64 if=/dev/zero of=blob.bin
| $ qemu-system-riscv32 -display none -M opentitan -device loader,addr=0x10001000,file=blob.bin
| qemu-system-riscv32: -device loader,addr=0x10001000,file=blob.bin: Cannot load specified image blob.bin
and this command fails too (we load a blob at an unmapped location)
| $ dd bs=1K count=64 if=/dev/zero of=blob.bin
| $ qemu-system-riscv32 -display none -M opentitan -device loader,addr=0x0,file=blob.bin
| qemu-system-riscv32: -device loader,addr=0x0,file=blob.bin: Address 0x0 does not exists
Thanks,
Damien
v2:
+ instead of disabling the ram_size check, look for the target
v1: https://lists.nongnu.org/archive/html/qemu-devel/2021-10/msg01077.html
See also the original discussion about generic-loader:
https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg04668.html
https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg04681.html
---
hw/core/generic-loader.c | 20 +++++++++++++++++---
1 file changed, 17 insertions(+), 3 deletions(-)
diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
index d14f932eea..88d3f9fd56 100644
--- a/hw/core/generic-loader.c
+++ b/hw/core/generic-loader.c
@@ -34,7 +34,6 @@
#include "hw/core/cpu.h"
#include "sysemu/dma.h"
#include "sysemu/reset.h"
-#include "hw/boards.h"
#include "hw/loader.h"
#include "hw/qdev-properties.h"
#include "qapi/error.h"
@@ -153,8 +152,23 @@ static void generic_loader_realize(DeviceState *dev, Error **errp)
}
if (size < 0 || s->force_raw) {
- /* Default to the maximum size being the machine's ram size */
- size = load_image_targphys_as(s->file, s->addr, current_machine->ram_size, as);
+ MemoryRegion *root = as ? as->root : get_system_memory();
+ MemoryRegionSection mrs;
+ uint64_t avail = 0;
+
+ mrs = memory_region_find(root, s->addr, 1);
+
+ if (mrs.mr) {
+ avail = int128_get64(mrs.mr->size) - mrs.offset_within_region;
+ memory_region_unref(mrs.mr);
+ } else {
+ error_setg(errp, "Address 0x%" PRIx64 " does not exists",
+ s->addr);
+ return;
+ }
+
+ /* Limit the file size to the memory region space */
+ size = load_image_targphys_as(s->file, s->addr, avail, as);
} else {
s->addr = entry;
}
--
2.33.0
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v2] generic-loader: check that binary file target location exists
2021-10-26 14:03 [PATCH v2] generic-loader: check that binary file target location exists Damien Hedde
@ 2021-10-27 22:43 ` Alistair Francis
2021-11-01 10:53 ` Peter Maydell
1 sibling, 0 replies; 6+ messages in thread
From: Alistair Francis @ 2021-10-27 22:43 UTC (permalink / raw)
To: Damien Hedde
Cc: Philippe Mathieu-Daudé, qemu-devel@nongnu.org Developers
On Wed, Oct 27, 2021 at 12:03 AM Damien Hedde
<damien.hedde@greensocs.com> wrote:
>
> When loading a binary file, we only check if it is smaller than the
> ram_size. It does not really check if the file will be loaded at an
> existing location (if there is nothing at the target address, it will
> "fail" silently later). It prevents loading a binary blob bigger than
> ram_size too even if the target location is big enough.
>
> Replace this check by looking for the target memory region size and
> prevent loading a bigger file than the available space.
>
> Get rid of "hw/boards.h" include, since we needed it only to access
> `current_machine`.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
Reviewed-by: Alistair Francis <alistair.francis@wdc.com>
Alistair
> ---
>
> Hi,
>
> This is an updated version implementing what we discussed in v1.
>
> This can be tested easily, eg, using opentitan machine which has a 64K ram
> located at 0x10000000.
>
> the following works (we a blob corresponding to the whole ram)
> | $ dd bs=1K count=64 if=/dev/zero of=blob.bin
> | $ qemu-system-riscv32 -display none -M opentitan -device loader,addr=0x10000000,file=blob.bin
>
> but this command fails because we load a blob which is too big
> | $ dd bs=1K count=64 if=/dev/zero of=blob.bin
> | $ qemu-system-riscv32 -display none -M opentitan -device loader,addr=0x10001000,file=blob.bin
> | qemu-system-riscv32: -device loader,addr=0x10001000,file=blob.bin: Cannot load specified image blob.bin
>
> and this command fails too (we load a blob at an unmapped location)
> | $ dd bs=1K count=64 if=/dev/zero of=blob.bin
> | $ qemu-system-riscv32 -display none -M opentitan -device loader,addr=0x0,file=blob.bin
> | qemu-system-riscv32: -device loader,addr=0x0,file=blob.bin: Address 0x0 does not exists
>
> Thanks,
> Damien
>
> v2:
> + instead of disabling the ram_size check, look for the target
>
> v1: https://lists.nongnu.org/archive/html/qemu-devel/2021-10/msg01077.html
>
> See also the original discussion about generic-loader:
> https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg04668.html
> https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg04681.html
> ---
> hw/core/generic-loader.c | 20 +++++++++++++++++---
> 1 file changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
> index d14f932eea..88d3f9fd56 100644
> --- a/hw/core/generic-loader.c
> +++ b/hw/core/generic-loader.c
> @@ -34,7 +34,6 @@
> #include "hw/core/cpu.h"
> #include "sysemu/dma.h"
> #include "sysemu/reset.h"
> -#include "hw/boards.h"
> #include "hw/loader.h"
> #include "hw/qdev-properties.h"
> #include "qapi/error.h"
> @@ -153,8 +152,23 @@ static void generic_loader_realize(DeviceState *dev, Error **errp)
> }
>
> if (size < 0 || s->force_raw) {
> - /* Default to the maximum size being the machine's ram size */
> - size = load_image_targphys_as(s->file, s->addr, current_machine->ram_size, as);
> + MemoryRegion *root = as ? as->root : get_system_memory();
> + MemoryRegionSection mrs;
> + uint64_t avail = 0;
> +
> + mrs = memory_region_find(root, s->addr, 1);
> +
> + if (mrs.mr) {
> + avail = int128_get64(mrs.mr->size) - mrs.offset_within_region;
> + memory_region_unref(mrs.mr);
> + } else {
> + error_setg(errp, "Address 0x%" PRIx64 " does not exists",
> + s->addr);
> + return;
> + }
> +
> + /* Limit the file size to the memory region space */
> + size = load_image_targphys_as(s->file, s->addr, avail, as);
> } else {
> s->addr = entry;
> }
> --
> 2.33.0
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] generic-loader: check that binary file target location exists
2021-10-26 14:03 [PATCH v2] generic-loader: check that binary file target location exists Damien Hedde
2021-10-27 22:43 ` Alistair Francis
@ 2021-11-01 10:53 ` Peter Maydell
2021-11-02 14:04 ` Damien Hedde
1 sibling, 1 reply; 6+ messages in thread
From: Peter Maydell @ 2021-11-01 10:53 UTC (permalink / raw)
To: Damien Hedde; +Cc: Alistair Francis, Philippe Mathieu-Daudé, qemu-devel
On Tue, 26 Oct 2021 at 15:11, Damien Hedde <damien.hedde@greensocs.com> wrote:
>
> When loading a binary file, we only check if it is smaller than the
> ram_size. It does not really check if the file will be loaded at an
> existing location (if there is nothing at the target address, it will
> "fail" silently later). It prevents loading a binary blob bigger than
> ram_size too even if the target location is big enough.
>
> Replace this check by looking for the target memory region size and
> prevent loading a bigger file than the available space.
>
> Get rid of "hw/boards.h" include, since we needed it only to access
> `current_machine`.
>
> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
> ---
>
> Hi,
>
> This is an updated version implementing what we discussed in v1.
>
> This can be tested easily, eg, using opentitan machine which has a 64K ram
> located at 0x10000000.
>
> the following works (we a blob corresponding to the whole ram)
> | $ dd bs=1K count=64 if=/dev/zero of=blob.bin
> | $ qemu-system-riscv32 -display none -M opentitan -device loader,addr=0x10000000,file=blob.bin
>
> but this command fails because we load a blob which is too big
> | $ dd bs=1K count=64 if=/dev/zero of=blob.bin
> | $ qemu-system-riscv32 -display none -M opentitan -device loader,addr=0x10001000,file=blob.bin
> | qemu-system-riscv32: -device loader,addr=0x10001000,file=blob.bin: Cannot load specified image blob.bin
>
> and this command fails too (we load a blob at an unmapped location)
> | $ dd bs=1K count=64 if=/dev/zero of=blob.bin
> | $ qemu-system-riscv32 -display none -M opentitan -device loader,addr=0x0,file=blob.bin
> | qemu-system-riscv32: -device loader,addr=0x0,file=blob.bin: Address 0x0 does not exists
>
> Thanks,
> Damien
>
> v2:
> + instead of disabling the ram_size check, look for the target
>
> v1: https://lists.nongnu.org/archive/html/qemu-devel/2021-10/msg01077.html
>
> See also the original discussion about generic-loader:
> https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg04668.html
> https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg04681.html
> ---
> hw/core/generic-loader.c | 20 +++++++++++++++++---
> 1 file changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
> index d14f932eea..88d3f9fd56 100644
> --- a/hw/core/generic-loader.c
> +++ b/hw/core/generic-loader.c
> @@ -34,7 +34,6 @@
> #include "hw/core/cpu.h"
> #include "sysemu/dma.h"
> #include "sysemu/reset.h"
> -#include "hw/boards.h"
> #include "hw/loader.h"
> #include "hw/qdev-properties.h"
> #include "qapi/error.h"
> @@ -153,8 +152,23 @@ static void generic_loader_realize(DeviceState *dev, Error **errp)
> }
>
> if (size < 0 || s->force_raw) {
> - /* Default to the maximum size being the machine's ram size */
> - size = load_image_targphys_as(s->file, s->addr, current_machine->ram_size, as);
> + MemoryRegion *root = as ? as->root : get_system_memory();
> + MemoryRegionSection mrs;
> + uint64_t avail = 0;
> +
> + mrs = memory_region_find(root, s->addr, 1);
> +
> + if (mrs.mr) {
> + avail = int128_get64(mrs.mr->size) - mrs.offset_within_region;
> + memory_region_unref(mrs.mr);
> + } else {
> + error_setg(errp, "Address 0x%" PRIx64 " does not exists",
> + s->addr);
> + return;
> + }
Won't this break the case of loading a file that spans two
consecutive-but-different memory regions ? I think if we want
to catch "we tried to load something to an address not backed
by something" we should do that by making load_image_targetphys_as()
correctly handle errors from the memory accesses it makes and
propagate an error-return to the caller.
-- PMM
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] generic-loader: check that binary file target location exists
2021-11-01 10:53 ` Peter Maydell
@ 2021-11-02 14:04 ` Damien Hedde
2021-11-02 14:15 ` Peter Maydell
0 siblings, 1 reply; 6+ messages in thread
From: Damien Hedde @ 2021-11-02 14:04 UTC (permalink / raw)
To: Peter Maydell; +Cc: Alistair Francis, Philippe Mathieu-Daudé, qemu-devel
On 11/1/21 11:53, Peter Maydell wrote:
> On Tue, 26 Oct 2021 at 15:11, Damien Hedde <damien.hedde@greensocs.com> wrote:
>>
>> When loading a binary file, we only check if it is smaller than the
>> ram_size. It does not really check if the file will be loaded at an
>> existing location (if there is nothing at the target address, it will
>> "fail" silently later). It prevents loading a binary blob bigger than
>> ram_size too even if the target location is big enough.
>>
>> Replace this check by looking for the target memory region size and
>> prevent loading a bigger file than the available space.
>>
>> Get rid of "hw/boards.h" include, since we needed it only to access
>> `current_machine`.
>>
>> Signed-off-by: Damien Hedde <damien.hedde@greensocs.com>
>> ---
>>
>> Hi,
>>
>> This is an updated version implementing what we discussed in v1.
>>
>> This can be tested easily, eg, using opentitan machine which has a 64K ram
>> located at 0x10000000.
>>
>> the following works (we a blob corresponding to the whole ram)
>> | $ dd bs=1K count=64 if=/dev/zero of=blob.bin
>> | $ qemu-system-riscv32 -display none -M opentitan -device loader,addr=0x10000000,file=blob.bin
>>
>> but this command fails because we load a blob which is too big
>> | $ dd bs=1K count=64 if=/dev/zero of=blob.bin
>> | $ qemu-system-riscv32 -display none -M opentitan -device loader,addr=0x10001000,file=blob.bin
>> | qemu-system-riscv32: -device loader,addr=0x10001000,file=blob.bin: Cannot load specified image blob.bin
>>
>> and this command fails too (we load a blob at an unmapped location)
>> | $ dd bs=1K count=64 if=/dev/zero of=blob.bin
>> | $ qemu-system-riscv32 -display none -M opentitan -device loader,addr=0x0,file=blob.bin
>> | qemu-system-riscv32: -device loader,addr=0x0,file=blob.bin: Address 0x0 does not exists
>>
>> Thanks,
>> Damien
>>
>> v2:
>> + instead of disabling the ram_size check, look for the target
>>
>> v1: https://lists.nongnu.org/archive/html/qemu-devel/2021-10/msg01077.html
>>
>> See also the original discussion about generic-loader:
>> https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg04668.html
>> https://lists.gnu.org/archive/html/qemu-devel/2016-02/msg04681.html
>> ---
>> hw/core/generic-loader.c | 20 +++++++++++++++++---
>> 1 file changed, 17 insertions(+), 3 deletions(-)
>>
>> diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
>> index d14f932eea..88d3f9fd56 100644
>> --- a/hw/core/generic-loader.c
>> +++ b/hw/core/generic-loader.c
>> @@ -34,7 +34,6 @@
>> #include "hw/core/cpu.h"
>> #include "sysemu/dma.h"
>> #include "sysemu/reset.h"
>> -#include "hw/boards.h"
>> #include "hw/loader.h"
>> #include "hw/qdev-properties.h"
>> #include "qapi/error.h"
>> @@ -153,8 +152,23 @@ static void generic_loader_realize(DeviceState *dev, Error **errp)
>> }
>>
>> if (size < 0 || s->force_raw) {
>> - /* Default to the maximum size being the machine's ram size */
>> - size = load_image_targphys_as(s->file, s->addr, current_machine->ram_size, as);
>> + MemoryRegion *root = as ? as->root : get_system_memory();
>> + MemoryRegionSection mrs;
>> + uint64_t avail = 0;
>> +
>> + mrs = memory_region_find(root, s->addr, 1);
>> +
>> + if (mrs.mr) {
>> + avail = int128_get64(mrs.mr->size) - mrs.offset_within_region;
>> + memory_region_unref(mrs.mr);
>> + } else {
>> + error_setg(errp, "Address 0x%" PRIx64 " does not exists",
>> + s->addr);
>> + return;
>> + }
>
> Won't this break the case of loading a file that spans two
> consecutive-but-different memory regions ?
yes. I did not thought about that.
> I think if we want
> to catch "we tried to load something to an address not backed
> by something" we should do that by making load_image_targetphys_as()
> correctly handle errors from the memory accesses it makes and
> propagate an error-return to the caller.
>
The problem is that load_image_targetphys_as() just uses rom_load_...().
And these macros/functions only put some "rom to load" objects in a list
without doing any real accesses.
The list is checked after against overlapping between "rom to load" objects.
But the real writing is done during reset, and any bad access is
silently ignored.
In consequence:
+ it is possible for the memory structure to be populated (or even
changed) between rom_load..() calls and real writing.
+ we ignore missing parts
I do not know if we make an actual use of these "features".
I'm not sure what will be the best course of action here in order to do
the checks in rom_load_...().
Thanks,
Damien
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] generic-loader: check that binary file target location exists
2021-11-02 14:04 ` Damien Hedde
@ 2021-11-02 14:15 ` Peter Maydell
2021-11-02 14:38 ` Damien Hedde
0 siblings, 1 reply; 6+ messages in thread
From: Peter Maydell @ 2021-11-02 14:15 UTC (permalink / raw)
To: Damien Hedde; +Cc: Alistair Francis, Philippe Mathieu-Daudé, qemu-devel
On Tue, 2 Nov 2021 at 14:04, Damien Hedde <damien.hedde@greensocs.com> wrote:
>
>
>
> On 11/1/21 11:53, Peter Maydell wrote:
> > Won't this break the case of loading a file that spans two
> > consecutive-but-different memory regions ?
>
> yes. I did not thought about that.
>
> > I think if we want
> > to catch "we tried to load something to an address not backed
> > by something" we should do that by making load_image_targetphys_as()
> > correctly handle errors from the memory accesses it makes and
> > propagate an error-return to the caller.
> >
>
> The problem is that load_image_targetphys_as() just uses rom_load_...().
> And these macros/functions only put some "rom to load" objects in a list
> without doing any real accesses.
Oh yes, I'd forgotten that this was all using the rom blob
mechanism underneath.
I think in that case that whatever we do we should aim
to be consistent between the different load-image functions;
we shouldn't add a check to load_image_targetphys_as() which
we don't do in eg load_elf_as(). That probably means we should
only be looking to warn or error at the point where we actually
load the ROM blobs into memory.
-- PMM
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] generic-loader: check that binary file target location exists
2021-11-02 14:15 ` Peter Maydell
@ 2021-11-02 14:38 ` Damien Hedde
0 siblings, 0 replies; 6+ messages in thread
From: Damien Hedde @ 2021-11-02 14:38 UTC (permalink / raw)
To: Peter Maydell; +Cc: Alistair Francis, Philippe Mathieu-Daudé, qemu-devel
On 11/2/21 15:15, Peter Maydell wrote:
> On Tue, 2 Nov 2021 at 14:04, Damien Hedde <damien.hedde@greensocs.com> wrote:
>>
>>
>>
>> On 11/1/21 11:53, Peter Maydell wrote:
>>> Won't this break the case of loading a file that spans two
>>> consecutive-but-different memory regions ?
>>
>> yes. I did not thought about that.
>>
>>> I think if we want
>>> to catch "we tried to load something to an address not backed
>>> by something" we should do that by making load_image_targetphys_as()
>>> correctly handle errors from the memory accesses it makes and
>>> propagate an error-return to the caller.
>>>
>>
>> The problem is that load_image_targetphys_as() just uses rom_load_...().
>> And these macros/functions only put some "rom to load" objects in a list
>> without doing any real accesses.
>
> Oh yes, I'd forgotten that this was all using the rom blob
> mechanism underneath.
>
> I think in that case that whatever we do we should aim
> to be consistent between the different load-image functions;
> we shouldn't add a check to load_image_targetphys_as() which
> we don't do in eg load_elf_as(). That probably means we should
> only be looking to warn or error at the point where we actually
> load the ROM blobs into memory.
>
> -- PMM
>
I'll look into that.
Thanks,
Damien
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-11-02 15:02 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-10-26 14:03 [PATCH v2] generic-loader: check that binary file target location exists Damien Hedde
2021-10-27 22:43 ` Alistair Francis
2021-11-01 10:53 ` Peter Maydell
2021-11-02 14:04 ` Damien Hedde
2021-11-02 14:15 ` Peter Maydell
2021-11-02 14:38 ` Damien Hedde
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).