.align may cause data to be interpreted as instructions

.align may cause data to be interpreted as instructions

[Taras Kondratiuk]

Hi

I was debugging kprobes-test for BE8 and noticed that some data fields
are stored in LE instead of BE. It happens because these data fields
get interpreted as instructions.

Is it a known issue?

For example:
test_align_fail_data:
	bx	lr
	.byte 0xaa
	.align
	.word 0x12345678

I would expect to see something like this:
00000000 <test_align_fail_data>:
   0:	e12fff1e 	bx	lr
   4:	aa          	.byte	0xaa
   5:	00          	.byte	0x00
   6:	0000      	.short	0x0000
   8:	12345678 	.word	0x12345678

But instead I have:
00000000 <test_align_fail_data>:
   0:	e12fff1e 	bx	lr
   4:	aa          	.byte	0xaa
   5:	00          	.byte	0x00
   6:	0000      	.short	0x0000
   8:	12345678 	eorsne	r5, r4, #120, 12	; 0x7800000

As a result the word 0x12345678 will be stored in LE.

I've run several tests and here are my observations:
- Double ".align" fixes the issue :)
- Behavior is the same for LE/BE, ARM/Thumb, GCC 4.4.1/4.6.x/4.8.2
- Size of alignment doesn't matter.
- Issue happens only if previous data is not instruction-aligned and
    0's are added before NOPs.
- Explicit filling with 0's (.align , 0) fixes the issue, but as a side
    effect data @0x4 is interpreted as a single ".word 0xaa000000"
    instead of ".byte .byte .short". I'm not sure if there can be any
    functional difference because of this.
- Issue doesn't happen if there is no instructions before data
  (no "bx lr" in the example).
- Issue doesn't happen if data after .align is defined as
    ".type <symbol>,%object".

-- 
Taras Kondratiuk

.align may cause data to be interpreted as instructions

[Ben Dooks]

On 15/10/13 23:38, Taras Kondratiuk wrote:
> Hi
>
> I was debugging kprobes-test for BE8 and noticed that some data fields
> are stored in LE instead of BE. It happens because these data fields
> get interpreted as instructions.
>
> Is it a known issue?

I reported the crashes to Tixy along with a different
method of sovling the problem (changed to using pointers to
the strings) a while ago. However it seems that nothing has
happened to fix this.

Since kprobes seems to work with the fixed tests I forgot
to follow up and prod Jon about looking into this problem.

Jon, if you are not interested in fixing this, then please
let me know and we can get a patch sorted to fix it.

PS, I am going to leave this out of the current be8 patchset
as I want to get that merged, and at the moment kprobes-test
is not essential to getting the system started.

> For example:
> test_align_fail_data:
> 	bx	lr
> 	.byte 0xaa
> 	.align
> 	.word 0x12345678
>
> I would expect to see something like this:
> 00000000<test_align_fail_data>:
>     0:	e12fff1e 	bx	lr
>     4:	aa          	.byte	0xaa
>     5:	00          	.byte	0x00
>     6:	0000      	.short	0x0000
>     8:	12345678 	.word	0x12345678
>
> But instead I have:
> 00000000<test_align_fail_data>:
>     0:	e12fff1e 	bx	lr
>     4:	aa          	.byte	0xaa
>     5:	00          	.byte	0x00
>     6:	0000      	.short	0x0000
>     8:	12345678 	eorsne	r5, r4, #120, 12	; 0x7800000
>
> As a result the word 0x12345678 will be stored in LE.
>
> I've run several tests and here are my observations:
> - Double ".align" fixes the issue :)
> - Behavior is the same for LE/BE, ARM/Thumb, GCC 4.4.1/4.6.x/4.8.2
> - Size of alignment doesn't matter.
> - Issue happens only if previous data is not instruction-aligned and
>      0's are added before NOPs.
> - Explicit filling with 0's (.align , 0) fixes the issue, but as a side
>      effect data @0x4 is interpreted as a single ".word 0xaa000000"
>      instead of ".byte .byte .short". I'm not sure if there can be any
>      functional difference because of this.
> - Issue doesn't happen if there is no instructions before data
>    (no "bx lr" in the example).
> - Issue doesn't happen if data after .align is defined as
>      ".type<symbol>,%object".

Thanks for getting down to a simple test case.

My view is to fix this by not doing complicated things by trying
to save a bit of space by embedding strings into the code. It is
not as if we cannot get the compiler to put the strings into the
relevant data area and give us a pointer we can use.

The code in this case is /not easy/ to follow and it would be nice
if it could be cleaned up to just take the string as a argument to
the test code instead of trying to find it via assembly magic.

-- 
Ben Dooks				http://www.codethink.co.uk/
Senior Engineer				Codethink - Providing Genius

.align may cause data to be interpreted as instructions

[Jon Medhurst (Tixy)]

On Wed, 2013-10-16 at 01:38 +0300, Taras Kondratiuk wrote:
> Hi
> 
> I was debugging kprobes-test for BE8 and noticed that some data fields
> are stored in LE instead of BE. It happens because these data fields
> get interpreted as instructions.
> 
> Is it a known issue?
> 
> For example:
> test_align_fail_data:
> 	bx	lr
> 	.byte 0xaa
> 	.align
> 	.word 0x12345678
> 
> I would expect to see something like this:
> 00000000 <test_align_fail_data>:
>    0:	e12fff1e 	bx	lr
>    4:	aa          	.byte	0xaa
>    5:	00          	.byte	0x00
>    6:	0000      	.short	0x0000
>    8:	12345678 	.word	0x12345678
> 
> But instead I have:
> 00000000 <test_align_fail_data>:
>    0:	e12fff1e 	bx	lr
>    4:	aa          	.byte	0xaa
>    5:	00          	.byte	0x00
>    6:	0000      	.short	0x0000
>    8:	12345678 	eorsne	r5, r4, #120, 12	; 0x7800000
> 
> As a result the word 0x12345678 will be stored in LE.

Hmm looks like a GCC bug, or we're doing something it considers
undefined behaviour?

> I've run several tests and here are my observations:
> - Double ".align" fixes the issue :)

Temping as a simple fix, but rather risky to rely on strange compiler
behaviour to fix another strange compiler error. :-)

> - Behavior is the same for LE/BE, ARM/Thumb, GCC 4.4.1/4.6.x/4.8.2

At least the compiler is being consistent and it's obviously an issue we
need to work around in the kernel.

> - Size of alignment doesn't matter.
> - Issue happens only if previous data is not instruction-aligned and
>     0's are added before NOPs.
> - Explicit filling with 0's (.align , 0) fixes the issue, but as a side
>     effect data @0x4 is interpreted as a single ".word 0xaa000000"
>     instead of ".byte .byte .short". I'm not sure if there can be any
>     functional difference because of this.
> - Issue doesn't happen if there is no instructions before data
>   (no "bx lr" in the example).
> - Issue doesn't happen if data after .align is defined as
>     ".type <symbol>,%object".

Seems like something which should be reported to the GCC people, and you
would think with the detailed diagnosis you've provided someone familiar
with the code would be able to quickly get to the bottom of it.

Meanwhile, I like the solution Den Dooks came up with, I'll comment
about that in my reply to his mail...

-- 
Tixy

.align may cause data to be interpreted as instructions

[Jon Medhurst (Tixy)]

On Wed, 2013-10-16 at 12:13 +0100, Ben Dooks wrote:
> On 15/10/13 23:38, Taras Kondratiuk wrote:
> > Hi
> >
> > I was debugging kprobes-test for BE8 and noticed that some data fields
> > are stored in LE instead of BE. It happens because these data fields
> > get interpreted as instructions.
> >
> > Is it a known issue?
> 
> I reported the crashes to Tixy along with a different
> method of sovling the problem (changed to using pointers to
> the strings) a while ago.

I found that fix in the list archives:
http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/186528.html

>  However it seems that nothing has
> happened to fix this.
>
> Since kprobes seems to work with the fixed tests I forgot
> to follow up and prod Jon about looking into this problem.

Sorry, I sorta forgot/ignored the issue, strange compiler problem with a
vague explanation on a big-endian kernel (who uses that ;-)

> Jon, if you are not interested in fixing this, then please
> let me know and we can get a patch sorted to fix it.

Looking at your old patch again, it looks good to me. So if someone
could post this to the lists again, with the commit message updated to
have a clearer explanation as to the symptoms, e.g. some of Taras'
analysis, that would be good. It should go to the list again to give
people another chance to comment...

Thanks

-- 
Tixy

.align may cause data to be interpreted as instructions

[Jon Medhurst (Tixy)]

On Wed, 2013-10-16 at 17:06 +0100, Jon Medhurst (Tixy) wrote:
> On Wed, 2013-10-16 at 12:13 +0100, Ben Dooks wrote:
> > On 15/10/13 23:38, Taras Kondratiuk wrote:
> > > Hi
> > >
> > > I was debugging kprobes-test for BE8 and noticed that some data fields
> > > are stored in LE instead of BE. It happens because these data fields
> > > get interpreted as instructions.
> > >
> > > Is it a known issue?
> > 
> > I reported the crashes to Tixy along with a different
> > method of sovling the problem (changed to using pointers to
> > the strings) a while ago.
> 
> I found that fix in the list archives:
> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/186528.html
> 
> >  However it seems that nothing has
> > happened to fix this.
> >
> > Since kprobes seems to work with the fixed tests I forgot
> > to follow up and prod Jon about looking into this problem.
> 
> Sorry, I sorta forgot/ignored the issue, strange compiler problem with a
> vague explanation on a big-endian kernel (who uses that ;-)
> 
> > Jon, if you are not interested in fixing this, then please
> > let me know and we can get a patch sorted to fix it.
> 
> Looking at your old patch again, it looks good to me. 

Actually it's broken for thumb, the pointer to the title needs storing
at a 4 byte alignment, which presumably will make the problem we're
trying to workaround re-occur. I'll try and find some time to take a
look for a different way of doing things...

-- 
Tixy

.align may cause data to be interpreted as instructions

[Taras Kondratiuk]

On 10/16/2013 10:25 PM, Dave Martin wrote:
> Unfortunately, objdump can and does get confused about data/instruction
> boundaries, so the output you see above may be misleading.
> 
> Displaying the symbol table with --special-syms will list the magic
> symbols that mark the instruction and data boundaries, to help debug
> this kind of situation.
> 
> 
> However, in this case, I think you've found a bug in the assembler,
> as shown below.
> 
> Before linking, the final $a symbol (indicating the start of ARM
> instructions) is at address 8, so in this case objdump is correct
> to show 0x12345678 as an instruction.
> 
> After linking, the mapping symbols ($[atd]) remain as before, and
> the linker has byteswapped this "instruction" (as it should).
> 
> This is likely related to the magic for inserting the extensible
> NOP-padding fragment which implements the .align in code sections.
> That is code, and requires a $a mapping symbol, but that somehow
> goes AWOL or gets displaced after the alignment padding ...
> 
> I can't quite get my head around what is going on in
> binutils/gas/config/tc-arm.c.  We would need to understand that
> before we can identify a reliable workaround.

Thanks for confirming the issue.
Does it makes sense to file a GCC bug?

.align may cause data to be interpreted as instructions

[Taras Kondratiuk]

On 10/16/2013 08:03 PM, Jon Medhurst (Tixy) wrote:
> On Wed, 2013-10-16 at 17:06 +0100, Jon Medhurst (Tixy) wrote:
>> On Wed, 2013-10-16 at 12:13 +0100, Ben Dooks wrote:
>>> On 15/10/13 23:38, Taras Kondratiuk wrote:
>>>> Hi
>>>>
>>>> I was debugging kprobes-test for BE8 and noticed that some data fields
>>>> are stored in LE instead of BE. It happens because these data fields
>>>> get interpreted as instructions.
>>>>
>>>> Is it a known issue?
>>>
>>> I reported the crashes to Tixy along with a different
>>> method of sovling the problem (changed to using pointers to
>>> the strings) a while ago.
>>
>> I found that fix in the list archives:
>> http://lists.infradead.org/pipermail/linux-arm-kernel/2013-July/186528.html
>>
>>>  However it seems that nothing has
>>> happened to fix this.
>>>
>>> Since kprobes seems to work with the fixed tests I forgot
>>> to follow up and prod Jon about looking into this problem.
>>
>> Sorry, I sorta forgot/ignored the issue, strange compiler problem with a
>> vague explanation on a big-endian kernel (who uses that ;-)
>>
>>> Jon, if you are not interested in fixing this, then please
>>> let me know and we can get a patch sorted to fix it.
>>
>> Looking at your old patch again, it looks good to me. 
> 
> Actually it's broken for thumb, the pointer to the title needs storing
> at a 4 byte alignment, which presumably will make the problem we're
> trying to workaround re-occur. I'll try and find some time to take a
> look for a different way of doing things...

Word alignment here doesn't cause the problem, so this patch should
workaround the issue.

.align may cause data to be interpreted as instructions

[Måns Rullgård]

Taras Kondratiuk <taras.kondratiuk@linaro.org> writes:

> On 10/16/2013 10:25 PM, Dave Martin wrote:
>> Unfortunately, objdump can and does get confused about data/instruction
>> boundaries, so the output you see above may be misleading.
>> 
>> Displaying the symbol table with --special-syms will list the magic
>> symbols that mark the instruction and data boundaries, to help debug
>> this kind of situation.
>> 
>> 
>> However, in this case, I think you've found a bug in the assembler,
>> as shown below.
>> 
>> Before linking, the final $a symbol (indicating the start of ARM
>> instructions) is at address 8, so in this case objdump is correct
>> to show 0x12345678 as an instruction.
>> 
>> After linking, the mapping symbols ($[atd]) remain as before, and
>> the linker has byteswapped this "instruction" (as it should).
>> 
>> This is likely related to the magic for inserting the extensible
>> NOP-padding fragment which implements the .align in code sections.
>> That is code, and requires a $a mapping symbol, but that somehow
>> goes AWOL or gets displaced after the alignment padding ...
>> 
>> I can't quite get my head around what is going on in
>> binutils/gas/config/tc-arm.c.  We would need to understand that
>> before we can identify a reliable workaround.
>
> Thanks for confirming the issue.
> Does it makes sense to file a GCC bug?

It seems like a binutils (gas) bug to me.

-- 
M?ns Rullg?rd
mans at mansr.com

.align may cause data to be interpreted as instructions

[Jon Medhurst (Tixy)]

On Wed, 2013-10-16 at 01:38 +0300, Taras Kondratiuk wrote:
> Hi
> 
> I was debugging kprobes-test for BE8 and noticed that some data fields
> are stored in LE instead of BE. It happens because these data fields
> get interpreted as instructions.
> 
> Is it a known issue?
> 
> For example:
> test_align_fail_data:
> 	bx	lr
> 	.byte 0xaa
> 	.align
> 	.word 0x12345678
> 
> I would expect to see something like this:
> 00000000 <test_align_fail_data>:
>    0:	e12fff1e 	bx	lr
>    4:	aa          	.byte	0xaa
>    5:	00          	.byte	0x00
>    6:	0000      	.short	0x0000
>    8:	12345678 	.word	0x12345678
> 
> But instead I have:
> 00000000 <test_align_fail_data>:
>    0:	e12fff1e 	bx	lr
>    4:	aa          	.byte	0xaa
>    5:	00          	.byte	0x00
>    6:	0000      	.short	0x0000
>    8:	12345678 	eorsne	r5, r4, #120, 12	; 0x7800000
> 
> As a result the word 0x12345678 will be stored in LE.
> 
> I've run several tests and here are my observations:
> - Double ".align" fixes the issue :)
> - Behavior is the same for LE/BE, ARM/Thumb, GCC 4.4.1/4.6.x/4.8.2
> - Size of alignment doesn't matter.
> - Issue happens only if previous data is not instruction-aligned and
>     0's are added before NOPs.
> - Explicit filling with 0's (.align , 0) fixes the issue, but as a side
>     effect data @0x4 is interpreted as a single ".word 0xaa000000"
>     instead of ".byte .byte .short". I'm not sure if there can be any
>     functional difference because of this.

After thinking about things overnight, I believe that this is the fix we
should go with. We want to stick alignment padding between data laid
down with .byte and .word so it makes sense to explicitly ask the
toolchain to pad with zeros rather than leaving it the opportunity to
get confused. (.align in the text section probably means it wants to
align with nops, but then sees the initial alignment and/or surrounding
statements look like binary data, not code, and then...)

I'll send a patch proposing that fix after I've worked out how to test
it on a big-endian kernel. Or if someone else sends a patch for that
with a good commit message that explains what's going on I'll happily
ack that.

-- 
Tixy

.align may cause data to be interpreted as instructions

[Taras Kondratiuk]

On 10/17/2013 03:17 PM, Jon Medhurst (Tixy) wrote:
> On Wed, 2013-10-16 at 01:38 +0300, Taras Kondratiuk wrote:
>> - Explicit filling with 0's (.align , 0) fixes the issue, but as a side
>>     effect data @0x4 is interpreted as a single ".word 0xaa000000"
>>     instead of ".byte .byte .short". I'm not sure if there can be any
>>     functional difference because of this.
> 
> After thinking about things overnight, I believe that this is the fix we
> should go with. We want to stick alignment padding between data laid
> down with .byte and .word so it makes sense to explicitly ask the
> toolchain to pad with zeros rather than leaving it the opportunity to
> get confused. (.align in the text section probably means it wants to
> align with nops, but then sees the initial alignment and/or surrounding
> statements look like binary data, not code, and then...)
> 
> I'll send a patch proposing that fix after I've worked out how to test
> it on a big-endian kernel. Or if someone else sends a patch for that
> with a good commit message that explains what's going on I'll happily
> ack that.

I have several fixes for BE Thumb kprobes on top of Ben's series.
".aling , 0" workaround is one of them.
All tests now pass for LE/BE and Thumb/ARM. I will clean up patches and
send them to Ben.

.align may cause data to be interpreted as instructions

[Jon Medhurst (Tixy)]

On Thu, 2013-10-17 at 13:55 +0100, Dave Martin wrote:
> On Thu, Oct 17, 2013 at 01:17:23PM +0100, Jon Medhurst (Tixy) wrote:
> > On Wed, 2013-10-16 at 01:38 +0300, Taras Kondratiuk wrote:
> > > Hi
> > > 
> > > I was debugging kprobes-test for BE8 and noticed that some data fields
> > > are stored in LE instead of BE. It happens because these data fields
> > > get interpreted as instructions.
> > > 
> > > Is it a known issue?
> > > 
> > > For example:
> > > test_align_fail_data:
> > > 	bx	lr
> > > 	.byte 0xaa
> > > 	.align
> > > 	.word 0x12345678
> > > 
> > > I would expect to see something like this:
> > > 00000000 <test_align_fail_data>:
> > >    0:	e12fff1e 	bx	lr
> > >    4:	aa          	.byte	0xaa
> > >    5:	00          	.byte	0x00
> > >    6:	0000      	.short	0x0000
> > >    8:	12345678 	.word	0x12345678
> > > 
> > > But instead I have:
> > > 00000000 <test_align_fail_data>:
> > >    0:	e12fff1e 	bx	lr
> > >    4:	aa          	.byte	0xaa
> > >    5:	00          	.byte	0x00
> > >    6:	0000      	.short	0x0000
> > >    8:	12345678 	eorsne	r5, r4, #120, 12	; 0x7800000
> > > 
> > > As a result the word 0x12345678 will be stored in LE.
> > > 
> > > I've run several tests and here are my observations:
> > > - Double ".align" fixes the issue :)
> > > - Behavior is the same for LE/BE, ARM/Thumb, GCC 4.4.1/4.6.x/4.8.2
> > > - Size of alignment doesn't matter.
> > > - Issue happens only if previous data is not instruction-aligned and
> > >     0's are added before NOPs.
> > > - Explicit filling with 0's (.align , 0) fixes the issue, but as a side
> > >     effect data @0x4 is interpreted as a single ".word 0xaa000000"
> > >     instead of ".byte .byte .short". I'm not sure if there can be any
> > >     functional difference because of this.
> > 
> > After thinking about things overnight, I believe that this is the fix we
> > should go with. We want to stick alignment padding between data laid
> > down with .byte and .word so it makes sense to explicitly ask the
> > toolchain to pad with zeros rather than leaving it the opportunity to
> > get confused. (.align in the text section probably means it wants to
> > align with nops, but then sees the initial alignment and/or surrounding
> > statements look like binary data, not code, and then...)
> 
> I believe this workaround will work.
> 
> Looking at the gas code, if .align is given an explicit fill value, the
> special NOP-padding code in gas is bypassed, and padding works just the
> same as it would in a data section.
> 
> Obviously, this only works in situations where the bytes emitted by
> .align are never executed by the CPU.
> 
> 
> > I'll send a patch proposing that fix after I've worked out how to test
> > it on a big-endian kernel. Or if someone else sends a patch for that
> > with a good commit message that explains what's going on I'll happily
> > ack that.
> 
> Disassemble-testing may be enough -- but I advise to dump the relevant
> code section with objdump -s as well as reading the code disassembly.
> The hex dump does not lie, whereas the disassembler does sometimes get
> confused in cases like this.
> 
> Building and disassembling in BE8 and LE configurations will exercise
> the two main cases (linker swabbing versus no swabbing) -- comparing
> the dump of vmlinux with the dump of the .o file shows what the linker
> is doing.

Thanks, that's the clue I was missing. I was looking at objdumps of .o
files and wasn't seeing any byte changes caused by this bug, just the
disassembler treating data as code. Doing a dump of the final linked
vmlinux I now see that exactly because the data is being treated as
code, the linking is swapping the byte order of the data, (because ARM
instructions are always little-endian and need to be fixed from their
big-endian representation in the .o file?).

In another branch of this thread, Taras said he had patches that were
tested, so I'll wait for those rather than doing one myself. Taras
should get the main credit for this investigation anyway :-)

-- 
Tixy

.align may cause data to be interpreted as instructions

[Taras Kondratiuk]

On 10/18/2013 02:03 PM, Jon Medhurst (Tixy) wrote:
> On Thu, 2013-10-17 at 13:55 +0100, Dave Martin wrote:
>> On Thu, Oct 17, 2013 at 01:17:23PM +0100, Jon Medhurst (Tixy) wrote:
>>> I'll send a patch proposing that fix after I've worked out how to test
>>> it on a big-endian kernel. Or if someone else sends a patch for that
>>> with a good commit message that explains what's going on I'll happily
>>> ack that.
>>
>> Disassemble-testing may be enough -- but I advise to dump the relevant
>> code section with objdump -s as well as reading the code disassembly.
>> The hex dump does not lie, whereas the disassembler does sometimes get
>> confused in cases like this.
>>
>> Building and disassembling in BE8 and LE configurations will exercise
>> the two main cases (linker swabbing versus no swabbing) -- comparing
>> the dump of vmlinux with the dump of the .o file shows what the linker
>> is doing.
> 
> Thanks, that's the clue I was missing. I was looking at objdumps of .o
> files and wasn't seeing any byte changes caused by this bug, just the
> disassembler treating data as code. Doing a dump of the final linked
> vmlinux I now see that exactly because the data is being treated as
> code, the linking is swapping the byte order of the data, (because ARM
> instructions are always little-endian and need to be fixed from their
> big-endian representation in the .o file?).
> 
> In another branch of this thread, Taras said he had patches that were
> tested, so I'll wait for those rather than doing one myself. Taras
> should get the main credit for this investigation anyway :-)
> 

I've just sent patches to Ben.

I was looking into gas code and found an opposite bug:
If NOP-padding is used to align data it can be treated as data
and saved in BE format.
So we definitely have to explicitly specify fill value for data
alignment in code section.

-- 
Taras Kondratiuk