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

Or use the xen.lds on, but we would need to tweak it as the ASSERTS
in it will complain.

> 
> >This allows us on ARM32 to erorr out with:
> >
> >livepatch: xen_hello_world: dest=00a0404a (.altinstructions) is not aligned 
> >properly!
> 
> I therefore doubt this is a correct diagnostic.

It is - if we try to muck around with the .altinstructions we will crash
on ARM32.

> 
> What you may want to consider is silently padding sections to a multiple
> of their specified alignment.

Hm. That hadn't occured to me. Let me try that.

> 
> Jan
> 

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