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

From: Konrad Rzeszutek Wilk <konrad@kernel.org>
To: Jan Beulich <jbeulich@suse.com>
Cc: andrew.cooper3@citrix.com, julien.grall@arm.com,
	sstabellini@kernel.org, xen-devel@lists.xenproject.org
Subject: Re: [PATCH v2 3/5] xen/livepatch/ARM32: Don't load and crash on livepatches loaded with wrong alignment.
Date: Thu, 7 Sep 2017 13:36:46 -0400	[thread overview]
Message-ID: <20170907173644.GA7717@localhost.localdomain> (raw)
In-Reply-To: <59819945020000780010323D@prv-mh.provo.novell.com>

On Wed, Aug 02, 2017 at 03:20:05AM -0600, Jan Beulich wrote:
> >>> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> 07/31/17 6:04 PM >>>
> >On Mon, Jul 31, 2017 at 07:55:34AM -0600, Jan Beulich wrote:
> >> >>> Konrad Rzeszutek Wilk <konrad@kernel.org> 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.

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

Thoughts on how you would want to catch those?

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