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

[Palmer Dabbelt]

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).

This won't happen with the default Linux setup, but could happen if users are 
trying to do something weird.