Re: [Qemu-riscv] [Qemu-devel] [PATCH v1 1/1] riscv: virt: Cast the initrd start address to target bit length

[Alistair Francis]

On Mon, Nov 26, 2018 at 11:02 AM Palmer Dabbelt <address@hidden> wrote:
>
> On Wed, 21 Nov 2018 18:09:27 PST (-0800), address@hidden wrote:
> > On Wed, Nov 21, 2018 at 5:58 PM Palmer Dabbelt <address@hidden> 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 <address@hidden>
> >> > Suggested-by: Alexander Graf <address@hidden>
> >> > Reported-by: Alexander Graf <address@hidden>
> >> > ---
> >> >  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.