* [Qemu-devel] [PATCH v1 1/1] riscv: virt: Cast the initrd start address to target bit length
@ 2018-11-21 22:34 ` Alistair Francis
0 siblings, 0 replies; 18+ messages in thread
From: Alistair Francis @ 2018-11-21 22:34 UTC (permalink / raw)
To: qemu-devel, qemu-riscv; +Cc: Alistair Francis, alistair23, agraf
Ensure that the calculate initrd offset points to a valid address for
the architecture.
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
Suggested-by: Alexander Graf <agraf@suse.de>
Reported-by: Alexander Graf <agraf@suse.de>
---
hw/riscv/virt.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 2b38f89070..4467195fac 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -85,7 +85,12 @@ static hwaddr load_initrd(const char *filename, uint64_t mem_size,
* halfway into RAM, and for boards with 256MB of RAM or more we put
* the initrd at 128MB.
*/
- *start = kernel_entry + MIN(mem_size / 2, 128 * MiB);
+ /* As hwaddr is a 64-bit number we need to cast it for 32-bit */
+#if defined(TARGET_RISCV32)
+ *start = (uint32_t) (kernel_entry + MIN(mem_size / 2, 128ULL * MiB));
+#elif defined(TARGET_RISCV64)
+ *start = (uint64_t) (kernel_entry + MIN(mem_size / 2, 128 * MiB));
+#endif
size = load_ramdisk(filename, *start, mem_size - *start);
if (size == -1) {
--
2.19.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* [Qemu-riscv] [PATCH v1 1/1] riscv: virt: Cast the initrd start address to target bit length
@ 2018-11-21 22:34 ` Alistair Francis
0 siblings, 0 replies; 18+ messages in thread
From: Alistair Francis @ 2018-11-21 22:34 UTC (permalink / raw)
To: qemu-devel, qemu-riscv; +Cc: Alistair Francis, alistair23, agraf
Ensure that the calculate initrd offset points to a valid address for
the architecture.
Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
Suggested-by: Alexander Graf <agraf@suse.de>
Reported-by: Alexander Graf <agraf@suse.de>
---
hw/riscv/virt.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index 2b38f89070..4467195fac 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -85,7 +85,12 @@ static hwaddr load_initrd(const char *filename, uint64_t mem_size,
* halfway into RAM, and for boards with 256MB of RAM or more we put
* the initrd at 128MB.
*/
- *start = kernel_entry + MIN(mem_size / 2, 128 * MiB);
+ /* As hwaddr is a 64-bit number we need to cast it for 32-bit */
+#if defined(TARGET_RISCV32)
+ *start = (uint32_t) (kernel_entry + MIN(mem_size / 2, 128ULL * MiB));
+#elif defined(TARGET_RISCV64)
+ *start = (uint64_t) (kernel_entry + MIN(mem_size / 2, 128 * MiB));
+#endif
size = load_ramdisk(filename, *start, mem_size - *start);
if (size == -1) {
--
2.19.1
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v1 1/1] riscv: virt: Cast the initrd start address to target bit length
2018-11-21 22:34 ` [Qemu-riscv] " Alistair Francis
@ 2018-11-22 1:58 ` Palmer Dabbelt
-1 siblings, 0 replies; 18+ messages in thread
From: Palmer Dabbelt @ 2018-11-22 1:58 UTC (permalink / raw)
Cc: qemu-devel, qemu-riscv, alistair23, Alistair Francis, agraf
On Wed, 21 Nov 2018 14:34:44 PST (-0800), Alistair Francis wrote:
> Ensure that the calculate initrd offset points to a valid address for
> the architecture.
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> Suggested-by: Alexander Graf <agraf@suse.de>
> Reported-by: Alexander Graf <agraf@suse.de>
> ---
> hw/riscv/virt.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 2b38f89070..4467195fac 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -85,7 +85,12 @@ static hwaddr load_initrd(const char *filename, uint64_t mem_size,
> * halfway into RAM, and for boards with 256MB of RAM or more we put
> * the initrd at 128MB.
> */
> - *start = kernel_entry + MIN(mem_size / 2, 128 * MiB);
> + /* As hwaddr is a 64-bit number we need to cast it for 32-bit */
> +#if defined(TARGET_RISCV32)
> + *start = (uint32_t) (kernel_entry + MIN(mem_size / 2, 128ULL * MiB));
> +#elif defined(TARGET_RISCV64)
> + *start = (uint64_t) (kernel_entry + MIN(mem_size / 2, 128 * MiB));
> +#endif
>
> size = load_ramdisk(filename, *start, mem_size - *start);
> if (size == -1) {
> --
> 2.19.1
Maybe I'm missing something, but wouldn't something like
uint64_t start_unclobbered = kernel_entry + MIN(mem_size / 2, 128ULL * MiB);
assert(start_unclobbered == (hwaddr)start_unclobbered);
*start = (hwaddr)start_unclobbered;
work better? That should actually check this all lines up, as opposed to just
silently truncating a bad address.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-riscv] [Qemu-devel] [PATCH v1 1/1] riscv: virt: Cast the initrd start address to target bit length
@ 2018-11-22 1:58 ` Palmer Dabbelt
0 siblings, 0 replies; 18+ messages in thread
From: Palmer Dabbelt @ 2018-11-22 1:58 UTC (permalink / raw)
To: Alistair Francis
Cc: qemu-devel, qemu-riscv, alistair23, Alistair Francis, agraf
On Wed, 21 Nov 2018 14:34:44 PST (-0800), Alistair Francis wrote:
> Ensure that the calculate initrd offset points to a valid address for
> the architecture.
>
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> Suggested-by: Alexander Graf <agraf@suse.de>
> Reported-by: Alexander Graf <agraf@suse.de>
> ---
> hw/riscv/virt.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index 2b38f89070..4467195fac 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -85,7 +85,12 @@ static hwaddr load_initrd(const char *filename, uint64_t mem_size,
> * halfway into RAM, and for boards with 256MB of RAM or more we put
> * the initrd at 128MB.
> */
> - *start = kernel_entry + MIN(mem_size / 2, 128 * MiB);
> + /* As hwaddr is a 64-bit number we need to cast it for 32-bit */
> +#if defined(TARGET_RISCV32)
> + *start = (uint32_t) (kernel_entry + MIN(mem_size / 2, 128ULL * MiB));
> +#elif defined(TARGET_RISCV64)
> + *start = (uint64_t) (kernel_entry + MIN(mem_size / 2, 128 * MiB));
> +#endif
>
> size = load_ramdisk(filename, *start, mem_size - *start);
> if (size == -1) {
> --
> 2.19.1
Maybe I'm missing something, but wouldn't something like
uint64_t start_unclobbered = kernel_entry + MIN(mem_size / 2, 128ULL * MiB);
assert(start_unclobbered == (hwaddr)start_unclobbered);
*start = (hwaddr)start_unclobbered;
work better? That should actually check this all lines up, as opposed to just
silently truncating a bad address.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v1 1/1] riscv: virt: Cast the initrd start address to target bit length
2018-11-22 1:58 ` [Qemu-riscv] " Palmer Dabbelt
@ 2018-11-22 2:09 ` Alistair Francis
-1 siblings, 0 replies; 18+ messages in thread
From: Alistair Francis @ 2018-11-22 2:09 UTC (permalink / raw)
To: Palmer Dabbelt
Cc: Alistair Francis, qemu-devel@nongnu.org Developers, qemu-riscv,
Alexander Graf
On Wed, Nov 21, 2018 at 5:58 PM Palmer Dabbelt <palmer@sifive.com> wrote:
>
> On Wed, 21 Nov 2018 14:34:44 PST (-0800), Alistair Francis wrote:
> > Ensure that the calculate initrd offset points to a valid address for
> > the architecture.
> >
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > Suggested-by: Alexander Graf <agraf@suse.de>
> > Reported-by: Alexander Graf <agraf@suse.de>
> > ---
> > hw/riscv/virt.c | 7 ++++++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> > index 2b38f89070..4467195fac 100644
> > --- a/hw/riscv/virt.c
> > +++ b/hw/riscv/virt.c
> > @@ -85,7 +85,12 @@ static hwaddr load_initrd(const char *filename, uint64_t mem_size,
> > * halfway into RAM, and for boards with 256MB of RAM or more we put
> > * the initrd at 128MB.
> > */
> > - *start = kernel_entry + MIN(mem_size / 2, 128 * MiB);
> > + /* As hwaddr is a 64-bit number we need to cast it for 32-bit */
> > +#if defined(TARGET_RISCV32)
> > + *start = (uint32_t) (kernel_entry + MIN(mem_size / 2, 128ULL * MiB));
> > +#elif defined(TARGET_RISCV64)
> > + *start = (uint64_t) (kernel_entry + MIN(mem_size / 2, 128 * MiB));
> > +#endif
> >
> > size = load_ramdisk(filename, *start, mem_size - *start);
> > if (size == -1) {
> > --
> > 2.19.1
>
> Maybe I'm missing something, but wouldn't something like
>
> uint64_t start_unclobbered = kernel_entry + MIN(mem_size / 2, 128ULL * MiB);
> assert(start_unclobbered == (hwaddr)start_unclobbered);
> *start = (hwaddr)start_unclobbered;
>
> work better? That should actually check this all lines up, as opposed to just
> silently truncating a bad address.
The problem is that hwaddr is always 64-bit.
Stupidly I forgot about target_ulong, which should work basically the
same as above. An assert() seems a little harsh though, Alex reported
problem was fixed with just a cast.
Alistair
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-riscv] [Qemu-devel] [PATCH v1 1/1] riscv: virt: Cast the initrd start address to target bit length
@ 2018-11-22 2:09 ` Alistair Francis
0 siblings, 0 replies; 18+ messages in thread
From: Alistair Francis @ 2018-11-22 2:09 UTC (permalink / raw)
To: Palmer Dabbelt
Cc: Alistair Francis, qemu-devel@nongnu.org Developers, qemu-riscv,
Alexander Graf
On Wed, Nov 21, 2018 at 5:58 PM Palmer Dabbelt <palmer@sifive.com> wrote:
>
> On Wed, 21 Nov 2018 14:34:44 PST (-0800), Alistair Francis wrote:
> > Ensure that the calculate initrd offset points to a valid address for
> > the architecture.
> >
> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > Suggested-by: Alexander Graf <agraf@suse.de>
> > Reported-by: Alexander Graf <agraf@suse.de>
> > ---
> > hw/riscv/virt.c | 7 ++++++-
> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> > index 2b38f89070..4467195fac 100644
> > --- a/hw/riscv/virt.c
> > +++ b/hw/riscv/virt.c
> > @@ -85,7 +85,12 @@ static hwaddr load_initrd(const char *filename, uint64_t mem_size,
> > * halfway into RAM, and for boards with 256MB of RAM or more we put
> > * the initrd at 128MB.
> > */
> > - *start = kernel_entry + MIN(mem_size / 2, 128 * MiB);
> > + /* As hwaddr is a 64-bit number we need to cast it for 32-bit */
> > +#if defined(TARGET_RISCV32)
> > + *start = (uint32_t) (kernel_entry + MIN(mem_size / 2, 128ULL * MiB));
> > +#elif defined(TARGET_RISCV64)
> > + *start = (uint64_t) (kernel_entry + MIN(mem_size / 2, 128 * MiB));
> > +#endif
> >
> > size = load_ramdisk(filename, *start, mem_size - *start);
> > if (size == -1) {
> > --
> > 2.19.1
>
> Maybe I'm missing something, but wouldn't something like
>
> uint64_t start_unclobbered = kernel_entry + MIN(mem_size / 2, 128ULL * MiB);
> assert(start_unclobbered == (hwaddr)start_unclobbered);
> *start = (hwaddr)start_unclobbered;
>
> work better? That should actually check this all lines up, as opposed to just
> silently truncating a bad address.
The problem is that hwaddr is always 64-bit.
Stupidly I forgot about target_ulong, which should work basically the
same as above. An assert() seems a little harsh though, Alex reported
problem was fixed with just a cast.
Alistair
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v1 1/1] riscv: virt: Cast the initrd start address to target bit length
2018-11-22 2:09 ` [Qemu-riscv] " Alistair Francis
@ 2018-11-26 19:02 ` Palmer Dabbelt
-1 siblings, 0 replies; 18+ messages in thread
From: Palmer Dabbelt @ 2018-11-26 19:02 UTC (permalink / raw)
To: alistair23; +Cc: Alistair Francis, qemu-devel, qemu-riscv, agraf
On Wed, 21 Nov 2018 18:09:27 PST (-0800), alistair23@gmail.com wrote:
> On Wed, Nov 21, 2018 at 5:58 PM Palmer Dabbelt <palmer@sifive.com> wrote:
>>
>> On Wed, 21 Nov 2018 14:34:44 PST (-0800), Alistair Francis wrote:
>> > Ensure that the calculate initrd offset points to a valid address for
>> > the architecture.
>> >
>> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
>> > Suggested-by: Alexander Graf <agraf@suse.de>
>> > Reported-by: Alexander Graf <agraf@suse.de>
>> > ---
>> > hw/riscv/virt.c | 7 ++++++-
>> > 1 file changed, 6 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
>> > index 2b38f89070..4467195fac 100644
>> > --- a/hw/riscv/virt.c
>> > +++ b/hw/riscv/virt.c
>> > @@ -85,7 +85,12 @@ static hwaddr load_initrd(const char *filename, uint64_t mem_size,
>> > * halfway into RAM, and for boards with 256MB of RAM or more we put
>> > * the initrd at 128MB.
>> > */
>> > - *start = kernel_entry + MIN(mem_size / 2, 128 * MiB);
>> > + /* As hwaddr is a 64-bit number we need to cast it for 32-bit */
>> > +#if defined(TARGET_RISCV32)
>> > + *start = (uint32_t) (kernel_entry + MIN(mem_size / 2, 128ULL * MiB));
>> > +#elif defined(TARGET_RISCV64)
>> > + *start = (uint64_t) (kernel_entry + MIN(mem_size / 2, 128 * MiB));
>> > +#endif
>> >
>> > size = load_ramdisk(filename, *start, mem_size - *start);
>> > if (size == -1) {
>> > --
>> > 2.19.1
>>
>> Maybe I'm missing something, but wouldn't something like
>>
>> uint64_t start_unclobbered = kernel_entry + MIN(mem_size / 2, 128ULL * MiB);
>> assert(start_unclobbered == (hwaddr)start_unclobbered);
>> *start = (hwaddr)start_unclobbered;
>>
>> work better? That should actually check this all lines up, as opposed to just
>> silently truncating a bad address.
>
> The problem is that hwaddr is always 64-bit.
>
> Stupidly I forgot about target_ulong, which should work basically the
> same as above. An assert() seems a little harsh though, Alex reported
> problem was fixed with just a cast.
OK, I must be misunderstanding the problem, then. With the current code, isn't
it possible to end up causing start to overflow a 32-bit address? This would
happen if kernel_entry is high, with IIUC is coming from the ELF to load and is
therefor user input. I think that's worth some sort of assertion, as it'll
definitely blow up trying to enter the kernel (and possible before that,
assuming there's no target memory where it overflows into).
This won't happen with the default Linux setup, but could happen if users are
trying to do something weird.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-riscv] [Qemu-devel] [PATCH v1 1/1] riscv: virt: Cast the initrd start address to target bit length
@ 2018-11-26 19:02 ` Palmer Dabbelt
0 siblings, 0 replies; 18+ messages in thread
From: Palmer Dabbelt @ 2018-11-26 19:02 UTC (permalink / raw)
To: alistair23; +Cc: Alistair Francis, qemu-devel, qemu-riscv, agraf
On Wed, 21 Nov 2018 18:09:27 PST (-0800), alistair23@gmail.com wrote:
> On Wed, Nov 21, 2018 at 5:58 PM Palmer Dabbelt <palmer@sifive.com> wrote:
>>
>> On Wed, 21 Nov 2018 14:34:44 PST (-0800), Alistair Francis wrote:
>> > Ensure that the calculate initrd offset points to a valid address for
>> > the architecture.
>> >
>> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
>> > Suggested-by: Alexander Graf <agraf@suse.de>
>> > Reported-by: Alexander Graf <agraf@suse.de>
>> > ---
>> > hw/riscv/virt.c | 7 ++++++-
>> > 1 file changed, 6 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
>> > index 2b38f89070..4467195fac 100644
>> > --- a/hw/riscv/virt.c
>> > +++ b/hw/riscv/virt.c
>> > @@ -85,7 +85,12 @@ static hwaddr load_initrd(const char *filename, uint64_t mem_size,
>> > * halfway into RAM, and for boards with 256MB of RAM or more we put
>> > * the initrd at 128MB.
>> > */
>> > - *start = kernel_entry + MIN(mem_size / 2, 128 * MiB);
>> > + /* As hwaddr is a 64-bit number we need to cast it for 32-bit */
>> > +#if defined(TARGET_RISCV32)
>> > + *start = (uint32_t) (kernel_entry + MIN(mem_size / 2, 128ULL * MiB));
>> > +#elif defined(TARGET_RISCV64)
>> > + *start = (uint64_t) (kernel_entry + MIN(mem_size / 2, 128 * MiB));
>> > +#endif
>> >
>> > size = load_ramdisk(filename, *start, mem_size - *start);
>> > if (size == -1) {
>> > --
>> > 2.19.1
>>
>> Maybe I'm missing something, but wouldn't something like
>>
>> uint64_t start_unclobbered = kernel_entry + MIN(mem_size / 2, 128ULL * MiB);
>> assert(start_unclobbered == (hwaddr)start_unclobbered);
>> *start = (hwaddr)start_unclobbered;
>>
>> work better? That should actually check this all lines up, as opposed to just
>> silently truncating a bad address.
>
> The problem is that hwaddr is always 64-bit.
>
> Stupidly I forgot about target_ulong, which should work basically the
> same as above. An assert() seems a little harsh though, Alex reported
> problem was fixed with just a cast.
OK, I must be misunderstanding the problem, then. With the current code, isn't
it possible to end up causing start to overflow a 32-bit address? This would
happen if kernel_entry is high, with IIUC is coming from the ELF to load and is
therefor user input. I think that's worth some sort of assertion, as it'll
definitely blow up trying to enter the kernel (and possible before that,
assuming there's no target memory where it overflows into).
This won't happen with the default Linux setup, but could happen if users are
trying to do something weird.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v1 1/1] riscv: virt: Cast the initrd start address to target bit length
2018-11-26 19:02 ` [Qemu-riscv] " Palmer Dabbelt
@ 2018-11-27 22:05 ` Alistair Francis
-1 siblings, 0 replies; 18+ messages in thread
From: Alistair Francis @ 2018-11-27 22:05 UTC (permalink / raw)
To: Palmer Dabbelt
Cc: Alistair Francis, qemu-devel@nongnu.org Developers, qemu-riscv,
Alexander Graf
On Mon, Nov 26, 2018 at 11:02 AM Palmer Dabbelt <palmer@sifive.com> wrote:
>
> On Wed, 21 Nov 2018 18:09:27 PST (-0800), alistair23@gmail.com wrote:
> > On Wed, Nov 21, 2018 at 5:58 PM Palmer Dabbelt <palmer@sifive.com> wrote:
> >>
> >> On Wed, 21 Nov 2018 14:34:44 PST (-0800), Alistair Francis wrote:
> >> > Ensure that the calculate initrd offset points to a valid address for
> >> > the architecture.
> >> >
> >> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> >> > Suggested-by: Alexander Graf <agraf@suse.de>
> >> > Reported-by: Alexander Graf <agraf@suse.de>
> >> > ---
> >> > hw/riscv/virt.c | 7 ++++++-
> >> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> >> > index 2b38f89070..4467195fac 100644
> >> > --- a/hw/riscv/virt.c
> >> > +++ b/hw/riscv/virt.c
> >> > @@ -85,7 +85,12 @@ static hwaddr load_initrd(const char *filename, uint64_t mem_size,
> >> > * halfway into RAM, and for boards with 256MB of RAM or more we put
> >> > * the initrd at 128MB.
> >> > */
> >> > - *start = kernel_entry + MIN(mem_size / 2, 128 * MiB);
> >> > + /* As hwaddr is a 64-bit number we need to cast it for 32-bit */
> >> > +#if defined(TARGET_RISCV32)
> >> > + *start = (uint32_t) (kernel_entry + MIN(mem_size / 2, 128ULL * MiB));
> >> > +#elif defined(TARGET_RISCV64)
> >> > + *start = (uint64_t) (kernel_entry + MIN(mem_size / 2, 128 * MiB));
> >> > +#endif
> >> >
> >> > size = load_ramdisk(filename, *start, mem_size - *start);
> >> > if (size == -1) {
> >> > --
> >> > 2.19.1
> >>
> >> Maybe I'm missing something, but wouldn't something like
> >>
> >> uint64_t start_unclobbered = kernel_entry + MIN(mem_size / 2, 128ULL * MiB);
> >> assert(start_unclobbered == (hwaddr)start_unclobbered);
> >> *start = (hwaddr)start_unclobbered;
> >>
> >> work better? That should actually check this all lines up, as opposed to just
> >> silently truncating a bad address.
> >
> > The problem is that hwaddr is always 64-bit.
> >
> > Stupidly I forgot about target_ulong, which should work basically the
> > same as above. An assert() seems a little harsh though, Alex reported
> > problem was fixed with just a cast.
>
> OK, I must be misunderstanding the problem, then. With the current code, isn't
> it possible to end up causing start to overflow a 32-bit address? This would
> happen if kernel_entry is high, with IIUC is coming from the ELF to load and is
> therefor user input. I think that's worth some sort of assertion, as it'll
> definitely blow up trying to enter the kernel (and possible before that,
> assuming there's no target memory where it overflows into).
I don't have a setup to reproduce this unfortunately, but Alex
reported that he saw start being excessively high (0xffffffff88000000)
when loading an initrd.
It looks like the kernel entry address ends up being converted to
0xffffffff80000000 incorrectly instead of being a real overflow.
Alistair
>
> This won't happen with the default Linux setup, but could happen if users are
> trying to do something weird.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-riscv] [Qemu-devel] [PATCH v1 1/1] riscv: virt: Cast the initrd start address to target bit length
@ 2018-11-27 22:05 ` Alistair Francis
0 siblings, 0 replies; 18+ messages in thread
From: Alistair Francis @ 2018-11-27 22:05 UTC (permalink / raw)
To: Palmer Dabbelt
Cc: Alistair Francis, qemu-devel@nongnu.org Developers, qemu-riscv,
Alexander Graf
On Mon, Nov 26, 2018 at 11:02 AM Palmer Dabbelt <palmer@sifive.com> wrote:
>
> On Wed, 21 Nov 2018 18:09:27 PST (-0800), alistair23@gmail.com wrote:
> > On Wed, Nov 21, 2018 at 5:58 PM Palmer Dabbelt <palmer@sifive.com> wrote:
> >>
> >> On Wed, 21 Nov 2018 14:34:44 PST (-0800), Alistair Francis wrote:
> >> > Ensure that the calculate initrd offset points to a valid address for
> >> > the architecture.
> >> >
> >> > Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> >> > Suggested-by: Alexander Graf <agraf@suse.de>
> >> > Reported-by: Alexander Graf <agraf@suse.de>
> >> > ---
> >> > hw/riscv/virt.c | 7 ++++++-
> >> > 1 file changed, 6 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> >> > index 2b38f89070..4467195fac 100644
> >> > --- a/hw/riscv/virt.c
> >> > +++ b/hw/riscv/virt.c
> >> > @@ -85,7 +85,12 @@ static hwaddr load_initrd(const char *filename, uint64_t mem_size,
> >> > * halfway into RAM, and for boards with 256MB of RAM or more we put
> >> > * the initrd at 128MB.
> >> > */
> >> > - *start = kernel_entry + MIN(mem_size / 2, 128 * MiB);
> >> > + /* As hwaddr is a 64-bit number we need to cast it for 32-bit */
> >> > +#if defined(TARGET_RISCV32)
> >> > + *start = (uint32_t) (kernel_entry + MIN(mem_size / 2, 128ULL * MiB));
> >> > +#elif defined(TARGET_RISCV64)
> >> > + *start = (uint64_t) (kernel_entry + MIN(mem_size / 2, 128 * MiB));
> >> > +#endif
> >> >
> >> > size = load_ramdisk(filename, *start, mem_size - *start);
> >> > if (size == -1) {
> >> > --
> >> > 2.19.1
> >>
> >> Maybe I'm missing something, but wouldn't something like
> >>
> >> uint64_t start_unclobbered = kernel_entry + MIN(mem_size / 2, 128ULL * MiB);
> >> assert(start_unclobbered == (hwaddr)start_unclobbered);
> >> *start = (hwaddr)start_unclobbered;
> >>
> >> work better? That should actually check this all lines up, as opposed to just
> >> silently truncating a bad address.
> >
> > The problem is that hwaddr is always 64-bit.
> >
> > Stupidly I forgot about target_ulong, which should work basically the
> > same as above. An assert() seems a little harsh though, Alex reported
> > problem was fixed with just a cast.
>
> OK, I must be misunderstanding the problem, then. With the current code, isn't
> it possible to end up causing start to overflow a 32-bit address? This would
> happen if kernel_entry is high, with IIUC is coming from the ELF to load and is
> therefor user input. I think that's worth some sort of assertion, as it'll
> definitely blow up trying to enter the kernel (and possible before that,
> assuming there's no target memory where it overflows into).
I don't have a setup to reproduce this unfortunately, but Alex
reported that he saw start being excessively high (0xffffffff88000000)
when loading an initrd.
It looks like the kernel entry address ends up being converted to
0xffffffff80000000 incorrectly instead of being a real overflow.
Alistair
>
> This won't happen with the default Linux setup, but could happen if users are
> trying to do something weird.
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v1 1/1] riscv: virt: Cast the initrd start address to target bit length
2018-11-27 22:05 ` [Qemu-riscv] " Alistair Francis
@ 2018-11-27 22:12 ` Alexander Graf
-1 siblings, 0 replies; 18+ messages in thread
From: Alexander Graf @ 2018-11-27 22:12 UTC (permalink / raw)
To: Alistair Francis, Palmer Dabbelt
Cc: Alistair Francis, qemu-devel@nongnu.org Developers, qemu-riscv
On 27.11.18 23:05, Alistair Francis wrote:
> On Mon, Nov 26, 2018 at 11:02 AM Palmer Dabbelt <palmer@sifive.com> wrote:
>>
>> On Wed, 21 Nov 2018 18:09:27 PST (-0800), alistair23@gmail.com wrote:
>>> On Wed, Nov 21, 2018 at 5:58 PM Palmer Dabbelt <palmer@sifive.com> wrote:
>>>>
>>>> On Wed, 21 Nov 2018 14:34:44 PST (-0800), Alistair Francis wrote:
>>>>> Ensure that the calculate initrd offset points to a valid address for
>>>>> the architecture.
>>>>>
>>>>> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
>>>>> Suggested-by: Alexander Graf <agraf@suse.de>
>>>>> Reported-by: Alexander Graf <agraf@suse.de>
>>>>> ---
>>>>> hw/riscv/virt.c | 7 ++++++-
>>>>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
>>>>> index 2b38f89070..4467195fac 100644
>>>>> --- a/hw/riscv/virt.c
>>>>> +++ b/hw/riscv/virt.c
>>>>> @@ -85,7 +85,12 @@ static hwaddr load_initrd(const char *filename, uint64_t mem_size,
>>>>> * halfway into RAM, and for boards with 256MB of RAM or more we put
>>>>> * the initrd at 128MB.
>>>>> */
>>>>> - *start = kernel_entry + MIN(mem_size / 2, 128 * MiB);
>>>>> + /* As hwaddr is a 64-bit number we need to cast it for 32-bit */
>>>>> +#if defined(TARGET_RISCV32)
>>>>> + *start = (uint32_t) (kernel_entry + MIN(mem_size / 2, 128ULL * MiB));
>>>>> +#elif defined(TARGET_RISCV64)
>>>>> + *start = (uint64_t) (kernel_entry + MIN(mem_size / 2, 128 * MiB));
>>>>> +#endif
>>>>>
>>>>> size = load_ramdisk(filename, *start, mem_size - *start);
>>>>> if (size == -1) {
>>>>> --
>>>>> 2.19.1
>>>>
>>>> Maybe I'm missing something, but wouldn't something like
>>>>
>>>> uint64_t start_unclobbered = kernel_entry + MIN(mem_size / 2, 128ULL * MiB);
>>>> assert(start_unclobbered == (hwaddr)start_unclobbered);
>>>> *start = (hwaddr)start_unclobbered;
>>>>
>>>> work better? That should actually check this all lines up, as opposed to just
>>>> silently truncating a bad address.
>>>
>>> The problem is that hwaddr is always 64-bit.
>>>
>>> Stupidly I forgot about target_ulong, which should work basically the
>>> same as above. An assert() seems a little harsh though, Alex reported
>>> problem was fixed with just a cast.
>>
>> OK, I must be misunderstanding the problem, then. With the current code, isn't
>> it possible to end up causing start to overflow a 32-bit address? This would
>> happen if kernel_entry is high, with IIUC is coming from the ELF to load and is
>> therefor user input. I think that's worth some sort of assertion, as it'll
>> definitely blow up trying to enter the kernel (and possible before that,
>> assuming there's no target memory where it overflows into).
>
> I don't have a setup to reproduce this unfortunately, but Alex
> reported that he saw start being excessively high (0xffffffff88000000)
> when loading an initrd.
Sorry for only jumping in so late.
I didn't actually propose the cast as a solution. The problem must be
sign extension of some variable in the mix. I didn't find out quickly
which one it was, so I figured I'd leave it for someone else to debug :).
The issue is very easy to reproduce: Build U-Boot for the virt machine
for riscv32. Then run it with
$ qemu-system-riscv32 -M virt -kernel u-boot -nographic -initrd <a file>
You can find the initrd address with
U-Boot# fdt addr $fdtcontroladdr
U-Boot# fdt ls /chosen
Then take a peek at that address:
U-Boot# md.b <addr>
and you will see that there is nothing there without this patch. The
reason is that the binary was loaded to a negative address. Again,
probably some misguided sign extension.
> It looks like the kernel entry address ends up being converted to
> 0xffffffff80000000 incorrectly instead of being a real overflow.
Yeah, so we seem to cast from int32_t to int64_t somewhere. Can you spot
where exactly? That's where the bug is :).
Alex
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-riscv] [Qemu-devel] [PATCH v1 1/1] riscv: virt: Cast the initrd start address to target bit length
@ 2018-11-27 22:12 ` Alexander Graf
0 siblings, 0 replies; 18+ messages in thread
From: Alexander Graf @ 2018-11-27 22:12 UTC (permalink / raw)
To: Alistair Francis, Palmer Dabbelt
Cc: Alistair Francis, qemu-devel@nongnu.org Developers, qemu-riscv
On 27.11.18 23:05, Alistair Francis wrote:
> On Mon, Nov 26, 2018 at 11:02 AM Palmer Dabbelt <palmer@sifive.com> wrote:
>>
>> On Wed, 21 Nov 2018 18:09:27 PST (-0800), alistair23@gmail.com wrote:
>>> On Wed, Nov 21, 2018 at 5:58 PM Palmer Dabbelt <palmer@sifive.com> wrote:
>>>>
>>>> On Wed, 21 Nov 2018 14:34:44 PST (-0800), Alistair Francis wrote:
>>>>> Ensure that the calculate initrd offset points to a valid address for
>>>>> the architecture.
>>>>>
>>>>> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
>>>>> Suggested-by: Alexander Graf <agraf@suse.de>
>>>>> Reported-by: Alexander Graf <agraf@suse.de>
>>>>> ---
>>>>> hw/riscv/virt.c | 7 ++++++-
>>>>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
>>>>> index 2b38f89070..4467195fac 100644
>>>>> --- a/hw/riscv/virt.c
>>>>> +++ b/hw/riscv/virt.c
>>>>> @@ -85,7 +85,12 @@ static hwaddr load_initrd(const char *filename, uint64_t mem_size,
>>>>> * halfway into RAM, and for boards with 256MB of RAM or more we put
>>>>> * the initrd at 128MB.
>>>>> */
>>>>> - *start = kernel_entry + MIN(mem_size / 2, 128 * MiB);
>>>>> + /* As hwaddr is a 64-bit number we need to cast it for 32-bit */
>>>>> +#if defined(TARGET_RISCV32)
>>>>> + *start = (uint32_t) (kernel_entry + MIN(mem_size / 2, 128ULL * MiB));
>>>>> +#elif defined(TARGET_RISCV64)
>>>>> + *start = (uint64_t) (kernel_entry + MIN(mem_size / 2, 128 * MiB));
>>>>> +#endif
>>>>>
>>>>> size = load_ramdisk(filename, *start, mem_size - *start);
>>>>> if (size == -1) {
>>>>> --
>>>>> 2.19.1
>>>>
>>>> Maybe I'm missing something, but wouldn't something like
>>>>
>>>> uint64_t start_unclobbered = kernel_entry + MIN(mem_size / 2, 128ULL * MiB);
>>>> assert(start_unclobbered == (hwaddr)start_unclobbered);
>>>> *start = (hwaddr)start_unclobbered;
>>>>
>>>> work better? That should actually check this all lines up, as opposed to just
>>>> silently truncating a bad address.
>>>
>>> The problem is that hwaddr is always 64-bit.
>>>
>>> Stupidly I forgot about target_ulong, which should work basically the
>>> same as above. An assert() seems a little harsh though, Alex reported
>>> problem was fixed with just a cast.
>>
>> OK, I must be misunderstanding the problem, then. With the current code, isn't
>> it possible to end up causing start to overflow a 32-bit address? This would
>> happen if kernel_entry is high, with IIUC is coming from the ELF to load and is
>> therefor user input. I think that's worth some sort of assertion, as it'll
>> definitely blow up trying to enter the kernel (and possible before that,
>> assuming there's no target memory where it overflows into).
>
> I don't have a setup to reproduce this unfortunately, but Alex
> reported that he saw start being excessively high (0xffffffff88000000)
> when loading an initrd.
Sorry for only jumping in so late.
I didn't actually propose the cast as a solution. The problem must be
sign extension of some variable in the mix. I didn't find out quickly
which one it was, so I figured I'd leave it for someone else to debug :).
The issue is very easy to reproduce: Build U-Boot for the virt machine
for riscv32. Then run it with
$ qemu-system-riscv32 -M virt -kernel u-boot -nographic -initrd <a file>
You can find the initrd address with
U-Boot# fdt addr $fdtcontroladdr
U-Boot# fdt ls /chosen
Then take a peek at that address:
U-Boot# md.b <addr>
and you will see that there is nothing there without this patch. The
reason is that the binary was loaded to a negative address. Again,
probably some misguided sign extension.
> It looks like the kernel entry address ends up being converted to
> 0xffffffff80000000 incorrectly instead of being a real overflow.
Yeah, so we seem to cast from int32_t to int64_t somewhere. Can you spot
where exactly? That's where the bug is :).
Alex
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v1 1/1] riscv: virt: Cast the initrd start address to target bit length
2018-11-27 22:12 ` [Qemu-riscv] " Alexander Graf
@ 2018-11-27 22:24 ` Alistair Francis
-1 siblings, 0 replies; 18+ messages in thread
From: Alistair Francis @ 2018-11-27 22:24 UTC (permalink / raw)
To: Alexander Graf
Cc: Palmer Dabbelt, Alistair Francis,
qemu-devel@nongnu.org Developers, qemu-riscv
On Tue, Nov 27, 2018 at 2:12 PM Alexander Graf <agraf@suse.de> wrote:
>
>
>
> On 27.11.18 23:05, Alistair Francis wrote:
> > On Mon, Nov 26, 2018 at 11:02 AM Palmer Dabbelt <palmer@sifive.com> wrote:
> >>
> >> On Wed, 21 Nov 2018 18:09:27 PST (-0800), alistair23@gmail.com wrote:
> >>> On Wed, Nov 21, 2018 at 5:58 PM Palmer Dabbelt <palmer@sifive.com> wrote:
> >>>>
> >>>> On Wed, 21 Nov 2018 14:34:44 PST (-0800), Alistair Francis wrote:
> >>>>> Ensure that the calculate initrd offset points to a valid address for
> >>>>> the architecture.
> >>>>>
> >>>>> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> >>>>> Suggested-by: Alexander Graf <agraf@suse.de>
> >>>>> Reported-by: Alexander Graf <agraf@suse.de>
> >>>>> ---
> >>>>> hw/riscv/virt.c | 7 ++++++-
> >>>>> 1 file changed, 6 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> >>>>> index 2b38f89070..4467195fac 100644
> >>>>> --- a/hw/riscv/virt.c
> >>>>> +++ b/hw/riscv/virt.c
> >>>>> @@ -85,7 +85,12 @@ static hwaddr load_initrd(const char *filename, uint64_t mem_size,
> >>>>> * halfway into RAM, and for boards with 256MB of RAM or more we put
> >>>>> * the initrd at 128MB.
> >>>>> */
> >>>>> - *start = kernel_entry + MIN(mem_size / 2, 128 * MiB);
> >>>>> + /* As hwaddr is a 64-bit number we need to cast it for 32-bit */
> >>>>> +#if defined(TARGET_RISCV32)
> >>>>> + *start = (uint32_t) (kernel_entry + MIN(mem_size / 2, 128ULL * MiB));
> >>>>> +#elif defined(TARGET_RISCV64)
> >>>>> + *start = (uint64_t) (kernel_entry + MIN(mem_size / 2, 128 * MiB));
> >>>>> +#endif
> >>>>>
> >>>>> size = load_ramdisk(filename, *start, mem_size - *start);
> >>>>> if (size == -1) {
> >>>>> --
> >>>>> 2.19.1
> >>>>
> >>>> Maybe I'm missing something, but wouldn't something like
> >>>>
> >>>> uint64_t start_unclobbered = kernel_entry + MIN(mem_size / 2, 128ULL * MiB);
> >>>> assert(start_unclobbered == (hwaddr)start_unclobbered);
> >>>> *start = (hwaddr)start_unclobbered;
> >>>>
> >>>> work better? That should actually check this all lines up, as opposed to just
> >>>> silently truncating a bad address.
> >>>
> >>> The problem is that hwaddr is always 64-bit.
> >>>
> >>> Stupidly I forgot about target_ulong, which should work basically the
> >>> same as above. An assert() seems a little harsh though, Alex reported
> >>> problem was fixed with just a cast.
> >>
> >> OK, I must be misunderstanding the problem, then. With the current code, isn't
> >> it possible to end up causing start to overflow a 32-bit address? This would
> >> happen if kernel_entry is high, with IIUC is coming from the ELF to load and is
> >> therefor user input. I think that's worth some sort of assertion, as it'll
> >> definitely blow up trying to enter the kernel (and possible before that,
> >> assuming there's no target memory where it overflows into).
> >
> > I don't have a setup to reproduce this unfortunately, but Alex
> > reported that he saw start being excessively high (0xffffffff88000000)
> > when loading an initrd.
>
> Sorry for only jumping in so late.
>
> I didn't actually propose the cast as a solution. The problem must be
> sign extension of some variable in the mix. I didn't find out quickly
> which one it was, so I figured I'd leave it for someone else to debug :).
>
> The issue is very easy to reproduce: Build U-Boot for the virt machine
> for riscv32. Then run it with
>
> $ qemu-system-riscv32 -M virt -kernel u-boot -nographic -initrd <a file>
Ah, cool I can reproduce it now.
It happens in load_elf32() (which is actually glue(load_elf, SZ)).
This line ends up sign extending the value:
*pentry = (uint64_t)(elf_sword)ehdr.e_entry;
and we just continue it from there.
So I think this diff is a much better solution:
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index e7f0716fb6..348ecc39d5 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -62,7 +62,7 @@ static const struct MemmapEntry {
[VIRT_PCIE_ECAM] = { 0x30000000, 0x10000000 },
};
-static uint64_t load_kernel(const char *kernel_filename)
+static target_ulong load_kernel(const char *kernel_filename)
{
uint64_t kernel_entry, kernel_high;
Alistair
>
> You can find the initrd address with
>
> U-Boot# fdt addr $fdtcontroladdr
> U-Boot# fdt ls /chosen
>
> Then take a peek at that address:
>
> U-Boot# md.b <addr>
>
> and you will see that there is nothing there without this patch. The
> reason is that the binary was loaded to a negative address. Again,
> probably some misguided sign extension.
>
> > It looks like the kernel entry address ends up being converted to
> > 0xffffffff80000000 incorrectly instead of being a real overflow.
>
> Yeah, so we seem to cast from int32_t to int64_t somewhere. Can you spot
> where exactly? That's where the bug is :).
>
>
> Alex
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [Qemu-riscv] [Qemu-devel] [PATCH v1 1/1] riscv: virt: Cast the initrd start address to target bit length
@ 2018-11-27 22:24 ` Alistair Francis
0 siblings, 0 replies; 18+ messages in thread
From: Alistair Francis @ 2018-11-27 22:24 UTC (permalink / raw)
To: Alexander Graf
Cc: Palmer Dabbelt, Alistair Francis,
qemu-devel@nongnu.org Developers, qemu-riscv
On Tue, Nov 27, 2018 at 2:12 PM Alexander Graf <agraf@suse.de> wrote:
>
>
>
> On 27.11.18 23:05, Alistair Francis wrote:
> > On Mon, Nov 26, 2018 at 11:02 AM Palmer Dabbelt <palmer@sifive.com> wrote:
> >>
> >> On Wed, 21 Nov 2018 18:09:27 PST (-0800), alistair23@gmail.com wrote:
> >>> On Wed, Nov 21, 2018 at 5:58 PM Palmer Dabbelt <palmer@sifive.com> wrote:
> >>>>
> >>>> On Wed, 21 Nov 2018 14:34:44 PST (-0800), Alistair Francis wrote:
> >>>>> Ensure that the calculate initrd offset points to a valid address for
> >>>>> the architecture.
> >>>>>
> >>>>> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> >>>>> Suggested-by: Alexander Graf <agraf@suse.de>
> >>>>> Reported-by: Alexander Graf <agraf@suse.de>
> >>>>> ---
> >>>>> hw/riscv/virt.c | 7 ++++++-
> >>>>> 1 file changed, 6 insertions(+), 1 deletion(-)
> >>>>>
> >>>>> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> >>>>> index 2b38f89070..4467195fac 100644
> >>>>> --- a/hw/riscv/virt.c
> >>>>> +++ b/hw/riscv/virt.c
> >>>>> @@ -85,7 +85,12 @@ static hwaddr load_initrd(const char *filename, uint64_t mem_size,
> >>>>> * halfway into RAM, and for boards with 256MB of RAM or more we put
> >>>>> * the initrd at 128MB.
> >>>>> */
> >>>>> - *start = kernel_entry + MIN(mem_size / 2, 128 * MiB);
> >>>>> + /* As hwaddr is a 64-bit number we need to cast it for 32-bit */
> >>>>> +#if defined(TARGET_RISCV32)
> >>>>> + *start = (uint32_t) (kernel_entry + MIN(mem_size / 2, 128ULL * MiB));
> >>>>> +#elif defined(TARGET_RISCV64)
> >>>>> + *start = (uint64_t) (kernel_entry + MIN(mem_size / 2, 128 * MiB));
> >>>>> +#endif
> >>>>>
> >>>>> size = load_ramdisk(filename, *start, mem_size - *start);
> >>>>> if (size == -1) {
> >>>>> --
> >>>>> 2.19.1
> >>>>
> >>>> Maybe I'm missing something, but wouldn't something like
> >>>>
> >>>> uint64_t start_unclobbered = kernel_entry + MIN(mem_size / 2, 128ULL * MiB);
> >>>> assert(start_unclobbered == (hwaddr)start_unclobbered);
> >>>> *start = (hwaddr)start_unclobbered;
> >>>>
> >>>> work better? That should actually check this all lines up, as opposed to just
> >>>> silently truncating a bad address.
> >>>
> >>> The problem is that hwaddr is always 64-bit.
> >>>
> >>> Stupidly I forgot about target_ulong, which should work basically the
> >>> same as above. An assert() seems a little harsh though, Alex reported
> >>> problem was fixed with just a cast.
> >>
> >> OK, I must be misunderstanding the problem, then. With the current code, isn't
> >> it possible to end up causing start to overflow a 32-bit address? This would
> >> happen if kernel_entry is high, with IIUC is coming from the ELF to load and is
> >> therefor user input. I think that's worth some sort of assertion, as it'll
> >> definitely blow up trying to enter the kernel (and possible before that,
> >> assuming there's no target memory where it overflows into).
> >
> > I don't have a setup to reproduce this unfortunately, but Alex
> > reported that he saw start being excessively high (0xffffffff88000000)
> > when loading an initrd.
>
> Sorry for only jumping in so late.
>
> I didn't actually propose the cast as a solution. The problem must be
> sign extension of some variable in the mix. I didn't find out quickly
> which one it was, so I figured I'd leave it for someone else to debug :).
>
> The issue is very easy to reproduce: Build U-Boot for the virt machine
> for riscv32. Then run it with
>
> $ qemu-system-riscv32 -M virt -kernel u-boot -nographic -initrd <a file>
Ah, cool I can reproduce it now.
It happens in load_elf32() (which is actually glue(load_elf, SZ)).
This line ends up sign extending the value:
*pentry = (uint64_t)(elf_sword)ehdr.e_entry;
and we just continue it from there.
So I think this diff is a much better solution:
diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
index e7f0716fb6..348ecc39d5 100644
--- a/hw/riscv/virt.c
+++ b/hw/riscv/virt.c
@@ -62,7 +62,7 @@ static const struct MemmapEntry {
[VIRT_PCIE_ECAM] = { 0x30000000, 0x10000000 },
};
-static uint64_t load_kernel(const char *kernel_filename)
+static target_ulong load_kernel(const char *kernel_filename)
{
uint64_t kernel_entry, kernel_high;
Alistair
>
> You can find the initrd address with
>
> U-Boot# fdt addr $fdtcontroladdr
> U-Boot# fdt ls /chosen
>
> Then take a peek at that address:
>
> U-Boot# md.b <addr>
>
> and you will see that there is nothing there without this patch. The
> reason is that the binary was loaded to a negative address. Again,
> probably some misguided sign extension.
>
> > It looks like the kernel entry address ends up being converted to
> > 0xffffffff80000000 incorrectly instead of being a real overflow.
>
> Yeah, so we seem to cast from int32_t to int64_t somewhere. Can you spot
> where exactly? That's where the bug is :).
>
>
> Alex
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v1 1/1] riscv: virt: Cast the initrd start address to target bit length
2018-11-27 22:24 ` [Qemu-riscv] " Alistair Francis
@ 2018-11-27 22:28 ` Alistair Francis
-1 siblings, 0 replies; 18+ messages in thread
From: Alistair Francis @ 2018-11-27 22:28 UTC (permalink / raw)
To: Alexander Graf
Cc: Palmer Dabbelt, Alistair Francis,
qemu-devel@nongnu.org Developers, qemu-riscv
On Tue, Nov 27, 2018 at 2:24 PM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Tue, Nov 27, 2018 at 2:12 PM Alexander Graf <agraf@suse.de> wrote:
> >
> >
> >
> > On 27.11.18 23:05, Alistair Francis wrote:
> > > On Mon, Nov 26, 2018 at 11:02 AM Palmer Dabbelt <palmer@sifive.com> wrote:
> > >>
> > >> On Wed, 21 Nov 2018 18:09:27 PST (-0800), alistair23@gmail.com wrote:
> > >>> On Wed, Nov 21, 2018 at 5:58 PM Palmer Dabbelt <palmer@sifive.com> wrote:
> > >>>>
> > >>>> On Wed, 21 Nov 2018 14:34:44 PST (-0800), Alistair Francis wrote:
> > >>>>> Ensure that the calculate initrd offset points to a valid address for
> > >>>>> the architecture.
> > >>>>>
> > >>>>> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > >>>>> Suggested-by: Alexander Graf <agraf@suse.de>
> > >>>>> Reported-by: Alexander Graf <agraf@suse.de>
> > >>>>> ---
> > >>>>> hw/riscv/virt.c | 7 ++++++-
> > >>>>> 1 file changed, 6 insertions(+), 1 deletion(-)
> > >>>>>
> > >>>>> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> > >>>>> index 2b38f89070..4467195fac 100644
> > >>>>> --- a/hw/riscv/virt.c
> > >>>>> +++ b/hw/riscv/virt.c
> > >>>>> @@ -85,7 +85,12 @@ static hwaddr load_initrd(const char *filename, uint64_t mem_size,
> > >>>>> * halfway into RAM, and for boards with 256MB of RAM or more we put
> > >>>>> * the initrd at 128MB.
> > >>>>> */
> > >>>>> - *start = kernel_entry + MIN(mem_size / 2, 128 * MiB);
> > >>>>> + /* As hwaddr is a 64-bit number we need to cast it for 32-bit */
> > >>>>> +#if defined(TARGET_RISCV32)
> > >>>>> + *start = (uint32_t) (kernel_entry + MIN(mem_size / 2, 128ULL * MiB));
> > >>>>> +#elif defined(TARGET_RISCV64)
> > >>>>> + *start = (uint64_t) (kernel_entry + MIN(mem_size / 2, 128 * MiB));
> > >>>>> +#endif
> > >>>>>
> > >>>>> size = load_ramdisk(filename, *start, mem_size - *start);
> > >>>>> if (size == -1) {
> > >>>>> --
> > >>>>> 2.19.1
> > >>>>
> > >>>> Maybe I'm missing something, but wouldn't something like
> > >>>>
> > >>>> uint64_t start_unclobbered = kernel_entry + MIN(mem_size / 2, 128ULL * MiB);
> > >>>> assert(start_unclobbered == (hwaddr)start_unclobbered);
> > >>>> *start = (hwaddr)start_unclobbered;
> > >>>>
> > >>>> work better? That should actually check this all lines up, as opposed to just
> > >>>> silently truncating a bad address.
> > >>>
> > >>> The problem is that hwaddr is always 64-bit.
> > >>>
> > >>> Stupidly I forgot about target_ulong, which should work basically the
> > >>> same as above. An assert() seems a little harsh though, Alex reported
> > >>> problem was fixed with just a cast.
> > >>
> > >> OK, I must be misunderstanding the problem, then. With the current code, isn't
> > >> it possible to end up causing start to overflow a 32-bit address? This would
> > >> happen if kernel_entry is high, with IIUC is coming from the ELF to load and is
> > >> therefor user input. I think that's worth some sort of assertion, as it'll
> > >> definitely blow up trying to enter the kernel (and possible before that,
> > >> assuming there's no target memory where it overflows into).
> > >
> > > I don't have a setup to reproduce this unfortunately, but Alex
> > > reported that he saw start being excessively high (0xffffffff88000000)
> > > when loading an initrd.
> >
> > Sorry for only jumping in so late.
> >
> > I didn't actually propose the cast as a solution. The problem must be
> > sign extension of some variable in the mix. I didn't find out quickly
> > which one it was, so I figured I'd leave it for someone else to debug :).
> >
> > The issue is very easy to reproduce: Build U-Boot for the virt machine
> > for riscv32. Then run it with
> >
> > $ qemu-system-riscv32 -M virt -kernel u-boot -nographic -initrd <a file>
>
> Ah, cool I can reproduce it now.
>
> It happens in load_elf32() (which is actually glue(load_elf, SZ)).
>
> This line ends up sign extending the value:
> *pentry = (uint64_t)(elf_sword)ehdr.e_entry;
>
> and we just continue it from there.
>
> So I think this diff is a much better solution:
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index e7f0716fb6..348ecc39d5 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -62,7 +62,7 @@ static const struct MemmapEntry {
> [VIRT_PCIE_ECAM] = { 0x30000000, 0x10000000 },
> };
>
> -static uint64_t load_kernel(const char *kernel_filename)
> +static target_ulong load_kernel(const char *kernel_filename)
> {
> uint64_t kernel_entry, kernel_high;
>
>
>
> Alistair
>
> >
> > You can find the initrd address with
> >
> > U-Boot# fdt addr $fdtcontroladdr
> > U-Boot# fdt ls /chosen
> >
> > Then take a peek at that address:
> >
> > U-Boot# md.b <addr>
> >
> > and you will see that there is nothing there without this patch. The
> > reason is that the binary was loaded to a negative address. Again,
> > probably some misguided sign extension.
> >
> > > It looks like the kernel entry address ends up being converted to
> > > 0xffffffff80000000 incorrectly instead of being a real overflow.
> >
> > Yeah, so we seem to cast from int32_t to int64_t somewhere. Can you spot
> > where exactly? That's where the bug is :).
Actually this diff looks like a better more generic fix:
diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
index 81cecaf27e..7c1930e7a3 100644
--- a/include/hw/elf_ops.h
+++ b/include/hw/elf_ops.h
@@ -342,8 +342,9 @@ static int glue(load_elf, SZ)(const char *name, int fd,
}
}
- if (pentry)
- *pentry = (uint64_t)(elf_sword)ehdr.e_entry;
+ if (pentry) {
+ *pentry = (uint64_t)(elf_word)ehdr.e_entry;
+ }
glue(load_symbols, SZ)(&ehdr, fd, must_swab, clear_lsb, sym_cb);
My only concern is that it will break some other 32-bit guest. It
seems odd that no one else has seen this before.
Alistair
> >
> >
> > Alex
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [Qemu-riscv] [Qemu-devel] [PATCH v1 1/1] riscv: virt: Cast the initrd start address to target bit length
@ 2018-11-27 22:28 ` Alistair Francis
0 siblings, 0 replies; 18+ messages in thread
From: Alistair Francis @ 2018-11-27 22:28 UTC (permalink / raw)
To: Alexander Graf
Cc: Palmer Dabbelt, Alistair Francis,
qemu-devel@nongnu.org Developers, qemu-riscv
On Tue, Nov 27, 2018 at 2:24 PM Alistair Francis <alistair23@gmail.com> wrote:
>
> On Tue, Nov 27, 2018 at 2:12 PM Alexander Graf <agraf@suse.de> wrote:
> >
> >
> >
> > On 27.11.18 23:05, Alistair Francis wrote:
> > > On Mon, Nov 26, 2018 at 11:02 AM Palmer Dabbelt <palmer@sifive.com> wrote:
> > >>
> > >> On Wed, 21 Nov 2018 18:09:27 PST (-0800), alistair23@gmail.com wrote:
> > >>> On Wed, Nov 21, 2018 at 5:58 PM Palmer Dabbelt <palmer@sifive.com> wrote:
> > >>>>
> > >>>> On Wed, 21 Nov 2018 14:34:44 PST (-0800), Alistair Francis wrote:
> > >>>>> Ensure that the calculate initrd offset points to a valid address for
> > >>>>> the architecture.
> > >>>>>
> > >>>>> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> > >>>>> Suggested-by: Alexander Graf <agraf@suse.de>
> > >>>>> Reported-by: Alexander Graf <agraf@suse.de>
> > >>>>> ---
> > >>>>> hw/riscv/virt.c | 7 ++++++-
> > >>>>> 1 file changed, 6 insertions(+), 1 deletion(-)
> > >>>>>
> > >>>>> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> > >>>>> index 2b38f89070..4467195fac 100644
> > >>>>> --- a/hw/riscv/virt.c
> > >>>>> +++ b/hw/riscv/virt.c
> > >>>>> @@ -85,7 +85,12 @@ static hwaddr load_initrd(const char *filename, uint64_t mem_size,
> > >>>>> * halfway into RAM, and for boards with 256MB of RAM or more we put
> > >>>>> * the initrd at 128MB.
> > >>>>> */
> > >>>>> - *start = kernel_entry + MIN(mem_size / 2, 128 * MiB);
> > >>>>> + /* As hwaddr is a 64-bit number we need to cast it for 32-bit */
> > >>>>> +#if defined(TARGET_RISCV32)
> > >>>>> + *start = (uint32_t) (kernel_entry + MIN(mem_size / 2, 128ULL * MiB));
> > >>>>> +#elif defined(TARGET_RISCV64)
> > >>>>> + *start = (uint64_t) (kernel_entry + MIN(mem_size / 2, 128 * MiB));
> > >>>>> +#endif
> > >>>>>
> > >>>>> size = load_ramdisk(filename, *start, mem_size - *start);
> > >>>>> if (size == -1) {
> > >>>>> --
> > >>>>> 2.19.1
> > >>>>
> > >>>> Maybe I'm missing something, but wouldn't something like
> > >>>>
> > >>>> uint64_t start_unclobbered = kernel_entry + MIN(mem_size / 2, 128ULL * MiB);
> > >>>> assert(start_unclobbered == (hwaddr)start_unclobbered);
> > >>>> *start = (hwaddr)start_unclobbered;
> > >>>>
> > >>>> work better? That should actually check this all lines up, as opposed to just
> > >>>> silently truncating a bad address.
> > >>>
> > >>> The problem is that hwaddr is always 64-bit.
> > >>>
> > >>> Stupidly I forgot about target_ulong, which should work basically the
> > >>> same as above. An assert() seems a little harsh though, Alex reported
> > >>> problem was fixed with just a cast.
> > >>
> > >> OK, I must be misunderstanding the problem, then. With the current code, isn't
> > >> it possible to end up causing start to overflow a 32-bit address? This would
> > >> happen if kernel_entry is high, with IIUC is coming from the ELF to load and is
> > >> therefor user input. I think that's worth some sort of assertion, as it'll
> > >> definitely blow up trying to enter the kernel (and possible before that,
> > >> assuming there's no target memory where it overflows into).
> > >
> > > I don't have a setup to reproduce this unfortunately, but Alex
> > > reported that he saw start being excessively high (0xffffffff88000000)
> > > when loading an initrd.
> >
> > Sorry for only jumping in so late.
> >
> > I didn't actually propose the cast as a solution. The problem must be
> > sign extension of some variable in the mix. I didn't find out quickly
> > which one it was, so I figured I'd leave it for someone else to debug :).
> >
> > The issue is very easy to reproduce: Build U-Boot for the virt machine
> > for riscv32. Then run it with
> >
> > $ qemu-system-riscv32 -M virt -kernel u-boot -nographic -initrd <a file>
>
> Ah, cool I can reproduce it now.
>
> It happens in load_elf32() (which is actually glue(load_elf, SZ)).
>
> This line ends up sign extending the value:
> *pentry = (uint64_t)(elf_sword)ehdr.e_entry;
>
> and we just continue it from there.
>
> So I think this diff is a much better solution:
>
> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
> index e7f0716fb6..348ecc39d5 100644
> --- a/hw/riscv/virt.c
> +++ b/hw/riscv/virt.c
> @@ -62,7 +62,7 @@ static const struct MemmapEntry {
> [VIRT_PCIE_ECAM] = { 0x30000000, 0x10000000 },
> };
>
> -static uint64_t load_kernel(const char *kernel_filename)
> +static target_ulong load_kernel(const char *kernel_filename)
> {
> uint64_t kernel_entry, kernel_high;
>
>
>
> Alistair
>
> >
> > You can find the initrd address with
> >
> > U-Boot# fdt addr $fdtcontroladdr
> > U-Boot# fdt ls /chosen
> >
> > Then take a peek at that address:
> >
> > U-Boot# md.b <addr>
> >
> > and you will see that there is nothing there without this patch. The
> > reason is that the binary was loaded to a negative address. Again,
> > probably some misguided sign extension.
> >
> > > It looks like the kernel entry address ends up being converted to
> > > 0xffffffff80000000 incorrectly instead of being a real overflow.
> >
> > Yeah, so we seem to cast from int32_t to int64_t somewhere. Can you spot
> > where exactly? That's where the bug is :).
Actually this diff looks like a better more generic fix:
diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
index 81cecaf27e..7c1930e7a3 100644
--- a/include/hw/elf_ops.h
+++ b/include/hw/elf_ops.h
@@ -342,8 +342,9 @@ static int glue(load_elf, SZ)(const char *name, int fd,
}
}
- if (pentry)
- *pentry = (uint64_t)(elf_sword)ehdr.e_entry;
+ if (pentry) {
+ *pentry = (uint64_t)(elf_word)ehdr.e_entry;
+ }
glue(load_symbols, SZ)(&ehdr, fd, must_swab, clear_lsb, sym_cb);
My only concern is that it will break some other 32-bit guest. It
seems odd that no one else has seen this before.
Alistair
> >
> >
> > Alex
^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: [Qemu-devel] [PATCH v1 1/1] riscv: virt: Cast the initrd start address to target bit length
2018-11-27 22:28 ` [Qemu-riscv] " Alistair Francis
@ 2018-11-27 22:35 ` Alexander Graf
-1 siblings, 0 replies; 18+ messages in thread
From: Alexander Graf @ 2018-11-27 22:35 UTC (permalink / raw)
To: Alistair Francis
Cc: Palmer Dabbelt, Alistair Francis,
qemu-devel@nongnu.org Developers, qemu-riscv
On 27.11.18 23:28, Alistair Francis wrote:
> On Tue, Nov 27, 2018 at 2:24 PM Alistair Francis <alistair23@gmail.com> wrote:
>>
>> On Tue, Nov 27, 2018 at 2:12 PM Alexander Graf <agraf@suse.de> wrote:
>>>
>>>
>>>
>>> On 27.11.18 23:05, Alistair Francis wrote:
>>>> On Mon, Nov 26, 2018 at 11:02 AM Palmer Dabbelt <palmer@sifive.com> wrote:
>>>>>
>>>>> On Wed, 21 Nov 2018 18:09:27 PST (-0800), alistair23@gmail.com wrote:
>>>>>> On Wed, Nov 21, 2018 at 5:58 PM Palmer Dabbelt <palmer@sifive.com> wrote:
>>>>>>>
>>>>>>> On Wed, 21 Nov 2018 14:34:44 PST (-0800), Alistair Francis wrote:
>>>>>>>> Ensure that the calculate initrd offset points to a valid address for
>>>>>>>> the architecture.
>>>>>>>>
>>>>>>>> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
>>>>>>>> Suggested-by: Alexander Graf <agraf@suse.de>
>>>>>>>> Reported-by: Alexander Graf <agraf@suse.de>
>>>>>>>> ---
>>>>>>>> hw/riscv/virt.c | 7 ++++++-
>>>>>>>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
>>>>>>>> index 2b38f89070..4467195fac 100644
>>>>>>>> --- a/hw/riscv/virt.c
>>>>>>>> +++ b/hw/riscv/virt.c
>>>>>>>> @@ -85,7 +85,12 @@ static hwaddr load_initrd(const char *filename, uint64_t mem_size,
>>>>>>>> * halfway into RAM, and for boards with 256MB of RAM or more we put
>>>>>>>> * the initrd at 128MB.
>>>>>>>> */
>>>>>>>> - *start = kernel_entry + MIN(mem_size / 2, 128 * MiB);
>>>>>>>> + /* As hwaddr is a 64-bit number we need to cast it for 32-bit */
>>>>>>>> +#if defined(TARGET_RISCV32)
>>>>>>>> + *start = (uint32_t) (kernel_entry + MIN(mem_size / 2, 128ULL * MiB));
>>>>>>>> +#elif defined(TARGET_RISCV64)
>>>>>>>> + *start = (uint64_t) (kernel_entry + MIN(mem_size / 2, 128 * MiB));
>>>>>>>> +#endif
>>>>>>>>
>>>>>>>> size = load_ramdisk(filename, *start, mem_size - *start);
>>>>>>>> if (size == -1) {
>>>>>>>> --
>>>>>>>> 2.19.1
>>>>>>>
>>>>>>> Maybe I'm missing something, but wouldn't something like
>>>>>>>
>>>>>>> uint64_t start_unclobbered = kernel_entry + MIN(mem_size / 2, 128ULL * MiB);
>>>>>>> assert(start_unclobbered == (hwaddr)start_unclobbered);
>>>>>>> *start = (hwaddr)start_unclobbered;
>>>>>>>
>>>>>>> work better? That should actually check this all lines up, as opposed to just
>>>>>>> silently truncating a bad address.
>>>>>>
>>>>>> The problem is that hwaddr is always 64-bit.
>>>>>>
>>>>>> Stupidly I forgot about target_ulong, which should work basically the
>>>>>> same as above. An assert() seems a little harsh though, Alex reported
>>>>>> problem was fixed with just a cast.
>>>>>
>>>>> OK, I must be misunderstanding the problem, then. With the current code, isn't
>>>>> it possible to end up causing start to overflow a 32-bit address? This would
>>>>> happen if kernel_entry is high, with IIUC is coming from the ELF to load and is
>>>>> therefor user input. I think that's worth some sort of assertion, as it'll
>>>>> definitely blow up trying to enter the kernel (and possible before that,
>>>>> assuming there's no target memory where it overflows into).
>>>>
>>>> I don't have a setup to reproduce this unfortunately, but Alex
>>>> reported that he saw start being excessively high (0xffffffff88000000)
>>>> when loading an initrd.
>>>
>>> Sorry for only jumping in so late.
>>>
>>> I didn't actually propose the cast as a solution. The problem must be
>>> sign extension of some variable in the mix. I didn't find out quickly
>>> which one it was, so I figured I'd leave it for someone else to debug :).
>>>
>>> The issue is very easy to reproduce: Build U-Boot for the virt machine
>>> for riscv32. Then run it with
>>>
>>> $ qemu-system-riscv32 -M virt -kernel u-boot -nographic -initrd <a file>
>>
>> Ah, cool I can reproduce it now.
>>
>> It happens in load_elf32() (which is actually glue(load_elf, SZ)).
>>
>> This line ends up sign extending the value:
>> *pentry = (uint64_t)(elf_sword)ehdr.e_entry;
>>
>> and we just continue it from there.
>>
>> So I think this diff is a much better solution:
>>
>> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
>> index e7f0716fb6..348ecc39d5 100644
>> --- a/hw/riscv/virt.c
>> +++ b/hw/riscv/virt.c
>> @@ -62,7 +62,7 @@ static const struct MemmapEntry {
>> [VIRT_PCIE_ECAM] = { 0x30000000, 0x10000000 },
>> };
>>
>> -static uint64_t load_kernel(const char *kernel_filename)
>> +static target_ulong load_kernel(const char *kernel_filename)
>> {
>> uint64_t kernel_entry, kernel_high;
>>
>>
>>
>> Alistair
>>
>>>
>>> You can find the initrd address with
>>>
>>> U-Boot# fdt addr $fdtcontroladdr
>>> U-Boot# fdt ls /chosen
>>>
>>> Then take a peek at that address:
>>>
>>> U-Boot# md.b <addr>
>>>
>>> and you will see that there is nothing there without this patch. The
>>> reason is that the binary was loaded to a negative address. Again,
>>> probably some misguided sign extension.
>>>
>>>> It looks like the kernel entry address ends up being converted to
>>>> 0xffffffff80000000 incorrectly instead of being a real overflow.
>>>
>>> Yeah, so we seem to cast from int32_t to int64_t somewhere. Can you spot
>>> where exactly? That's where the bug is :).
>
> Actually this diff looks like a better more generic fix:
>
> diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
> index 81cecaf27e..7c1930e7a3 100644
> --- a/include/hw/elf_ops.h
> +++ b/include/hw/elf_ops.h
> @@ -342,8 +342,9 @@ static int glue(load_elf, SZ)(const char *name, int fd,
> }
> }
>
> - if (pentry)
> - *pentry = (uint64_t)(elf_sword)ehdr.e_entry;
> + if (pentry) {
> + *pentry = (uint64_t)(elf_word)ehdr.e_entry;
> + }
>
> glue(load_symbols, SZ)(&ehdr, fd, must_swab, clear_lsb, sym_cb);
>
> My only concern is that it will break some other 32-bit guest. It
> seems odd that no one else has seen this before.
True. IIRC we were playing dirty tricks with sign extension with
OpenBIOS back in the day.
So maybe a riscv (board) specific solution would be better.
Alex
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [Qemu-riscv] [Qemu-devel] [PATCH v1 1/1] riscv: virt: Cast the initrd start address to target bit length
@ 2018-11-27 22:35 ` Alexander Graf
0 siblings, 0 replies; 18+ messages in thread
From: Alexander Graf @ 2018-11-27 22:35 UTC (permalink / raw)
To: Alistair Francis
Cc: Palmer Dabbelt, Alistair Francis,
qemu-devel@nongnu.org Developers, qemu-riscv
On 27.11.18 23:28, Alistair Francis wrote:
> On Tue, Nov 27, 2018 at 2:24 PM Alistair Francis <alistair23@gmail.com> wrote:
>>
>> On Tue, Nov 27, 2018 at 2:12 PM Alexander Graf <agraf@suse.de> wrote:
>>>
>>>
>>>
>>> On 27.11.18 23:05, Alistair Francis wrote:
>>>> On Mon, Nov 26, 2018 at 11:02 AM Palmer Dabbelt <palmer@sifive.com> wrote:
>>>>>
>>>>> On Wed, 21 Nov 2018 18:09:27 PST (-0800), alistair23@gmail.com wrote:
>>>>>> On Wed, Nov 21, 2018 at 5:58 PM Palmer Dabbelt <palmer@sifive.com> wrote:
>>>>>>>
>>>>>>> On Wed, 21 Nov 2018 14:34:44 PST (-0800), Alistair Francis wrote:
>>>>>>>> Ensure that the calculate initrd offset points to a valid address for
>>>>>>>> the architecture.
>>>>>>>>
>>>>>>>> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
>>>>>>>> Suggested-by: Alexander Graf <agraf@suse.de>
>>>>>>>> Reported-by: Alexander Graf <agraf@suse.de>
>>>>>>>> ---
>>>>>>>> hw/riscv/virt.c | 7 ++++++-
>>>>>>>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>>>>>>>
>>>>>>>> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
>>>>>>>> index 2b38f89070..4467195fac 100644
>>>>>>>> --- a/hw/riscv/virt.c
>>>>>>>> +++ b/hw/riscv/virt.c
>>>>>>>> @@ -85,7 +85,12 @@ static hwaddr load_initrd(const char *filename, uint64_t mem_size,
>>>>>>>> * halfway into RAM, and for boards with 256MB of RAM or more we put
>>>>>>>> * the initrd at 128MB.
>>>>>>>> */
>>>>>>>> - *start = kernel_entry + MIN(mem_size / 2, 128 * MiB);
>>>>>>>> + /* As hwaddr is a 64-bit number we need to cast it for 32-bit */
>>>>>>>> +#if defined(TARGET_RISCV32)
>>>>>>>> + *start = (uint32_t) (kernel_entry + MIN(mem_size / 2, 128ULL * MiB));
>>>>>>>> +#elif defined(TARGET_RISCV64)
>>>>>>>> + *start = (uint64_t) (kernel_entry + MIN(mem_size / 2, 128 * MiB));
>>>>>>>> +#endif
>>>>>>>>
>>>>>>>> size = load_ramdisk(filename, *start, mem_size - *start);
>>>>>>>> if (size == -1) {
>>>>>>>> --
>>>>>>>> 2.19.1
>>>>>>>
>>>>>>> Maybe I'm missing something, but wouldn't something like
>>>>>>>
>>>>>>> uint64_t start_unclobbered = kernel_entry + MIN(mem_size / 2, 128ULL * MiB);
>>>>>>> assert(start_unclobbered == (hwaddr)start_unclobbered);
>>>>>>> *start = (hwaddr)start_unclobbered;
>>>>>>>
>>>>>>> work better? That should actually check this all lines up, as opposed to just
>>>>>>> silently truncating a bad address.
>>>>>>
>>>>>> The problem is that hwaddr is always 64-bit.
>>>>>>
>>>>>> Stupidly I forgot about target_ulong, which should work basically the
>>>>>> same as above. An assert() seems a little harsh though, Alex reported
>>>>>> problem was fixed with just a cast.
>>>>>
>>>>> OK, I must be misunderstanding the problem, then. With the current code, isn't
>>>>> it possible to end up causing start to overflow a 32-bit address? This would
>>>>> happen if kernel_entry is high, with IIUC is coming from the ELF to load and is
>>>>> therefor user input. I think that's worth some sort of assertion, as it'll
>>>>> definitely blow up trying to enter the kernel (and possible before that,
>>>>> assuming there's no target memory where it overflows into).
>>>>
>>>> I don't have a setup to reproduce this unfortunately, but Alex
>>>> reported that he saw start being excessively high (0xffffffff88000000)
>>>> when loading an initrd.
>>>
>>> Sorry for only jumping in so late.
>>>
>>> I didn't actually propose the cast as a solution. The problem must be
>>> sign extension of some variable in the mix. I didn't find out quickly
>>> which one it was, so I figured I'd leave it for someone else to debug :).
>>>
>>> The issue is very easy to reproduce: Build U-Boot for the virt machine
>>> for riscv32. Then run it with
>>>
>>> $ qemu-system-riscv32 -M virt -kernel u-boot -nographic -initrd <a file>
>>
>> Ah, cool I can reproduce it now.
>>
>> It happens in load_elf32() (which is actually glue(load_elf, SZ)).
>>
>> This line ends up sign extending the value:
>> *pentry = (uint64_t)(elf_sword)ehdr.e_entry;
>>
>> and we just continue it from there.
>>
>> So I think this diff is a much better solution:
>>
>> diff --git a/hw/riscv/virt.c b/hw/riscv/virt.c
>> index e7f0716fb6..348ecc39d5 100644
>> --- a/hw/riscv/virt.c
>> +++ b/hw/riscv/virt.c
>> @@ -62,7 +62,7 @@ static const struct MemmapEntry {
>> [VIRT_PCIE_ECAM] = { 0x30000000, 0x10000000 },
>> };
>>
>> -static uint64_t load_kernel(const char *kernel_filename)
>> +static target_ulong load_kernel(const char *kernel_filename)
>> {
>> uint64_t kernel_entry, kernel_high;
>>
>>
>>
>> Alistair
>>
>>>
>>> You can find the initrd address with
>>>
>>> U-Boot# fdt addr $fdtcontroladdr
>>> U-Boot# fdt ls /chosen
>>>
>>> Then take a peek at that address:
>>>
>>> U-Boot# md.b <addr>
>>>
>>> and you will see that there is nothing there without this patch. The
>>> reason is that the binary was loaded to a negative address. Again,
>>> probably some misguided sign extension.
>>>
>>>> It looks like the kernel entry address ends up being converted to
>>>> 0xffffffff80000000 incorrectly instead of being a real overflow.
>>>
>>> Yeah, so we seem to cast from int32_t to int64_t somewhere. Can you spot
>>> where exactly? That's where the bug is :).
>
> Actually this diff looks like a better more generic fix:
>
> diff --git a/include/hw/elf_ops.h b/include/hw/elf_ops.h
> index 81cecaf27e..7c1930e7a3 100644
> --- a/include/hw/elf_ops.h
> +++ b/include/hw/elf_ops.h
> @@ -342,8 +342,9 @@ static int glue(load_elf, SZ)(const char *name, int fd,
> }
> }
>
> - if (pentry)
> - *pentry = (uint64_t)(elf_sword)ehdr.e_entry;
> + if (pentry) {
> + *pentry = (uint64_t)(elf_word)ehdr.e_entry;
> + }
>
> glue(load_symbols, SZ)(&ehdr, fd, must_swab, clear_lsb, sym_cb);
>
> My only concern is that it will break some other 32-bit guest. It
> seems odd that no one else has seen this before.
True. IIRC we were playing dirty tricks with sign extension with
OpenBIOS back in the day.
So maybe a riscv (board) specific solution would be better.
Alex
^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2018-11-27 23:20 UTC | newest]
Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-11-21 22:34 [Qemu-devel] [PATCH v1 1/1] riscv: virt: Cast the initrd start address to target bit length Alistair Francis
2018-11-21 22:34 ` [Qemu-riscv] " Alistair Francis
2018-11-22 1:58 ` [Qemu-devel] " Palmer Dabbelt
2018-11-22 1:58 ` [Qemu-riscv] " Palmer Dabbelt
2018-11-22 2:09 ` Alistair Francis
2018-11-22 2:09 ` [Qemu-riscv] " Alistair Francis
2018-11-26 19:02 ` Palmer Dabbelt
2018-11-26 19:02 ` [Qemu-riscv] " Palmer Dabbelt
2018-11-27 22:05 ` Alistair Francis
2018-11-27 22:05 ` [Qemu-riscv] " Alistair Francis
2018-11-27 22:12 ` Alexander Graf
2018-11-27 22:12 ` [Qemu-riscv] " Alexander Graf
2018-11-27 22:24 ` Alistair Francis
2018-11-27 22:24 ` [Qemu-riscv] " Alistair Francis
2018-11-27 22:28 ` Alistair Francis
2018-11-27 22:28 ` [Qemu-riscv] " Alistair Francis
2018-11-27 22:35 ` Alexander Graf
2018-11-27 22:35 ` [Qemu-riscv] " Alexander Graf
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.