[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v1 1/1] riscv: virt: Cast the initrd start addre
From: |
Alexander Graf |
Subject: |
Re: [Qemu-devel] [PATCH v1 1/1] riscv: virt: Cast the initrd start address to target bit length |
Date: |
Tue, 27 Nov 2018 23:35:58 +0100 |
User-agent: |
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.14; rv:60.0) Gecko/20100101 Thunderbird/60.3.1 |
On 27.11.18 23:28, Alistair Francis wrote:
> On Tue, Nov 27, 2018 at 2:24 PM Alistair Francis <address@hidden> wrote:
>>
>> On Tue, Nov 27, 2018 at 2:12 PM Alexander Graf <address@hidden> wrote:
>>>
>>>
>>>
>>> On 27.11.18 23:05, Alistair Francis wrote:
>>>> 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.
>>>
>>> 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
- [Qemu-devel] [PATCH v1 1/1] riscv: virt: Cast the initrd start address to target bit length, Alistair Francis, 2018/11/21
- Re: [Qemu-devel] [PATCH v1 1/1] riscv: virt: Cast the initrd start address to target bit length, Palmer Dabbelt, 2018/11/21
- Re: [Qemu-devel] [PATCH v1 1/1] riscv: virt: Cast the initrd start address to target bit length, Alistair Francis, 2018/11/21
- Re: [Qemu-devel] [PATCH v1 1/1] riscv: virt: Cast the initrd start address to target bit length, Palmer Dabbelt, 2018/11/26
- Re: [Qemu-devel] [PATCH v1 1/1] riscv: virt: Cast the initrd start address to target bit length, Alistair Francis, 2018/11/27
- Re: [Qemu-devel] [PATCH v1 1/1] riscv: virt: Cast the initrd start address to target bit length, Alexander Graf, 2018/11/27
- Re: [Qemu-devel] [PATCH v1 1/1] riscv: virt: Cast the initrd start address to target bit length, Alistair Francis, 2018/11/27
- Re: [Qemu-devel] [PATCH v1 1/1] riscv: virt: Cast the initrd start address to target bit length, Alistair Francis, 2018/11/27
- Re: [Qemu-devel] [PATCH v1 1/1] riscv: virt: Cast the initrd start address to target bit length,
Alexander Graf <=