Re: [Xen-devel] [PATCH v1 3/3] xen/livepatch/ARM32: Don't crash on livepatches loaded with wrong alignment.

[Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx>]

On Mon, Jul 24, 2017 at 11:34:26AM +0100, Julien Grall wrote:
> Hi,
> 
> On 12/07/17 07:07, Jan Beulich wrote:
> > > > > Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> 07/11/17 10:34 PM >>>
> > > On Tue, Jul 11, 2017 at 02:06:09PM -0600, Jan Beulich wrote:
> > > > > > > Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> 07/11/17 6:53 PM 
> > > > > > > >>>
> > > > > This issue was observed on ARM32 with a cross compiler for the
> > > > > livepatches. Mainly the livepatches .data section size was not
> > > > > aligned to the section alignment:
> > > > > 
> > > > > ARM32 native:
> > > > > Contents of section .rodata:
> > > >  >0000 68695f66 756e6300 63686563 6b5f666e  hi_func.check_fn
> > > >  >0010 63000000 78656e5f 65787472 615f7665  c...xen_extra_ve
> > > >  >0020 7273696f 6e000000                    rsion...
> > > > > 
> > > > > ARM32 cross compiler:
> > > > > Contents of section .rodata:
> > > >  >0000 68695f66 756e6300 63686563 6b5f666e  hi_func.check_fn
> > > >  >0010 63000000 78656e5f 65787472 615f7665  c...xen_extra_ve
> > > >  >0020 7273696f 6e00                        rsion.
> > > > > 
> > > > > And when we loaded it:
> > > > > 
> > > > > native:
> > > > > 
> > > > > (XEN) livepatch.c:413: livepatch: xen_hello_world: Loaded .text at 
> > > > > 00a02000
> > > > > (XEN) livepatch.c:413: livepatch: xen_hello_world: Loaded .rodata at 
> > > > > 00a04024
> > > > > (XEN) livepatch.c:413: livepatch: xen_hello_world: Loaded 
> > > > > .altinstructions at 00a0404c
> > > > > 
> > > > > cross compiler:
> > > > > (XEN) livepatch.c:413: livepatch: xen_hello_world: Loaded .text at 
> > > > > 00a02000
> > > > > (XEN) livepatch.c:413: livepatch: xen_hello_world: Loaded .rodata at 
> > > > > 00a04024
> > > > > (XEN) livepatch.c:413: livepatch: xen_hello_world: Loaded 
> > > > > .altinstructions at 00a0404a
> > > > > 
> > > > > (See 4a vs 4c)
> > > > > 
> > > > > native readelf:
> > > >   >[ 4] .rodata           PROGBITS        00000000 000164 000028 00   A 
> > > >  0   0  4
> > > >   >[ 5] .altinstructions  PROGBITS        00000000 00018c 00000c 00   A 
> > > >  0   0  1
> > > > > 
> > > > > cross compiler readelf --sections:
> > > >   >[ 4] .rodata           PROGBITS        00000000 000164 000026 00   A 
> > > >  0   0  4
> > > >   >[ 5] .altinstructions  PROGBITS        00000000 00018a 00000c 00   A 
> > > >  0   0  1
> > > > > 
> > > > > And as can be seen the .altinstructions have alignment of 1 which from
> > > > > 'man elf' is: "Values of zero and one mean no alignment is required."
> > > > > which means we can ignore it.
> > > > > 
> > > > > However ignoring this will result in a crash as when we started 
> > > > > processing
> > > > > ".rel.altinstructions" for ".altinstructions" with a cross-compiled 
> > > > > payload
> > > > > we would end up poking in an section that was not aligned properly in 
> > > > > memory
> > > > > and crash.
> > > > 
> > > > Which section is it that would not be aligned properly in memory?
> > > 
> > > .altinstructions, thanks to .rodata not being padded properly.
> > > 
> > > > .altinstructions, with an alignment of 1, can be placed anywhere. You
> > > > shouldn't enforce extra alignment. If higher alignment is needed, the
> > > > code producing this section emission needs to be fixed.
> > > 
> > > And there is the path to madness :-) We would need to provide an
> > > linker map to make sure that they are with the correct alignment.
> > 
> > Why? I'd expect it to be the assembler directives creating contributions to
> > that section to simply lack a .align or alike. And indeed, there's nothing
> > like that in ARM's alternative.h. Please see commit 01fe4da624 ("x86: force
> > suitable alignment in sources rather than in linker script") for further
> > context.
> 
> FWIW, I agree with Jan here. .altinstructions section should contain the
> right alignment in the source code.

Sure.

Regardless of that  - should this code which catches errant alignments
of livepatches (say they are generated by other tools) still be in the code 
base?

It is extra belt and suspenders.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel