[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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





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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.