On Tue, Nov 27, 2018 at 2:24 PM Alistair Francis <alistai...@gmail.com> wrote: > > On Tue, Nov 27, 2018 at 2:12 PM Alexander Graf <ag...@suse.de> wrote: > > > > > > > > On 27.11.18 23:05, Alistair Francis wrote: > > > On Mon, Nov 26, 2018 at 11:02 AM Palmer Dabbelt <pal...@sifive.com> wrote: > > >> > > >> On Wed, 21 Nov 2018 18:09:27 PST (-0800), alistai...@gmail.com wrote: > > >>> On Wed, Nov 21, 2018 at 5:58 PM Palmer Dabbelt <pal...@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.fran...@wdc.com> > > >>>>> Suggested-by: Alexander Graf <ag...@suse.de> > > >>>>> Reported-by: Alexander Graf <ag...@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