| Message ID | 20170920223148.13137-5-konrad.wilk@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
>>> On 21.09.17 at 00:31, <konrad@kernel.org> wrote: > @@ -272,6 +271,16 @@ int arch_livepatch_perform(struct livepatch_elf *elf, > elf->name, symndx); > return -EINVAL; > } > + else if ( (type != R_ARM_ABS32 && type != R_ARM_REL32) /* Only check code. */ && > + ((uint32_t)dest % sizeof(uint32_t)) ) > + { > + dprintk(XENLOG_ERR, LIVEPATCH "%s: dest=%p (%s) is not aligned properly!\n", > + elf->name, dest, base->name); > + return -EINVAL; > + } And no similar check being added to ARM64? Looking at that code I also notice that the general "minimum 32-bit width" there is likely wrong for at least ABS16 and PREL16. > --- a/xen/common/livepatch.c > +++ b/xen/common/livepatch.c > @@ -473,6 +473,13 @@ static bool section_ok(const struct livepatch_elf *elf, > return false; > } > > + if ( !arch_livepatch_verify_alignment(sec) ) > + { > + dprintk(XENLOG_ERR, LIVEPATCH "%s: %s text section is not aligned properly!\n", > + elf->name, sec->name); If you really mean to say "text section" here, then the SHF_EXECINSTR check should move here too. Jan
On 09/20/2017 11:31 PM, Konrad Rzeszutek Wilk wrote: > The ARM 32&64 ELF specification says "sections containing ARM > code must be at least 32-bit aligned." This patch adds the > check for that. We also make sure that this check is done > when doing relocations for the types that are considered > ARM code. However we don't have to check for all as we only > implement a small subset of them - as such we only check for > data types that are implemented - and if the type is anything else > and not aligned to 32-bit, then we error out. > > Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> snip > +bool arch_livepatch_verify_alignment(const struct livepatch_elf_sec *sec) > +{ > + if ( (sec->sec->sh_flags & SHF_EXECINSTR) && > + ((vaddr_t)sec->load_addr % sizeof(uint32_t)) ) > + return false; > + > + return true; > +}; > + There is a stray semicolon here. If I understand correctly, this patch seems to be doing two separate things: * Checking section alignment when loading the livepatch on ARM32/64. * Checking that relocation destinations are aligned when applying relocations (on ARM32 only). Surely these are separate and should it should be split into two patches? Is the latter check not needed for ARM64? -- Ross Lagerwall
diff --git a/docs/misc/livepatch.markdown b/docs/misc/livepatch.markdown index 54a6b850cb..59f89aa292 100644 --- a/docs/misc/livepatch.markdown +++ b/docs/misc/livepatch.markdown @@ -279,6 +279,8 @@ It may also have some architecture-specific sections. For example: * Exception tables. * Relocations for each of these sections. +Note that on ARM 32 & 64 the sections containing code MUST be four byte aligned. + The Xen Live Patch core code loads the payload as a standard ELF binary, relocates it and handles the architecture-specifc sections as needed. This process is much like what the Linux kernel module loader does. diff --git a/xen/arch/arm/arm32/livepatch.c b/xen/arch/arm/arm32/livepatch.c index 41378a54ae..4fcbb59be5 100644 --- a/xen/arch/arm/arm32/livepatch.c +++ b/xen/arch/arm/arm32/livepatch.c @@ -233,7 +233,7 @@ int arch_livepatch_perform(struct livepatch_elf *elf, uint32_t val; void *dest; unsigned char type; - s32 addend; + s32 addend = 0; if ( use_rela ) { @@ -251,7 +251,6 @@ int arch_livepatch_perform(struct livepatch_elf *elf, symndx = ELF32_R_SYM(r->r_info); type = ELF32_R_TYPE(r->r_info); dest = base->load_addr + r->r_offset; /* P */ - addend = get_addend(type, dest); } if ( symndx == STN_UNDEF ) @@ -272,6 +271,16 @@ int arch_livepatch_perform(struct livepatch_elf *elf, elf->name, symndx); return -EINVAL; } + else if ( (type != R_ARM_ABS32 && type != R_ARM_REL32) /* Only check code. */ && + ((uint32_t)dest % sizeof(uint32_t)) ) + { + dprintk(XENLOG_ERR, LIVEPATCH "%s: dest=%p (%s) is not aligned properly!\n", + elf->name, dest, base->name); + return -EINVAL; + } + + if ( !use_rela ) + addend = get_addend(type, dest); val = elf->sym[symndx].sym->st_value; /* S */ diff --git a/xen/arch/arm/livepatch.c b/xen/arch/arm/livepatch.c index 3e53524365..76723f1f1a 100644 --- a/xen/arch/arm/livepatch.c +++ b/xen/arch/arm/livepatch.c @@ -135,6 +135,15 @@ bool arch_livepatch_symbol_ok(const struct livepatch_elf *elf, return true; } +bool arch_livepatch_verify_alignment(const struct livepatch_elf_sec *sec) +{ + if ( (sec->sec->sh_flags & SHF_EXECINSTR) && + ((vaddr_t)sec->load_addr % sizeof(uint32_t)) ) + return false; + + return true; +}; + int arch_livepatch_secure(const void *va, unsigned int pages, enum va_type type) { unsigned long start = (unsigned long)va; diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c index 406eb910cc..48d20fdacd 100644 --- a/xen/arch/x86/livepatch.c +++ b/xen/arch/x86/livepatch.c @@ -148,6 +148,12 @@ bool arch_livepatch_symbol_deny(const struct livepatch_elf *elf, return false; } +bool arch_livepatch_verify_alignment(const struct livepatch_elf_sec *sec) +{ + /* Unaligned access on x86 is fine. */ + return true; +} + int arch_livepatch_perform_rel(struct livepatch_elf *elf, const struct livepatch_elf_sec *base, const struct livepatch_elf_sec *rela) diff --git a/xen/common/livepatch.c b/xen/common/livepatch.c index b9376c94e9..f736c3a7ea 100644 --- a/xen/common/livepatch.c +++ b/xen/common/livepatch.c @@ -473,6 +473,13 @@ static bool section_ok(const struct livepatch_elf *elf, return false; } + if ( !arch_livepatch_verify_alignment(sec) ) + { + dprintk(XENLOG_ERR, LIVEPATCH "%s: %s text section is not aligned properly!\n", + elf->name, sec->name); + return false; + } + return true; } diff --git a/xen/include/xen/livepatch.h b/xen/include/xen/livepatch.h index 98ec01216b..e9bab87f28 100644 --- a/xen/include/xen/livepatch.h +++ b/xen/include/xen/livepatch.h @@ -76,6 +76,7 @@ void arch_livepatch_init(void); #include <asm/livepatch.h> int arch_livepatch_verify_func(const struct livepatch_func *func); +bool arch_livepatch_verify_alignment(const struct livepatch_elf_sec *sec); static inline unsigned int livepatch_insn_len(const struct livepatch_func *func) {
The ARM 32&64 ELF specification says "sections containing ARM code must be at least 32-bit aligned." This patch adds the check for that. We also make sure that this check is done when doing relocations for the types that are considered ARM code. However we don't have to check for all as we only implement a small subset of them - as such we only check for data types that are implemented - and if the type is anything else and not aligned to 32-bit, then we error out. Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- Cc: Ross Lagerwall <ross.lagerwall@citrix.com> Cc: Andrew Cooper <andrew.cooper3@citrix.com> Cc: George Dunlap <George.Dunlap@eu.citrix.com> Cc: Ian Jackson <ian.jackson@eu.citrix.com> Cc: Jan Beulich <jbeulich@suse.com> Cc: Stefano Stabellini <sstabellini@kernel.org> Cc: Tim Deegan <tim@xen.org> Cc: Wei Liu <wei.liu2@citrix.com> v1: First posting. v2: Redo the commit to include the commits which fix the alignment issues. Also mention the need in the docs v3: Change the docs to explicitly mention text code section alignment requirements. Invert arch_livepatch_verify_alignment return value (true for alignment is ok). Drop the alignment check in check_special_sections. Make the alignment check in check_section only for executable sections. Rewrote the commit message as it is not applicable to v2 of the patch anymore. v4: Also do the check on ARM64 Put () around the section check Use vaddr_t instead of uint32_t as that won't work on ARM64. --- docs/misc/livepatch.markdown | 2 ++ xen/arch/arm/arm32/livepatch.c | 13 +++++++++++-- xen/arch/arm/livepatch.c | 9 +++++++++ xen/arch/x86/livepatch.c | 6 ++++++ xen/common/livepatch.c | 7 +++++++ xen/include/xen/livepatch.h | 1 + 6 files changed, 36 insertions(+), 2 deletions(-)