Re: [kvmtool][PATCH] arm64: Obtain text offset from kernel image

[Marc Zyngier <maz@kernel.org>]

Hi Alex,

On 2020-06-05 13:16, Alexandru Elisei wrote:
> Hi Marc,
> 
> On 6/5/20 11:49 AM, Marc Zyngier wrote:
>> Recent changes made to Linux 5.8 have outlined that kvmtool
>> hardcodes the text offset instead of reading it from the arm64
>> image itself.
>> 
>> To address this, import the image header structure into kvmtool
>> and do the right thing. 32bit guests are still loaded to their
>> usual locations.
>> 
>> Reported-by: Ard Biesheuvel <ardb@kernel.org>
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> ---
>>  Makefile                           |  1 +
>>  arm/aarch32/include/kvm/kvm-arch.h |  2 +-
>>  arm/aarch64/include/asm/image.h    | 59 
>> ++++++++++++++++++++++++++++++
>>  arm/aarch64/include/kvm/kvm-arch.h |  5 +--
>>  arm/aarch64/kvm.c                  | 30 +++++++++++++++
>>  arm/kvm.c                          |  2 +-
>>  6 files changed, 94 insertions(+), 5 deletions(-)
>>  create mode 100644 arm/aarch64/include/asm/image.h
>>  create mode 100644 arm/aarch64/kvm.c
>> 
>> [..]
> 
> This is a great addition to kvmtool, thank you! Before I do a more 
> in-depth
> review, I have some general questions.
> 
> Regarding the actual value of text_offset, the booting.rst document 
> says:
> 
> "Prior to v3.17, the endianness of text_offset was not specified.  In
> these cases
> image_size is zero and text_offset is 0x80000 in the endianness of the 
> kernel. 
> Where image_size is non-zero image_size is little-endian and must be 
> respected. 
> Where image_size is zero, text_offset can be assumed to be 0x80000".
> 
> All header fields are declared little-endian, which looks to me like it 
> would
> break kernels older than 3.17. If that was intentional, I think it's 
> worth
> documenting somewhere, or at least a comment for the 
> kvm__arch_get_kern_offset
> function.

TBH, I didn't give pre-3.17 *big-endian* much thought. Happy to document
it though.

> 
> Now that we are parsing the kernel header, have you considered
> checking the magic
> number to make sure the user specified a valid kernel image? It might
> save someone
> some time debugging why the kernel isn't booting, if, for example, they 
> are
> booting an armv7 kernel, but they forgot to specify --aarch32.

That'd be interesting. I'd be reluctant to prevent it from booting
altogether, but I can definitely detect the lack of signature and
print a warning.

Thanks,

         M.
-- 
Jazz is not dead. It just smells funny...
_______________________________________________
kvmarm mailing list
kvmarm@lists.cs.columbia.edu
https://lists.cs.columbia.edu/mailman/listinfo/kvmarm