| Message ID | 20170726194756.20265-4-konrad@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 26/07/2017 20:47, Konrad Rzeszutek Wilk wrote: > diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c > index 2247b925a0..7b36210ccd 100644 > --- a/xen/arch/arm/arm64/livepatch.c > +++ b/xen/arch/arm/arm64/livepatch.c > @@ -86,6 +86,12 @@ bool arch_livepatch_symbol_deny(const struct livepatch_elf *elf, > return false; > } > > +bool arch_livepatch_verify_alignment(const struct livepatch_elf_sec *sec) > +{ Semantically, "verify_alignment" implies "the alignment is correct", but the return value is the opposite. I'd recommend inverting the sense of these functions, returning true for x86/arm64, and == 0 for arm32... > + /* Unaligned access on ARM 64 is OK. */ > + return false; > +} > + > enum aarch64_reloc_op { > RELOC_OP_NONE, > RELOC_OP_ABS, > diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c > index 406eb910cc..b3cbdac9b7 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 false; > +} > + > 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 40ff6b3a27..13d8f25a4b 100644 > --- a/xen/common/livepatch.c > +++ b/xen/common/livepatch.c > @@ -472,6 +472,13 @@ static int check_section(const struct livepatch_elf *elf, > return -EINVAL; > } > > + if ( arch_livepatch_verify_alignment(sec) ) > + { > + dprintk(XENLOG_ERR, LIVEPATCH "%s: %s is not aligned properly!\n", > + elf->name, sec->name); It also means this would read as if ( !arch_livepatch_verify_alignment(sec) ) { "$blah not properly aligned" which is also the usual way around to think. (Also, bool functions and int functions really do have opposite success cases, and should be kept that way.) ~Andrew > + return -EINVAL; > + } > + > return 0; > } >
>>> 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. Jan
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).
>>> 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. Jan
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?
>>> On 07.09.17 at 19:36, <konrad@kernel.org> wrote: > 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. 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? I think what you want to detect simply can't be detected reliably, without risking false positives. Jan
On Fri, Sep 08, 2017 at 03:30:07AM -0600, Jan Beulich wrote: > >>> On 07.09.17 at 19:36, <konrad@kernel.org> wrote: > > 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. > > 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. > I think what you want to detect simply can't be detected reliably, > without risking false positives. > > Jan >
>>> On 09.09.17 at 14:05, <konrad@kernel.org> wrote: > On Fri, Sep 08, 2017 at 03:30:07AM -0600, Jan Beulich wrote: >> >>> On 07.09.17 at 19:36, <konrad@kernel.org> wrote: >> > 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. >> >> 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
On Mon, Sep 11, 2017 at 03:01:15AM -0600, Jan Beulich wrote: > >>> On 09.09.17 at 14:05, <konrad@kernel.org> wrote: > > On Fri, Sep 08, 2017 at 03:30:07AM -0600, Jan Beulich wrote: > >> >>> On 07.09.17 at 19:36, <konrad@kernel.org> wrote: > >> > 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. > >> > >> 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.
>>> On 12.09.17 at 02:22, <konrad@kernel.org> wrote: > On Mon, Sep 11, 2017 at 03:01:15AM -0600, Jan Beulich wrote: >> 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 >[...] > Keep in mind that this only happens if I cross-compile ARM32 under x86. That would suggest a build environment / build tools issue then: Cross builds aren't supposed to produce binaries different from native builds. > If I compile the test-case under ARM32 it works OK (as the > .livepatch.depends ends up being aligned to four bytes). So why is that? What entity is creating this section (or the directive(s) to create it)? Jan
On Tue, Sep 12, 2017 at 02:57:04AM -0600, Jan Beulich wrote: > >>> On 12.09.17 at 02:22, <konrad@kernel.org> wrote: > > On Mon, Sep 11, 2017 at 03:01:15AM -0600, Jan Beulich wrote: > >> 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 > >[...] > > Keep in mind that this only happens if I cross-compile ARM32 under x86. > > That would suggest a build environment / build tools issue then: > Cross builds aren't supposed to produce binaries different from > native builds. Hm, the gcc parameters on both native and cross compiler have same args: konrad@osstest:/srv/cubietruck/source$ diff native.invocation /tmp/cross.invocation 1c1 < gcc -marm -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes -Wdeclaration-after-statement -Wno-unused-but-set-variable -Wno-unused-local-typedefs -O1 -nostdinc -fno-builtin -fno-common -Werror -Wredundant-decls -Wno-pointer-arith -pipe -g -D__XEN__ -include /source/xen.orig.git/xen/include/xen/config.h '-D__OBJECT_FILE__="xen_bye_world.o"' -Wa,--strip-local-absolute -fno-omit-frame-pointer -MMD -MF ./.xen_bye_world.o.d -msoft-float -mcpu=cortex-a15 -I/source/xen.orig.git/xen/include -fno-stack-protector -fno-exceptions -Wnested-externs -DGCC_HAS_VISIBILITY_ATTRIBUTE -marm -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes -Wdeclaration-after-statement -Wno-unused-but-set-variable -Wno-unused-local-typedefs -c xen_bye_world.c -o xen_bye_world.o --- > arm-linux-gnu-gcc -marm -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes -Wdeclaration-after-statement -Wno-unused-but-set-variable -Wno-unused-local-typedefs -O1 -nostdinc -fno-builtin -fno-common -Werror -Wredundant-decls -Wno-pointer-arith -pipe -g -D__XEN__ -include /home/konrad/A20/xen.git/xen/include/xen/config.h '-D__OBJECT_FILE__="xen_bye_world.o"' -Wa,--strip-local-absolute -fno-omit-frame-pointer -MMD -MF ./.xen_bye_world.o.d -msoft-float -mcpu=cortex-a15 -I/home/konrad/A20/xen.git/xen/include -fno-stack-protector -fno-exceptions -Wnested-externs -DGCC_HAS_VISIBILITY_ATTRIBUTE -marm -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes -Wdeclaration-after-statement -Wno-unused-but-set-variable -Wno-unused-local-typedefs -c xen_bye_world.c -o xen_bye_world.o > > > If I compile the test-case under ARM32 it works OK (as the > > .livepatch.depends ends up being aligned to four bytes). > > So why is that? What entity is creating this section (or the > directive(s) to create it)? gcc Looking at the xen_bye_world.o produced by cross-compiler: xen_bye_world.o: file format elf32-littlearm Contents of section .rodata: 0000 78656e5f 65787472 615f7665 7273696f xen_extra_versio 0010 6e00 n. And native: xen_bye_world.o: file format elf32-littlearm Contents of section .rodata: 0000 78656e5f 65787472 615f7665 7273696f xen_extra_versio 0010 6e000000 n... (The cross compiler is 7.0.1, while native is 4.9.2).
>>> On 18.09.17 at 21:37, <konrad.wilk@oracle.com> wrote: > On Tue, Sep 12, 2017 at 02:57:04AM -0600, Jan Beulich wrote: >> >>> On 12.09.17 at 02:22, <konrad@kernel.org> wrote: >> > If I compile the test-case under ARM32 it works OK (as the >> > .livepatch.depends ends up being aligned to four bytes). >> >> So why is that? What entity is creating this section (or the >> directive(s) to create it)? > > gcc > > Looking at the xen_bye_world.o produced by cross-compiler: > > xen_bye_world.o: file format elf32-littlearm > > Contents of section .rodata: > 0000 78656e5f 65787472 615f7665 7273696f xen_extra_versio > 0010 6e00 n. > > And native: > > xen_bye_world.o: file format elf32-littlearm > > Contents of section .rodata: > 0000 78656e5f 65787472 615f7665 7273696f xen_extra_versio > 0010 6e000000 n... This may rather be a gas than a gcc behavioral difference. What's the alignment of .rodata in both cases? Jan
On Tue, Sep 19, 2017 at 09:04:44AM -0600, Jan Beulich wrote: > >>> On 18.09.17 at 21:37, <konrad.wilk@oracle.com> wrote: > > On Tue, Sep 12, 2017 at 02:57:04AM -0600, Jan Beulich wrote: > >> >>> On 12.09.17 at 02:22, <konrad@kernel.org> wrote: > >> > If I compile the test-case under ARM32 it works OK (as the > >> > .livepatch.depends ends up being aligned to four bytes). > >> > >> So why is that? What entity is creating this section (or the > >> directive(s) to create it)? > > > > gcc > > > > Looking at the xen_bye_world.o produced by cross-compiler: > > > > xen_bye_world.o: file format elf32-littlearm > > > > Contents of section .rodata: > > 0000 78656e5f 65787472 615f7665 7273696f xen_extra_versio > > 0010 6e00 n. > > > > And native: > > > > xen_bye_world.o: file format elf32-littlearm > > > > Contents of section .rodata: > > 0000 78656e5f 65787472 615f7665 7273696f xen_extra_versio > > 0010 6e000000 n... > > This may rather be a gas than a gcc behavioral difference. What's > the alignment of .rodata in both cases? Cross: * on the livepatch: ..snip.. [ 4] .rodata PROGBITS 00000000 000074 000012 00 A 0 0 4 [ 5] .rodata.str1.4 PROGBITS 00000000 000088 00000b 01 AMS 0 0 4 [ 6] .livepatch.depend PROGBITS 00000000 000093 000024 00 A 0 0 1 * on the .o file: Section Headers: [Nr] Name Type Addr Off Size ES Flg Lk Inf Al .. snip.. [ 1] .text PROGBITS 00000000 000034 000000 00 AX 0 0 1 [ 2] .data PROGBITS 00000000 000034 000000 00 WA 0 0 1 [ 3] .bss NOBITS 00000000 000034 000000 00 WA 0 0 1 [ 4] .rodata PROGBITS 00000000 000034 000014 00 A 0 0 4 [ 5] .livepatch.funcs PROGBITS 00000000 000048 000034 00 WA 0 0 4 Native: * on the livepatch: ..snip.. [ 4] .rodata PROGBITS 00000000 000074 000014 00 A 0 0 4 [ 5] .rodata.str1.4 PROGBITS 00000000 000088 00000c 01 AMS 0 0 4 [ 6] .livepatch.depend PROGBITS 00000000 000094 000024 00 A 0 0 1 * on the .o file: ..snip.. [ 1] .text PROGBITS 00000000 000034 000000 00 AX 0 0 1 [ 2] .data PROGBITS 00000000 000034 000000 00 WA 0 0 1 [ 3] .bss NOBITS 00000000 000034 000000 00 WA 0 0 1 [ 4] .rodata PROGBITS 00000000 000034 000012 00 A 0 0 4 [ 5] .livepatch.funcs PROGBITS 00000000 000048 000034 00 WA 0 0 4
>>> On 20.09.17 at 17:12, <konrad@kernel.org> wrote: > On Tue, Sep 19, 2017 at 09:04:44AM -0600, Jan Beulich wrote: >> >>> On 18.09.17 at 21:37, <konrad.wilk@oracle.com> wrote: >> > On Tue, Sep 12, 2017 at 02:57:04AM -0600, Jan Beulich wrote: >> >> >>> On 12.09.17 at 02:22, <konrad@kernel.org> wrote: >> >> > If I compile the test-case under ARM32 it works OK (as the >> >> > .livepatch.depends ends up being aligned to four bytes). >> >> >> >> So why is that? What entity is creating this section (or the >> >> directive(s) to create it)? >> > >> > gcc >> > >> > Looking at the xen_bye_world.o produced by cross-compiler: >> > >> > xen_bye_world.o: file format elf32-littlearm >> > >> > Contents of section .rodata: >> > 0000 78656e5f 65787472 615f7665 7273696f xen_extra_versio >> > 0010 6e00 n. >> > >> > And native: >> > >> > xen_bye_world.o: file format elf32-littlearm >> > >> > Contents of section .rodata: >> > 0000 78656e5f 65787472 615f7665 7273696f xen_extra_versio >> > 0010 6e000000 n... >> >> This may rather be a gas than a gcc behavioral difference. What's >> the alignment of .rodata in both cases? > > Cross: > > * on the livepatch: > ..snip.. > [ 4] .rodata PROGBITS 00000000 000074 000012 00 A 0 0 4 > [ 5] .rodata.str1.4 PROGBITS 00000000 000088 00000b 01 AMS 0 0 4 > [ 6] .livepatch.depend PROGBITS 00000000 000093 000024 00 A 0 0 1 > > * on the .o file: > Section Headers: > [Nr] Name Type Addr Off Size ES Flg Lk Inf Al > .. snip.. > [ 1] .text PROGBITS 00000000 000034 000000 00 AX 0 0 1 > [ 2] .data PROGBITS 00000000 000034 000000 00 WA 0 0 1 > [ 3] .bss NOBITS 00000000 000034 000000 00 WA 0 0 1 > [ 4] .rodata PROGBITS 00000000 000034 000014 00 A 0 0 4 > [ 5] .livepatch.funcs PROGBITS 00000000 000048 000034 00 WA 0 0 4 Hard to believe - a 0x14 bytes section gets shrunk to 0x12 bytes by (presumably) ld -r? > Native: > > * on the livepatch: > ..snip.. > [ 4] .rodata PROGBITS 00000000 000074 000014 00 A 0 0 4 > [ 5] .rodata.str1.4 PROGBITS 00000000 000088 00000c 01 AMS 0 0 4 > [ 6] .livepatch.depend PROGBITS 00000000 000094 000024 00 A 0 0 1 > > * on the .o file: > ..snip.. > [ 1] .text PROGBITS 00000000 000034 000000 00 AX 0 0 1 > [ 2] .data PROGBITS 00000000 000034 000000 00 WA 0 0 1 > [ 3] .bss NOBITS 00000000 000034 000000 00 WA 0 0 1 > [ 4] .rodata PROGBITS 00000000 000034 000012 00 A 0 0 4 > [ 5] .livepatch.funcs PROGBITS 00000000 000048 000034 00 WA 0 0 4 With things being the other way around here - did you perhaps mix up files? Jan
diff --git a/docs/misc/livepatch.markdown b/docs/misc/livepatch.markdown index 54a6b850cb..c2c4acd3d3 100644 --- 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. + 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. @@ -341,7 +345,7 @@ The size of the structure is 64 bytes on 64-bit hypervisors. It will be * `opaque` **MUST** be zero. The size of the `livepatch_func` array is determined from the ELF section -size. +size. On ARM32 this section must by four-byte aligned. When applying the patch the hypervisor iterates over each `livepatch_func` structure and the core code inserts a trampoline at `old_addr` to `new_addr`. diff --git a/xen/arch/arm/arm32/livepatch.c b/xen/arch/arm/arm32/livepatch.c index 41378a54ae..d48820f8ba 100644 --- a/xen/arch/arm/arm32/livepatch.c +++ b/xen/arch/arm/arm32/livepatch.c @@ -112,6 +112,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 ARM 32 will crash with Data Abort. */ + return (uint32_t)sec->load_addr % sizeof(uint32_t); +}; + static s32 get_addend(unsigned char type, void *dest) { s32 addend = 0; @@ -266,6 +272,15 @@ int arch_livepatch_perform(struct livepatch_elf *elf, elf->name, symndx); return -EINVAL; } + + if ( !use_rela ) + addend = get_addend(type, dest); + else if ( (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; + } else if ( !elf->sym[symndx].sym ) { dprintk(XENLOG_ERR, LIVEPATCH "%s: No relative symbol@%u\n", diff --git a/xen/arch/arm/arm64/livepatch.c b/xen/arch/arm/arm64/livepatch.c index 2247b925a0..7b36210ccd 100644 --- a/xen/arch/arm/arm64/livepatch.c +++ b/xen/arch/arm/arm64/livepatch.c @@ -86,6 +86,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 ARM 64 is OK. */ + return false; +} + enum aarch64_reloc_op { RELOC_OP_NONE, RELOC_OP_ABS, diff --git a/xen/arch/x86/livepatch.c b/xen/arch/x86/livepatch.c index 406eb910cc..b3cbdac9b7 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 false; +} + 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 40ff6b3a27..13d8f25a4b 100644 --- a/xen/common/livepatch.c +++ b/xen/common/livepatch.c @@ -472,6 +472,13 @@ static int check_section(const struct livepatch_elf *elf, return -EINVAL; } + if ( arch_livepatch_verify_alignment(sec) ) + { + dprintk(XENLOG_ERR, LIVEPATCH "%s: %s is not aligned properly!\n", + elf->name, sec->name); + return -EINVAL; + } + return 0; } @@ -501,7 +508,12 @@ static int check_special_sections(const struct livepatch_elf *elf) elf->name, names[i]); return -EINVAL; } - + if ( arch_livepatch_verify_alignment(sec) ) + { + dprintk(XENLOG_ERR, LIVEPATCH "%s: %s is not aligned properly!\n", + elf->name, names[i]); + return -EINVAL; + } if ( test_and_set_bit(i, found) ) { dprintk(XENLOG_ERR, LIVEPATCH "%s: %s was seen more than once!\n", 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) {
This issue was observed on ARM32 with a cross compiler for the livepatches. Mainly the livepatches .data section size was not padded 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 the next section would be put right behind 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. See patch titled "alternative/x86/arm32: Align altinstructions (and altinstr_replacement) sections." which fixes the .altinstructions to have the proper alignment. 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. This allows us on ARM32 to error out with: livepatch: xen_hello_world: dest=00a0404a (.altinstructions) is not aligned properly! Furthermore we also observe that the alignment is not correct for other sections (when cross compiling) as such we add the check in various other places which allows us to get. livepatch: xen_bye_world: .livepatch.depends alignment is wrong! instead of a crash. (See patch "xen/livepatch/x86/arm32: Force livepatch_depends to be uint32_t aligned" which fixes the .livepatch.depends alignment) Signed-off-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> --- v1: Initial patch v2: Redo the commit to include the commits which fix the alignment issues. Also mention the need in the docs --- docs/misc/livepatch.markdown | 6 +++++- xen/arch/arm/arm32/livepatch.c | 15 +++++++++++++++ xen/arch/arm/arm64/livepatch.c | 6 ++++++ xen/arch/x86/livepatch.c | 6 ++++++ xen/common/livepatch.c | 14 +++++++++++++- xen/include/xen/livepatch.h | 1 + 6 files changed, 46 insertions(+), 2 deletions(-)