Re: XTF-on-ARM: Bugs

[Julien Grall <julien@xxxxxxx>]

Hi Andrew,

On 21/06/2022 14:30, Andrew Cooper wrote:
> On 21/06/2022 13:07, Julien Grall wrote:
> > On 21/06/2022 12:27, Andrew Cooper wrote:
> > > Hello,
> > > I tried to have a half hour respite from security and push forward
> > > with XTF-on-ARM, but the result was a mess.
> > >
> > > https://github.com/andyhhp/xtf/commit/bc86e2d271f2107da9b1c9bc55a050dbdf07c6c6
> > > is the absolute bare minimum stub VM, which has a zImage{32,64}
> > > header, sets up the stack, makes one CONSOLEIO_write hypercall, and
> > > then a clean SCHEDOP_shutdown.
> > >
> > > There are some bugs:
> > >
> > > 1) kernel_zimage32_probe() rejects relocatable binaries, but if I
> > > skip the check it works fine.
> > Hmmmm... which check are you referring to?
> if ( (end - start) > size )
>      return -EINVAL;
>
> Although now I think about it, the problem is subtly different.
>
> Section Headers:
>    [Nr] Name              Type            Addr     Off    Size   ES Flg
> Lk Inf Al
>    [ 0]                   NULL            00000000 000000 000000 00
> 0   0  0
>    [ 1] .text             PROGBITS        40000000 010000 000094 00  AX
> 0   0  4
>    [ 2] .data             PROGBITS        40001000 011000 000000 00  WA
> 0   0  1
>    [ 3] .rodata           PROGBITS        40001000 011000 000012 00   A
> 0   0  4
>    [ 4] .bss              NOBITS          40002000 011012 001000 00  WA
> 0   0  4
>
> end is calculated as 0x3000 which includes the bss (inc stack which is
> bss page aligned), while the raw binary size is 0x1012 because it stops
> at the end of .rodata.
Ok. I agree this is a bug. Can you send a patch?
> > > Furthermore, kernel_zimage64_probe() ignores the header and assumes
> > > the binary is relocatable.
> > Are you referring to bit 3 "Kernel physical placement"?
> No. This:
>
> /* Currently there is no length in the header, so just use the size */
> start = 0;
> end = size;
>
> Which isn't true even for the v0 header.  The field named text_offset in
> Xen's code is start, and res1 is end (or size for relocatable).
Hmmm... text_offset is not the start. But I agree that res1 is the 
effective size and should be used instead of the binary size.
> > > Both probe functions fail to check the endianness marker.
> > AFAIU the header is little endian. So it is not clear to me why we
> > should check the endianess marker?
> Not the endieness of the header, the endianness of the image.  Both
> headers have a field which should ought to be checked for != LE seeing
> as Xen doesn't support big endian domains yet
Aside potential bugs, big endian OS should boot on Xen (PV protocol and 
hypercalls are always litte endian).
[...]

> > > (XEN) Hardware Dom0 halted: halting machine
> > >
> > > which is weird.  The CONSOLEIO_write fails to read the passed
> > > pointer, despite appearing to have a ip-relative load to find the
> > > string, while the SCHEDOP_shutdown passes its parameter fine (it's a
> > > stack relative load).
> >  From a brief look, your code is still running with MMU off and Cache
> > "off" (on armv8, it is more a bypass "cache" rather than off).
> >
> > This means that you ought to be a lot more careful when
> > reading/writing value to avoid reading any stall data.
> There are no relocation/etc so everything has well defined behaviour
> even when the caches are off.
The problem is you are writing to the stack and then passing a pointer 
to the stack to Xen. For hypercalls, we mandate the memory to be 
cacheable (see arch-arm.h). So Xen may read a different value than what 
you passed.
> > > Other observations:
> > >
> > > * There is no documented vCPU starting state.
> > See
> > https://github.com/torvalds/linux/blob/master/Documentation/arm64/booting.rst.
> What's it got to do with Xen's vCPU starting state?
Because we are following what Image defined. Anything outside is 
implementation defined and not something that an OS should rely on.
>   Also, that's
> clearly not relevant for arm32 even if the implication is "Xen only
> speaks the Linux ABI".
The interface exposed to the guest depends on the binary format used. At 
the moment, we are implementing zImage, Image and U-boot. If there were 
another, then the vCPU will be the same as defined by the new format.
> It needs to be in docs/ (or public at a stretch) and not in the heads of
> the maintainers.
Patches are welcomed.

> > > * Qemu is infinitely easier to to use (i.e. no messing with dtb/etc)
> > > as -kernel xen -initrd test-$foo with a oneliner change to the dtb
> > > parsing to treat ramdisk and no kernel as the dom0 kernel.  Maybe a
> > > better change would be to modify qemu to understand multiple -kernel's.
> > > * Xen can't load ELFs.
> > The support was dropped in 2018 because it was bogus and not used:
> >
> > https://lists.xenproject.org/archives/html/xen-devel/2018-06/msg00242.html
> >
> >
> > Personally, I think that zImage/Image is simple enough that
> > re-introducing ELF is not worth it. But I would be OK to consider
> > patches if you feel like writing them.
> There is a massive usability improvement from being able to point normal
> toolchain tools at the same binary you're trying to load.
Ditto.

Cheers,

--
Julien Grall