| Message ID | 6559716eae4b53d2cad39ec6011cdd7b1b32138a.1542839652.git.alistair.francis@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v1,1/1] riscv: virt: Cast the initrd start address to target bit length | expand |
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.
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
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.
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.
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
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
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
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
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) {
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(-)