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

[Julien Grall <julien.grall@xxxxxxx>]

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.
Cheers,

--
Julien Grall

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