| Message ID | 1382112776-20300-1-git-send-email-zonque@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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) ?
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
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
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?
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
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,
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(-)