[PATCH v2] ARM: head-common.S: relocate atags area if necessary

[PATCH v2] ARM: head-common.S: relocate atags area if necessary

[Daniel Mack]

If the supplied atags/dtb pointer is located at memory inside the bss
section, it will be erased by __mmap_switched. The problem is that the
code that sets up the pointer can't know about a safe value unless it
examines the kernel's symbol tables, so we should care about that case
and relocate the area if necessary.

This patch does that from inside __vet_atags. In order to determine the
size of the section in dtb cases, it reads the next word after the dtb
binary magic, and also has to convert that value from big to CPU
endianess. For the atags case, a total size of up to 4k is assumed for
now.

Signed-off-by: Daniel Mack <zonque@gmail.com>
---
v2: move the relocation destination to '.end + atags_size' instead of
    '.end', because the atags area could actually overlap the .end
    pointer, which then would cause the relocation to fail.

 arch/arm/kernel/head-common.S | 58 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 52 insertions(+), 6 deletions(-)

diff --git a/arch/arm/kernel/head-common.S b/arch/arm/kernel/head-common.S
index 47cd974..5434767 100644
--- a/arch/arm/kernel/head-common.S
+++ b/arch/arm/kernel/head-common.S
@@ -36,12 +36,18 @@
  * that the pointer be aligned, in the first 16k of physical RAM and
  * that the ATAG_CORE marker is first and present.  If CONFIG_OF_FLATTREE
  * is selected, then it will also accept a dtb pointer.  Future revisions
- * of this function may be more lenient with the physical address and
- * may also be able to move the ATAGS block if necessary.
+ * of this function may be more lenient with the physical address.
+ *
+ * It is also checked whether the atags/dtb area is located before the
+ * end of the kernel's bss section and would hence be overridden by zeros
+ * later. In that case, the atags area is relocated to the '_end' symbol.
+ *
+ * r2 = atags or dtb
+ * r8 = phys_offset
  *
  * Returns:
  *  r2 either valid atags pointer, valid dtb pointer, or zero
- *  r5, r6 corrupted
+ *  r3, r5 - r7 corrupted
  */
 __vet_atags:
 	tst	r2, #0x3			@ aligned?
@@ -51,21 +57,61 @@ __vet_atags:
 #ifdef CONFIG_OF_FLATTREE
 	ldr	r6, =OF_DT_MAGIC		@ is it a DTB?
 	cmp	r5, r6
-	beq	2f
-#endif
-	cmp	r5, #ATAG_CORE_SIZE		@ is first tag ATAG_CORE?
+	bne	5f
+
+	ldreq	r5, [r2, #4]			@ fdt total size is at offset 4 ...
+#ifndef CONFIG_CPU_BIG_ENDIAN
+	eor	r6, r5, r5, ror #16		@ ... and stored in be32 order
+	mov	r6, r6, lsr #8
+	bic	r6, r6, #0xff00
+	eor	r5, r6, r5, ror #8
+#endif /* !CONFIG_CPU_BIG_ENDIAN */
+
+	add	r5, r5, #4			@ align the size to 32bit
+	bic	r5, r5, #3
+	b	4f
+#endif /* CONFIG_OF_FLATTREE */
+
+5:	cmp	r5, #ATAG_CORE_SIZE		@ is first tag ATAG_CORE?
 	cmpne	r5, #ATAG_CORE_SIZE_EMPTY
 	bne	1f
 	ldr	r5, [r2, #4]
 	ldr	r6, =ATAG_CORE
 	cmp	r5, r6
+	movne	r5, #4096			@ FIXME: we should walk the atags and
+						@ determine the real size.
 	bne	1f
 
+4:	adr	r3, 6f
+	ldmia	r3!, {r6, r7}
+
+	@ The kernel end address is stored in virtual address space, but we're
+	@ still in flat mapping. Hence, we have to do virt_to_phys() manually.
+	subs	r6, r6, r7			@ r7 = PAGE_OFFSET
+	add	r6, r6, r8			@ r8 = PHYS_OFFET
+
+	cmp	r2, r6				@ is the atags pointer inside the
+						@ kernel area?
+	bgt	2f
+
+	add	r3, r6, r5			@ relocate start = .end + length
+	add	r6, r3, r5			@ relocate end = .end + length * 2
+3:	cmp	r3, r6
+	ldrne	fp, [r2], #4
+	strne	fp, [r3], #4
+	bne	3b
+
+	subs	r2, r6, r5			@ rewind back to the new
+						@ atags/dtb image start
+
 2:	mov	pc, lr				@ atag/dtb pointer is ok
 
 1:	mov	r2, #0
 	mov	pc, lr
 ENDPROC(__vet_atags)
+	.align
+6:	.long	_end				@ r6
+	.long	PAGE_OFFSET			@ r7
 
 /*
  * The following fragment of code is executed with the MMU on in MMU mode,
-- 
1.8.3.1

[PATCH v2] ARM: head-common.S: relocate atags area if necessary

[Russell King - ARM Linux]

On Fri, Oct 18, 2013 at 06:12:56PM +0200, Daniel Mack wrote:
> If the supplied atags/dtb pointer is located at memory inside the bss
> section, it will be erased by __mmap_switched. The problem is that the
> code that sets up the pointer can't know about a safe value unless it
> examines the kernel's symbol tables, so we should care about that case
> and relocate the area if necessary.
> 
> This patch does that from inside __vet_atags. In order to determine the
> size of the section in dtb cases, it reads the next word after the dtb
> binary magic, and also has to convert that value from big to CPU
> endianess. For the atags case, a total size of up to 4k is assumed for
> now.

I'm not convinced that this is a good solution.  If this oerlaps the
BSS region, it could well end up being overlapped by something else
more serious, like the data segment - at which point doing the fixup
here means we've already lost.

We already give the decompressor the size of the kernel's BSS and take
action if we believe that the kernel's BSS will overlap the DTB/ATAGs
there.

[PATCH v2] ARM: head-common.S: relocate atags area if necessary

[Daniel Mack]

Hi Russell,

thanks for having a look.

On 10/18/2013 06:29 PM, Russell King - ARM Linux wrote:
> On Fri, Oct 18, 2013 at 06:12:56PM +0200, Daniel Mack wrote:
>> If the supplied atags/dtb pointer is located at memory inside the bss
>> section, it will be erased by __mmap_switched. The problem is that the
>> code that sets up the pointer can't know about a safe value unless it
>> examines the kernel's symbol tables, so we should care about that case
>> and relocate the area if necessary.
>>
>> This patch does that from inside __vet_atags. In order to determine the
>> size of the section in dtb cases, it reads the next word after the dtb
>> binary magic, and also has to convert that value from big to CPU
>> endianess. For the atags case, a total size of up to 4k is assumed for
>> now.
> 
> I'm not convinced that this is a good solution.  If this oerlaps the
> BSS region, it could well end up being overlapped by something else
> more serious, like the data segment

My v1 had a cover letter, don't know if you've seen it. In that, I
describe that the setup I see that faulty condition is caused by kexec,
which currently assumes that the decompressed kernel size is max. 4
times bigger than the zImage it executes. While that is naive, it seems
to be an assumption that reflects reality in my tests.

Correct me if I'm wrong, but AFAIK the .bss section is the only thing
that can grow to arbitrary size at runtime without accounting to the
compressed image size; so in order to fix kernels executed by kexec that
way, my fixup seems reasonable, doesn't it?

> - at which point doing the fixup
> here means we've already lost.

The question is whether it does any harm to at least try to fix up
things in that situation.

> We already give the decompressor the size of the kernel's BSS and take
> action if we believe that the kernel's BSS will overlap the DTB/ATAGs
> there.

Only for CONFIG_ARM_ATAG_DTB_COMPAT (which is unset in my config),
right? I've read through the sources relevant for my system (AM33xx),
and couldn't find any other place where my case is covered. The
relocation I propose does fix it, though.


Thanks,
Daniel

[PATCH v2] ARM: head-common.S: relocate atags area if necessary

[Russell King - ARM Linux]

On Fri, Oct 18, 2013 at 07:09:42PM +0200, Daniel Mack wrote:
> On 10/18/2013 06:29 PM, Russell King - ARM Linux wrote:
> > On Fri, Oct 18, 2013 at 06:12:56PM +0200, Daniel Mack wrote:
> >> If the supplied atags/dtb pointer is located at memory inside the bss
> >> section, it will be erased by __mmap_switched. The problem is that the
> >> code that sets up the pointer can't know about a safe value unless it
> >> examines the kernel's symbol tables, so we should care about that case
> >> and relocate the area if necessary.
> >>
> >> This patch does that from inside __vet_atags. In order to determine the
> >> size of the section in dtb cases, it reads the next word after the dtb
> >> binary magic, and also has to convert that value from big to CPU
> >> endianess. For the atags case, a total size of up to 4k is assumed for
> >> now.
> > 
> > I'm not convinced that this is a good solution.  If this oerlaps the
> > BSS region, it could well end up being overlapped by something else
> > more serious, like the data segment
> 
> My v1 had a cover letter, don't know if you've seen it. In that, I
> describe that the setup I see that faulty condition is caused by kexec,
> which currently assumes that the decompressed kernel size is max. 4
> times bigger than the zImage it executes. While that is naive, it seems
> to be an assumption that reflects reality in my tests.
>  
> Correct me if I'm wrong, but AFAIK the .bss section is the only thing
> that can grow to arbitrary size at runtime without accounting to the
> compressed image size; so in order to fix kernels executed by kexec that
> way, my fixup seems reasonable, doesn't it?

The BSS is actually fixed at compile time (which is how arm-linux-size
vmlinux can report this.)  Userspace is slightly different, because
after the BSS is a heap whose upper limit is controlled by brk().  The
kernel doesn't have that facility though.

> > - at which point doing the fixup
> > here means we've already lost.
> 
> The question is whether it does any harm to at least try to fix up
> things in that situation.

That's not really the question.  The real question is whether we will
end up having to revert this later because we've ended up with the
kernels .data section overlapping the DTB.

That's why I think this is the wrong place to be doing it - by the
time we get anywhere near to this place, we could have already lost
the game and overwritten the DTB.

> > We already give the decompressor the size of the kernel's BSS and take
> > action if we believe that the kernel's BSS will overlap the DTB/ATAGs
> > there.
> 
> Only for CONFIG_ARM_ATAG_DTB_COMPAT (which is unset in my config),
> right? I've read through the sources relevant for my system (AM33xx),
> and couldn't find any other place where my case is covered. The
> relocation I propose does fix it, though.

When things were at a fixed location, they were always placed at
around 256 bytes into the memory, below the kernel.  I guess now people
are placing them at some random location elsewhere.

This is becoming more a more of a pain.  We have all sorts of randomly
placed objects in memory now that its only a matter of time before we
hit something.  We keep on dreaming up these "well, let's move it"
solutions but does that really help?  Move it to where?  Another place
where we think there isn't going to be anything?  What if there is?

Back in the old days it was a lot more simple.  ATAGs started at +0x100
bytes.  The decompressor and kernel placed their initial page tables
at +0x4000, and the decompressed kernel started at +0x8000.  You could
then load the compressed kernel at a very high address, and place the
init ramdisk at an address sufficiently out of the way of all of that.
Yes, we made the compressed kernel care about whether it was going to
be overwritten by the decompressed image - that was more to cope with
the compressed image executing from the same location as the
decompressed image should be.

But now we have the situation where the DTB is located at some random
address, we have an initrd also at some unknown address, and we end
up hoping that we can move the compressed kernel to some other address
which doesn't trample over these, decompress it, and hope that the
decompressed image doesn't overwrite anything...

It's all extremely fragile and full of a hell of a lot of unknowns.

Should we continue sticking plasters over this by shifting objects
to other unknown locations in memory (which, incidentally, we don't
really know where or how much memory there is in early boot) ?

[PATCH v2] ARM: head-common.S: relocate atags area if necessary

[Nicolas Pitre]

On Fri, 18 Oct 2013, Russell King - ARM Linux wrote:

> When things were at a fixed location, they were always placed at
> around 256 bytes into the memory, below the kernel.  I guess now people
> are placing them at some random location elsewhere.
> 
> This is becoming more a more of a pain.  We have all sorts of randomly
> placed objects in memory now that its only a matter of time before we
> hit something.  We keep on dreaming up these "well, let's move it"
> solutions but does that really help?  Move it to where?  Another place
> where we think there isn't going to be anything?  What if there is?

This is why I suggested adding a recommendation for placing the 
initrd/DTB/ATAGs above the 128MB boundary from start of RAM in the ARM 
booting document.

The zImage code does have to take into account the location of the final 
kernel's .bss area in order to properly accommodate appended DTB to 
zImage, otherwise the simple relocation of the decompressor (with its 
appended DTB) out of the way of the final kernel image is not sufficient 
to guarantee that the appended DTB won't be overwritten.

But I think that this is the extent of what we should do.  We already 
don't relocate the initrd if it is in the way either.  Trying to make 
everything automagically relocate properly in all cases during very 
early boot is very difficult, so it is best to simply not attempt it.  
On the other hand, the loader agent, whether it is kexec or a 
bootloader, should be aware of all the available RAM and could therefore 
make things safe.

If you have the ability to load the DTB and/or ATAG independently from 
zImage or uncompressed Image (which is not the case for the appended 
DTB) then it is your responsibility to load it sufficiently far away 
from the kernel.  Same rule applies for the initrd.  Hence the "above 
128MB from start of RAM" recommendation.


Nicolas

[PATCH v2] ARM: head-common.S: relocate atags area if necessary

[Daniel Mack]

On 10/19/2013 05:15 PM, Nicolas Pitre wrote:
> On Fri, 18 Oct 2013, Russell King - ARM Linux wrote:
> 
>> When things were at a fixed location, they were always placed at
>> around 256 bytes into the memory, below the kernel.  I guess now people
>> are placing them at some random location elsewhere.
>>
>> This is becoming more a more of a pain.  We have all sorts of randomly
>> placed objects in memory now that its only a matter of time before we
>> hit something.  We keep on dreaming up these "well, let's move it"
>> solutions but does that really help?  Move it to where?  Another place
>> where we think there isn't going to be anything?  What if there is?
> 
> This is why I suggested adding a recommendation for placing the 
> initrd/DTB/ATAGs above the 128MB boundary from start of RAM in the ARM 
> booting document.
> 
> The zImage code does have to take into account the location of the final 
> kernel's .bss area in order to properly accommodate appended DTB to 
> zImage, otherwise the simple relocation of the decompressor (with its 
> appended DTB) out of the way of the final kernel image is not sufficient 
> to guarantee that the appended DTB won't be overwritten.
> 
> But I think that this is the extent of what we should do.  We already 
> don't relocate the initrd if it is in the way either.  Trying to make 
> everything automagically relocate properly in all cases during very 
> early boot is very difficult, so it is best to simply not attempt it.

Well, I totally see your point. However, we have machines out there that
won't update via kexec due to this, and fixing kexec is unfortunately
not an option, as updating anything would involve kexec'ing into an
update kernel again. So out only chance is to fix up the dtb location in
early boot code.

You might say my case is special, and you're most probably right. I see
that trying to fix things is opening a can of worms.

So - no worries, we'll just keep that patch around locally as it 'works
for me' (tm) :)


Daniel

[PATCH v2] ARM: head-common.S: relocate atags area if necessary

[Russell King - ARM Linux]

On Sat, Oct 19, 2013 at 06:10:16PM +0200, Daniel Mack wrote:
> Well, I totally see your point. However, we have machines out there that
> won't update via kexec due to this, and fixing kexec is unfortunately
> not an option, as updating anything would involve kexec'ing into an
> update kernel again. So out only chance is to fix up the dtb location in
> early boot code.
> 
> You might say my case is special, and you're most probably right. I see
> that trying to fix things is opening a can of worms.
> 
> So - no worries, we'll just keep that patch around locally as it 'works
> for me' (tm) :)

How about fixing kexec to work properly anyway, so that at a later date
you can drop that patch?

[PATCH v2] ARM: head-common.S: relocate atags area if necessary

[Daniel Mack]

On 10/19/2013 06:13 PM, Russell King - ARM Linux wrote:
> On Sat, Oct 19, 2013 at 06:10:16PM +0200, Daniel Mack wrote:
>> Well, I totally see your point. However, we have machines out there that
>> won't update via kexec due to this, and fixing kexec is unfortunately
>> not an option, as updating anything would involve kexec'ing into an
>> update kernel again. So out only chance is to fix up the dtb location in
>> early boot code.
>>
>> You might say my case is special, and you're most probably right. I see
>> that trying to fix things is opening a can of worms.
>>
>> So - no worries, we'll just keep that patch around locally as it 'works
>> for me' (tm) :)
> 
> How about fixing kexec to work properly anyway, so that at a later date
> you can drop that patch?

I'll have a look on how to properly fix this in kexec, yes.

However, new images have to stay compatible to old versions to update
from, so we'll not be able to drop the patch any time soon :(


Daniel