Re: [Xen-devel] [PATCH v2 3/5] xen/livepatch/ARM32: Don't load and crash on livepatches loaded with wrong alignment.
>>> On 20.09.17 at 17:12, wrote: > On Tue, Sep 19, 2017 at 09:04:44AM -0600, Jan Beulich wrote: >> >>> On 18.09.17 at 21:37, wrote: >> > On Tue, Sep 12, 2017 at 02:57:04AM -0600, Jan Beulich wrote: >> >> >>> On 12.09.17 at 02:22, 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: >> > 78656e5f 65787472 615f7665 7273696f xen_extra_versio >> > 0010 6e00 n. >> > >> > And native: >> > >> > xen_bye_world.o: file format elf32-littlearm >> > >> > Contents of section .rodata: >> > 78656e5f 65787472 615f7665 7273696f xen_extra_versio >> > 0010 6e00 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 74 12 00 A 0 0 > 4 > [ 5] .rodata.str1.4PROGBITS 88 0b 01 AMS 0 0 > 4 > [ 6] .livepatch.depend PROGBITS 93 24 00 A 0 0 > 1 > > * on the .o file: > Section Headers: > [Nr] Name TypeAddr OffSize ES Flg Lk Inf > Al > .. snip.. > [ 1] .text PROGBITS 34 00 00 AX 0 0 > 1 > [ 2] .data PROGBITS 34 00 00 WA 0 0 > 1 > [ 3] .bss NOBITS 34 00 00 WA 0 0 > 1 > [ 4] .rodata PROGBITS 34 14 00 A 0 0 > 4 > [ 5] .livepatch.funcs PROGBITS 48 34 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 74 14 00 A 0 0 > 4 > [ 5] .rodata.str1.4PROGBITS 88 0c 01 AMS 0 0 > 4 > [ 6] .livepatch.depend PROGBITS 94 24 00 A 0 0 > 1 > > * on the .o file: > ..snip.. > [ 1] .text PROGBITS 34 00 00 AX 0 0 > 1 > [ 2] .data PROGBITS 34 00 00 WA 0 0 > 1 > [ 3] .bss NOBITS 34 00 00 WA 0 0 > 1 > [ 4] .rodata PROGBITS 34 12 00 A 0 0 > 4 > [ 5] .livepatch.funcs PROGBITS 48 34 00 WA 0 0 > 4 With things being the other way around here - did you perhaps mix up files? Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 3/5] xen/livepatch/ARM32: Don't load and crash on livepatches loaded with wrong alignment.
On Tue, Sep 19, 2017 at 09:04:44AM -0600, Jan Beulich wrote: > >>> On 18.09.17 at 21:37, wrote: > > On Tue, Sep 12, 2017 at 02:57:04AM -0600, Jan Beulich wrote: > >> >>> On 12.09.17 at 02:22, 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: > > 78656e5f 65787472 615f7665 7273696f xen_extra_versio > > 0010 6e00 n. > > > > And native: > > > > xen_bye_world.o: file format elf32-littlearm > > > > Contents of section .rodata: > > 78656e5f 65787472 615f7665 7273696f xen_extra_versio > > 0010 6e00 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 74 12 00 A 0 0 4 [ 5] .rodata.str1.4PROGBITS 88 0b 01 AMS 0 0 4 [ 6] .livepatch.depend PROGBITS 93 24 00 A 0 0 1 * on the .o file: Section Headers: [Nr] Name TypeAddr OffSize ES Flg Lk Inf Al .. snip.. [ 1] .text PROGBITS 34 00 00 AX 0 0 1 [ 2] .data PROGBITS 34 00 00 WA 0 0 1 [ 3] .bss NOBITS 34 00 00 WA 0 0 1 [ 4] .rodata PROGBITS 34 14 00 A 0 0 4 [ 5] .livepatch.funcs PROGBITS 48 34 00 WA 0 0 4 Native: * on the livepatch: ..snip.. [ 4] .rodata PROGBITS 74 14 00 A 0 0 4 [ 5] .rodata.str1.4PROGBITS 88 0c 01 AMS 0 0 4 [ 6] .livepatch.depend PROGBITS 94 24 00 A 0 0 1 * on the .o file: ..snip.. [ 1] .text PROGBITS 34 00 00 AX 0 0 1 [ 2] .data PROGBITS 34 00 00 WA 0 0 1 [ 3] .bss NOBITS 34 00 00 WA 0 0 1 [ 4] .rodata PROGBITS 34 12 00 A 0 0 4 [ 5] .livepatch.funcs PROGBITS 48 34 00 WA 0 0 4 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 3/5] xen/livepatch/ARM32: Don't load and crash on livepatches loaded with wrong alignment.
>>> On 18.09.17 at 21:37, wrote: > On Tue, Sep 12, 2017 at 02:57:04AM -0600, Jan Beulich wrote: >> >>> On 12.09.17 at 02:22, 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: > 78656e5f 65787472 615f7665 7273696f xen_extra_versio > 0010 6e00 n. > > And native: > > xen_bye_world.o: file format elf32-littlearm > > Contents of section .rodata: > 78656e5f 65787472 615f7665 7273696f xen_extra_versio > 0010 6e00 n... This may rather be a gas than a gcc behavioral difference. What's the alignment of .rodata in both cases? Jan ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 3/5] xen/livepatch/ARM32: Don't load and crash on livepatches loaded with wrong alignment.
On Tue, Sep 12, 2017 at 02:57:04AM -0600, Jan Beulich wrote: > >>> On 12.09.17 at 02:22, 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: 78656e5f 65787472 615f7665 7273696f xen_extra_versio 0010 6e00 n. And native: xen_bye_world.o: file format elf32-littlearm Contents of section .rodata: 78656e5f 65787472 615f7665 7273696f xen_extra_versio 0010 6e00 n... (The cross compiler is 7.0.1, while native is 4.9.2). ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 3/5] xen/livepatch/ARM32: Don't load and crash on livepatches loaded with wrong alignment.
>>> On 12.09.17 at 02:22, 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 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 3/5] xen/livepatch/ARM32: Don't load and crash on livepatches loaded with wrong alignment.
On Mon, Sep 11, 2017 at 03:01:15AM -0600, Jan Beulich wrote: > >>> On 09.09.17 at 14:05, wrote: > > On Fri, Sep 08, 2017 at 03:30:07AM -0600, Jan Beulich wrote: > >> >>> On 07.09.17 at 19:36, wrote: > >> > On Wed, Aug 02, 2017 at 03:20:05AM -0600, Jan Beulich wrote: > >> >> >>> Konrad Rzeszutek Wilk 07/31/17 6:04 PM >>> > >> >> >On Mon, Jul 31, 2017 at 07:55:34AM -0600, Jan Beulich wrote: > >> >> >> >>> Konrad Rzeszutek Wilk 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 0070 > >> >00ca AX 0 0 16 > >> > [ 4] .altinstr_replace PROGBITS 013c > >> >000b AX 0 0 4 > >> > [ 5] .fixupPROGBITS 0147 > >> >000d 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 00a080
Re: [Xen-devel] [PATCH v2 3/5] xen/livepatch/ARM32: Don't load and crash on livepatches loaded with wrong alignment.
>>> On 09.09.17 at 14:05, wrote: > On Fri, Sep 08, 2017 at 03:30:07AM -0600, Jan Beulich wrote: >> >>> On 07.09.17 at 19:36, wrote: >> > On Wed, Aug 02, 2017 at 03:20:05AM -0600, Jan Beulich wrote: >> >> >>> Konrad Rzeszutek Wilk 07/31/17 6:04 PM >>> >> >> >On Mon, Jul 31, 2017 at 07:55:34AM -0600, Jan Beulich wrote: >> >> >> >>> Konrad Rzeszutek Wilk 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 0070 >> >00ca AX 0 0 16 >> > [ 4] .altinstr_replace PROGBITS 013c >> >000b AX 0 0 4 >> > [ 5] .fixupPROGBITS 0147 >> >000d 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@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 3/5] xen/livepatch/ARM32: Don't load and crash on livepatches loaded with wrong alignment.
On Fri, Sep 08, 2017 at 03:30:07AM -0600, Jan Beulich wrote: > >>> On 07.09.17 at 19:36, wrote: > > On Wed, Aug 02, 2017 at 03:20:05AM -0600, Jan Beulich wrote: > >> >>> Konrad Rzeszutek Wilk 07/31/17 6:04 PM >>> > >> >On Mon, Jul 31, 2017 at 07:55:34AM -0600, Jan Beulich wrote: > >> >> >>> Konrad Rzeszutek Wilk 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 0070 > >00ca AX 0 0 16 > > [ 4] .altinstr_replace PROGBITS 013c > >000b AX 0 0 4 > > [ 5] .fixupPROGBITS 0147 > >000d 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 > ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 3/5] xen/livepatch/ARM32: Don't load and crash on livepatches loaded with wrong alignment.
>>> On 07.09.17 at 19:36, wrote: > On Wed, Aug 02, 2017 at 03:20:05AM -0600, Jan Beulich wrote: >> >>> Konrad Rzeszutek Wilk 07/31/17 6:04 PM >>> >> >On Mon, Jul 31, 2017 at 07:55:34AM -0600, Jan Beulich wrote: >> >> >>> Konrad Rzeszutek Wilk 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 0070 >00ca AX 0 0 16 > [ 4] .altinstr_replace PROGBITS 013c >000b AX 0 0 4 > [ 5] .fixupPROGBITS 0147 >000d 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 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 3/5] xen/livepatch/ARM32: Don't load and crash on livepatches loaded with wrong alignment.
On Wed, Aug 02, 2017 at 03:20:05AM -0600, Jan Beulich wrote: > >>> Konrad Rzeszutek Wilk 07/31/17 6:04 PM >>> > >On Mon, Jul 31, 2017 at 07:55:34AM -0600, Jan Beulich wrote: > >> >>> Konrad Rzeszutek Wilk 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 0070 00ca AX 0 0 16 [ 4] .altinstr_replace PROGBITS 013c 000b AX 0 0 4 [ 5] .fixupPROGBITS 0147 000d 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
Re: [Xen-devel] [PATCH v2 3/5] xen/livepatch/ARM32: Don't load and crash on livepatches loaded with wrong alignment.
>>> Konrad Rzeszutek Wilk 07/31/17 6:04 PM >>> >On Mon, Jul 31, 2017 at 07:55:34AM -0600, Jan Beulich wrote: >> >>> Konrad Rzeszutek Wilk 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 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 3/5] xen/livepatch/ARM32: Don't load and crash on livepatches loaded with wrong alignment.
On Mon, Jul 31, 2017 at 07:55:34AM -0600, Jan Beulich wrote: > >>> Konrad Rzeszutek Wilk 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). ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 3/5] xen/livepatch/ARM32: Don't load and crash on livepatches loaded with wrong alignment.
>>> Konrad Rzeszutek Wilk 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 ___ Xen-devel mailing list Xen-devel@lists.xen.org https://lists.xen.org/xen-devel
Re: [Xen-devel] [PATCH v2 3/5] xen/livepatch/ARM32: Don't load and crash on livepatches loaded with wrong alignment.
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;
> }
>
___
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel
