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

[Julien Grall <julien.grall@xxxxxxx>]

On 24/07/17 14:14, Konrad Rzeszutek Wilk wrote:
> 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.
I think they should go. We want to limit the number of potential crash 
and give useful feedback to the user :).
Cheers,

--
Julien Grall

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