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

["Jan Beulich" <JBeulich@xxxxxxxx>]

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

Jan

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