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

[Konrad Rzeszutek Wilk <konrad@xxxxxxxxxx>]

On Mon, Sep 11, 2017 at 03:01:15AM -0600, Jan Beulich wrote:
> >>> On 09.09.17 at 14:05, <konrad@xxxxxxxxxx> wrote:
> > On Fri, Sep 08, 2017 at 03:30:07AM -0600, Jan Beulich wrote:
> >> >>> On 07.09.17 at 19:36, <konrad@xxxxxxxxxx> wrote:
> >> > On Wed, Aug 02, 2017 at 03:20:05AM -0600, Jan Beulich wrote:
> >> >> >>> Konrad Rzeszutek Wilk <konrad.wilk@xxxxxxxxxx> 07/31/17 6:04 PM >>>
> >> >> >On Mon, Jul 31, 2017 at 07:55:34AM -0600, Jan Beulich wrote:
> >> >> >> >>> Konrad Rzeszutek Wilk <konrad@xxxxxxxxxx> 07/26/17 9:50 PM >>>
> >> >> >> >--- a/docs/misc/livepatch.markdown
> >> >> >> >+++ b/docs/misc/livepatch.markdown
> >> >> >> >@@ -279,6 +279,10 @@ It may also have some architecture-specific 
> >> >> >> >sections. 
> >> > For example:
> >> >> >> >* Exception tables.
> >> >> >> >* Relocations for each of these sections.
> >> >> >>  >
> >> >> >> >+Note that on ARM 32 the sections SHOULD be four byte aligned. 
> >> >> >> >Otherwise
> >> >> >> >+we risk hitting Data Abort exception as un-aligned manipulation of 
> >> >> >> >data is
> >> >> >> >+prohibited on ARM 32.
> >> >> >> 
> >> >> >> This (and hence the rest of the patch) is not in line with the 
> >> >> >> outcome of 
> >> > the
> >> >> >> earlier discussion we had. Nothing is wrong with a section having 
> >> >> >> smaller
> >> >> >> alignment, as long as there are no 32-bit (or wider, but I don't 
> >> >> >> think 
> > there
> >> >> >> are any such) relocations against such a section. And even if there 
> >> >> >> were, 
> > I
> >> >> >> think it should rather be the code doing the relocations needing to 
> >> >> >> cope, 
> >> > as
> >> >> >> I don't think the ARM ELF ABI imposes any such restriction.
> >> >> >
> >> >> >The idea behind this patch is to give advance warnings. Akin to what
> >> >> >2ff229643b739e2fd0cd0536ee9fca506cfa92f8
> >> >> >"xen/livepatch: Don't crash on encountering STN_UNDEF relocations" did.
> >> >> >
> >> >> >The other patches in this series fix the alignment issues.
> >> >> >
> >> >> >The ARM ELF ABI 
> >> > 
> > (http://infocenter.arm.com/help/topic/com.arm.doc.ihi0044f/IHI0044F_aaelf.pdf
> >  
> > 
> >> > )
> >> >> >
> >> >> >says:
> >> >> >
> >> >> >4.3.5 Section Alignment
> >> >> >There is no minimum alignment required for a section. However, 
> >> >> >sections 
> >> > containing thumb code must be at least
> >> >> >16-bit aligned and sections containing ARM code must be at least 
> >> >> >32-bit 
> >> > aligned.
> >> >> >Platform standards may set a limit on the maximum alignment that they 
> >> >> >can 
> >> > guarantee (normally the page size).
> >> >> 
> >> >> Note the "thumb code" and "ARM code" in here - iirc you're checking 
> >> >> _all_
> >> >> sections, not just ones containing code.
> >> > 
> >> > I can fix the code to only do the check for 'X' ones:
> >> > 
> >> >   [ 2] .text             PROGBITS         0000000000000000  00000070
> >> >        00000000000000ca  0000000000000000  AX       0     0     16
> >> >   [ 4] .altinstr_replace PROGBITS         0000000000000000  0000013c
> >> >        000000000000000b  0000000000000000  AX       0     0     4
> >> >   [ 5] .fixup            PROGBITS         0000000000000000  00000147
> >> >        000000000000000d  0000000000000000  AX       0     0     1
> >> > 
> >> > 
> >> > And also have the check in the relocation - which right now are
> >> > 32-bit: R_ARM_ABS32, R_ARM_REL32, R_ARM_MOVW_ABS_NC, R_ARM_MOVT_ABS,
> >> > R_ARM_CALL, R_ARM_JUMP24 so will leave the code as in
> >> > arch_livepatch_perform.
> >> 
> >> Relocations applicable to code only _may_ be acceptable to have
> >> such an alignment check (but I could see cases where even that
> >> might be too aggressive), but afaik R_ARM_ABS32 isn't a code
> >> only one (out of the set listed above), so I doubt this should have
> >> an alignment check.
> >> 
> >> > But neither one of those is going to help in catching livepatches
> >> > that have the wrong alignment without relocations and not executable.
> >> > For example .livepatch.depends
> >> 
> >> What does "wrong alignment" mean when there's no code involved?
> > 
> > Anything which we try to access as a structure, or unsigned int,
> > that is not aligned to four bytes.
> > 
> > For example accessing .livepatch.depends from memory and blowing
> > up (hypervisor crashes) b/c it does not start at an four byte aligned
> > location.
> 
> Hmm, as long as the relocation isn't required to be against aligned
> fields only (mandated by the processor ABI) I think the code doing
> the relocations would instead need to split the access, rather than
> calling the section misaligned or increasing alignment beyond what
> the ELF section headers say.

Maybe the serial log would explain this better:

xend_config_format     : 4
Executing: '(set -e;cd /root/test/livepatch;xen-livepatch load 
xen_bye_world.livepatch)' ..(XEN) livepatch.c:413: livepatch: xen_bye_world: 
Loaded .note.gnu.build-id at 00a08000
(XEN) livepatch.c:413: livepatch: xen_bye_world: Loaded .text at 00a06000
(XEN) livepatch.c:413: livepatch: xen_bye_world: Loaded .rodata at 00a08024
(XEN) livepatch.c:413: livepatch: xen_bye_world: Loaded .rodata.str1.4 at 
00a08038
(XEN) livepatch.c:413: livepatch: xen_bye_world: Loaded .livepatch.depends at 
00a08043
(XEN) livepatch.c:413: livepatch: xen_bye_world: Loaded .livepatch.funcs at 
00a07000
(XEN) livepatch_elf.c:343: livepatch: xen_bye_world: Symbol resolved:  => 
0xa08000 (.note.gnu.build-id)
(XEN) livepatch_elf.c:343: livepatch: xen_bye_world: Symbol resolved:  => 
0xa06000 (.text)
(XEN) livepatch_elf.c:343: livepatch: xen_bye_world: Symbol resolved:  => 
0xa08024 (.rodata)
(XEN) livepatch_elf.c:343: livepatch: xen_bye_world: Symbol resolved:  => 
0xa08038 (.rodata.str1.4)
(XEN) livepatch_elf.c:343: livepatch: xen_bye_world: Symbol resolved:  => 
0xa08043 (.livepatch.depends)
(XEN) livepatch_elf.c:343: livepatch: xen_bye_world: Symbol resolved:  => 
0xa07000 (.livepatch.funcs)
(XEN) livepatch_elf.c:319: livepatch: xen_bye_world: Absolute symbol: 
xen_bye_world_func.c => 00000000
(XEN) livepatch_elf.c:343: livepatch: xen_bye_world: Symbol resolved: $a => 
0xa06000 (.text)
(XEN) livepatch_elf.c:343: livepatch: xen_bye_world: Symbol resolved: .LC0 => 
0xa08038 (.rodata.str1.4)
(XEN) livepatch_elf.c:343: livepatch: xen_bye_world: Symbol resolved: $d => 
0xa08038 (.rodata.str1.4)
(XEN) livepatch_elf.c:319: livepatch: xen_bye_world: Absolute symbol: 
xen_bye_world.c => 00000000
(XEN) livepatch_elf.c:343: livepatch: xen_bye_world: Symbol resolved: $d => 
0xa08024 (.rodata)
(XEN) livepatch_elf.c:343: livepatch: xen_bye_world: Symbol resolved: 
bye_world_patch_this_fnc => 0xa08024 (.rodata)
(XEN) livepatch_elf.c:343: livepatch: xen_bye_world: Symbol resolved: $d => 
0xa07000 (.livepatch.funcs)
(XEN) livepatch_elf.c:343: livepatch: xen_bye_world: Symbol resolved: 
livepatch_xen_bye_world => 0xa07000 (.livepatch.funcs)
(XEN) livepatch_elf.c:314: livepatch: xen_bye_world: Undefined symbol resolved: 
xen_extra_version => 0x23ffe0
(XEN) livepatch_elf.c:343: livepatch: xen_bye_world: Symbol resolved: 
xen_bye_world => 0xa06000 (.text)
(XEN) CPU0: Unexpected Trap: Data Abort
(XEN) ----[ Xen-4.9Hello World  arm32  debug=y   Not tainted ]----
(XEN) CPU:    0
(XEN) PC:     002400a0 xen_build_id_check+0x8/0xe8
(XEN) CPSR:   2007001a MODE:Hypervisor
(XEN)      R0: 00a08043 R1: 00000024 R2: 43fde940 R3: 43fde944
(XEN)      R4: 002960b0 R5: 00000000 R6: 00000000 R7: 43fde8b8
(XEN)      R8: 002960bc R9: 00000000 R10:1001c000 R11:43fd7dd4 R12:00000000
(XEN) HYP: SP: 43fd7cd4 LR: 0021a9c0
(XEN) 
(XEN)   VTCR_EL2: 80003558
(XEN)  VTTBR_EL2: 00010000bf9ce000
(XEN) 
(XEN)  SCTLR_EL2: 30cd187f
(XEN)    HCR_EL2: 000000000038663f
(XEN)  TTBR0_EL2: 00000000bfb12000
(XEN) 
(XEN)    ESR_EL2: 94000021
(XEN)  HPFAR_EL2: 000000000001c810
(XEN)      HDFAR: 00a0804b
(XEN)      HIFAR: 31dc8c18
(XEN) 
(XEN) Xen stack trace from sp=43fd7cd4:
(XEN)    94000021 1001d890 00a07000 0028b7f0 0028b410 00000008 0028b974 0028b410
(XEN)    00000001 00000002 00000007 43fd7d0c 00263bd4 43fd7d44 43fde958 00001d64
(XEN)    1001c000 43fde9e0 43fdeb98 0000001e 43fdeb60 43fdeb70 00000018 5f6e6578
(XEN)    5f657962 6c726f77 00000064 00000098 00009680 0028c293 400265e0 00000000
(XEN)    00989680 00000000 00989680 00000000 43fd7dc0 43fd7d7c 0025ffb0 02786800
(XEN)    0007c340 0007c200 00262f94 7c340b80 00000088 00000088 00000088 fc340004
(XEN)    b6fa7004 02786800 43fd7e40 43fd7dd4 0025bafc 00318580 43fd7ec8 43fd7e3c
(XEN)    00319614 43fd7f58 0029624c 00318580 002e1f80 00318580 00000000 43fd7eec
(XEN)    0023bbe4 1ca3fcf8 b6fa7004 00000000 00000000 00001cb5 00008c40 43fd7eac
(XEN)    00000000 43fd7e0c 0025ffb0 025ef920 0006f7c9 0006f600 00262f94 6f7c9b80
(XEN)    00000017 00000001 00000001 025ef920 ef7c9017 0029709c ef7c9017 43fd7e6c
(XEN)    0025b7b4 00000000 00318580 0000001b 0000000f 00000000 00000000 b6faa004
(XEN)    00000000 0000000e 00000000 00001d64 00000000 b6fa8004 00000000 b6e6b9cc
(XEN)    00000000 00023064 00000000 00010660 00000000 00023014 00000000 00010660
(XEN)    b6de12c0 b6de1780 00000001 00000000 b6f92fdf 00000000 00000001 00000001
(XEN)    00000000 b6de12c0 b6f575ac 00023118 00000002 43fd7f58 0023b21c 43fd7f58
(XEN)    00000000 00305000 beb7b8e0 edcf2000 00000000 43fd7f54 002673c4 00000000
(XEN)    2007009a 43fd7f24 43fd7f24 00000000 00000021 00000004 43fd7f58 00000000
(XEN)    00318580 ee2b8840 7c340f5f 00e00000 eff80800 00000d38 ffefed38 43fd7f44
(XEN)    00000000 c0a57000 00000000 00305000 beb7b8e0 edcf2000 00000000 43fd7f58
(XEN) Xen call trace:
(XEN)    [<002400a0>] xen_build_id_check+0x8/0xe8 (PC)
(XEN)    [<0021a9c0>] livepatch_op+0x768/0x1610 (LR)
(XEN)    [<0023bbe4>] do_sysctl+0x9c8/0xa9c
(XEN)    [<002673c4>] do_trap_guest_sync+0x11e0/0x177c
(XEN)    [<0026b6a0>] entry.o#return_from_trap+0/0x4
(XEN) 
(XEN) 
(XEN) ****************************************
(XEN) Panic on CPU 0:
(XEN) CPU0: Unexpected Trap: Data Abort
(XEN) 
(XEN) ****************************************
(XEN) 
(XEN) Reboot in five seconds...


Keep in mind that this only happens if I cross-compile ARM32 under x86.

If I compile the test-case under ARM32 it works OK (as the
.livepatch.depends ends up being aligned to four bytes).

Now having said I am going to be posting a v3 patchset shortly
which has a fix for this ("xen/livepatch/x86/arm32: Force
.livepatch.depends section to be uint32_t aligned.").

But nonethless it may be better to have the extra belt
and suspends to catch these issues during run-time if somebody
does mess up with the alignment by mistake.

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