kexec failures with DEBUG_RODATA

kexec failures with DEBUG_RODATA

[Russell King - ARM Linux]

Guys,

Having added Keystone2 support to kexec, and asking TI to validate
linux-next with mainline kexec-tools, I received two reports from
them.

The first was a report of success, but was kexecing a 4.4 kernel
from linux-next.

The second was a failure report, kexecing current linux-next from
linux-next on this platform.  However, my local tests (using my
4.7-rc3 derived kernel) showed there to be no problem.

Building my 4.7-rc3 derived kernel with TI's configuration they
were using with linux-next similarly failed.  So, it came down to
a configuration difference.

After trialling several configurations, it turns out that the
failure is, in part, caused by CONFIG_DEBUG_RODATA being enabled
on TI's kernel but not mine.  Why should this make any difference?

Well, CONFIG_DEBUG_RODATA has the side effect that the kernel
contains a lot of additional padding - we pad out to section size
(1MB) the ELF sections with differing attributes.  This should not
normally be a problem, except kexec contains this assumption:

                /* Otherwise, assume the maximum kernel compression ratio
                 * is 4, and just to be safe, place ramdisk after that */
                initrd_base = base + _ALIGN(len * 4, getpagesize());

Now, first things first.  Don't get misled by the comment - it's
totally false.  That may be what's desired, but that is far from
what actually happens in reality.

"base" is _not_ the address of the start of the kernel image, but
is the base address of the start of the region that the kernel is
to be loaded into - remember that the kernel is normally loaded
32k higher than the start of memory.  This 32k offset is _not_
included in either "base" nor "len".  So, even if we did want to
assume that there was a maximum compression ratio of 4, the above
always calculates 32k short of that value.

The other invalid thing here is this whole "maximum kernel compression
ratio" assumption.  Consider this non-DEBUG_RODATA kernel image:

   text    data     bss     dec     hex filename
6583513 2273816  215344 9072673  8a7021 ../build/ks2/vmlinux

This results in an image and zimage of:
-rwxrwxr-x 1 rmk rmk 8871936 Jun 14 18:02 ../build/ks2/arch/arm/boot/Image
-rwxrwxr-x 1 rmk rmk 4381592 Jun 14 18:02 ../build/ks2/arch/arm/boot/zImage

which is a ratio of about a 49%.  On entry to the decompressor, the
compressed image will be relocated above the expected resulting
kernel size.  So, let's say that it's relocated to 9MB.  This means
the zImage will occupy around 9MB-14MB above the start of memory.
Going by the 4x ratio, we place the other images at 16.7MB.  This
leaves around 2.7MB free.  So that's probably fine... but think
about this.  We assumed a ratio of 4x, but really we're in a rather
tight squeeze - we actually have only about 50% of the compressed
image size spare.

Now let's look at the DEBUG_RODATA case:

   text    data     bss     dec     hex filename
6585305 2273952  215344 9074601  8a77a9 ../build/ks2/vmlinux

And the resulting sizes:
-rwxrwxr-x 1 rmk rmk 15024128 Jun 14 18:49 ../build/ks2/arch/arm/boot/Image
-rwxrwxr-x 1 rmk rmk  4399040 Jun 14 18:49 ../build/ks2/arch/arm/boot/zImage

That's a compression ratio of about 29%.  Still within the 4x limit,
but going through the same calculation above shows that we end up
totally overflowing the available space this time.

That's exactly the same kernel configuration except for
CONFIG_DEBUG_RODATA - enabling this has almost _doubled_ the
decompressed image size without affecting the compressed size.

We've known for some time that this ratio of 4x doesn't work - we
used to use the same assumption in the decompressor when self-
relocating, and we found that there are images which achieve a
better compression ratio and make this invalid.  Yet, the 4x thing
has persisted in kexec code... and buggily too.

Since the kernel now has CONFIG_DEBUG_RODATA by default, this means
that these kinds of ratio-based assumptions are even more invalid
than they have been.

Right now, a zImage doesn't advertise the size of its uncompressed
image, but I think with things like CONFIG_DEBUG_RODATA, we can no
longer make assumptions like we have done in the past, and we need
the zImage to provide this information so that the boot environment
can be setup sanely by boot loaders/kexec rather than relying on
broken heuristics like this.

Thoughts?

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

kexec failures with DEBUG_RODATA

[Russell King - ARM Linux]

Guys,

Having added Keystone2 support to kexec, and asking TI to validate
linux-next with mainline kexec-tools, I received two reports from
them.

The first was a report of success, but was kexecing a 4.4 kernel
from linux-next.

The second was a failure report, kexecing current linux-next from
linux-next on this platform.  However, my local tests (using my
4.7-rc3 derived kernel) showed there to be no problem.

Building my 4.7-rc3 derived kernel with TI's configuration they
were using with linux-next similarly failed.  So, it came down to
a configuration difference.

After trialling several configurations, it turns out that the
failure is, in part, caused by CONFIG_DEBUG_RODATA being enabled
on TI's kernel but not mine.  Why should this make any difference?

Well, CONFIG_DEBUG_RODATA has the side effect that the kernel
contains a lot of additional padding - we pad out to section size
(1MB) the ELF sections with differing attributes.  This should not
normally be a problem, except kexec contains this assumption:

                /* Otherwise, assume the maximum kernel compression ratio
                 * is 4, and just to be safe, place ramdisk after that */
                initrd_base = base + _ALIGN(len * 4, getpagesize());

Now, first things first.  Don't get misled by the comment - it's
totally false.  That may be what's desired, but that is far from
what actually happens in reality.

"base" is _not_ the address of the start of the kernel image, but
is the base address of the start of the region that the kernel is
to be loaded into - remember that the kernel is normally loaded
32k higher than the start of memory.  This 32k offset is _not_
included in either "base" nor "len".  So, even if we did want to
assume that there was a maximum compression ratio of 4, the above
always calculates 32k short of that value.

The other invalid thing here is this whole "maximum kernel compression
ratio" assumption.  Consider this non-DEBUG_RODATA kernel image:

   text    data     bss     dec     hex filename
6583513 2273816  215344 9072673  8a7021 ../build/ks2/vmlinux

This results in an image and zimage of:
-rwxrwxr-x 1 rmk rmk 8871936 Jun 14 18:02 ../build/ks2/arch/arm/boot/Image
-rwxrwxr-x 1 rmk rmk 4381592 Jun 14 18:02 ../build/ks2/arch/arm/boot/zImage

which is a ratio of about a 49%.  On entry to the decompressor, the
compressed image will be relocated above the expected resulting
kernel size.  So, let's say that it's relocated to 9MB.  This means
the zImage will occupy around 9MB-14MB above the start of memory.
Going by the 4x ratio, we place the other images at 16.7MB.  This
leaves around 2.7MB free.  So that's probably fine... but think
about this.  We assumed a ratio of 4x, but really we're in a rather
tight squeeze - we actually have only about 50% of the compressed
image size spare.

Now let's look at the DEBUG_RODATA case:

   text    data     bss     dec     hex filename
6585305 2273952  215344 9074601  8a77a9 ../build/ks2/vmlinux

And the resulting sizes:
-rwxrwxr-x 1 rmk rmk 15024128 Jun 14 18:49 ../build/ks2/arch/arm/boot/Image
-rwxrwxr-x 1 rmk rmk  4399040 Jun 14 18:49 ../build/ks2/arch/arm/boot/zImage

That's a compression ratio of about 29%.  Still within the 4x limit,
but going through the same calculation above shows that we end up
totally overflowing the available space this time.

That's exactly the same kernel configuration except for
CONFIG_DEBUG_RODATA - enabling this has almost _doubled_ the
decompressed image size without affecting the compressed size.

We've known for some time that this ratio of 4x doesn't work - we
used to use the same assumption in the decompressor when self-
relocating, and we found that there are images which achieve a
better compression ratio and make this invalid.  Yet, the 4x thing
has persisted in kexec code... and buggily too.

Since the kernel now has CONFIG_DEBUG_RODATA by default, this means
that these kinds of ratio-based assumptions are even more invalid
than they have been.

Right now, a zImage doesn't advertise the size of its uncompressed
image, but I think with things like CONFIG_DEBUG_RODATA, we can no
longer make assumptions like we have done in the past, and we need
the zImage to provide this information so that the boot environment
can be setup sanely by boot loaders/kexec rather than relying on
broken heuristics like this.

Thoughts?

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

kexec failures with DEBUG_RODATA

[Kees Cook]

On Tue, Jun 14, 2016 at 10:59 AM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> Guys,
>
> Having added Keystone2 support to kexec, and asking TI to validate
> linux-next with mainline kexec-tools, I received two reports from
> them.
>
> The first was a report of success, but was kexecing a 4.4 kernel
> from linux-next.
>
> The second was a failure report, kexecing current linux-next from
> linux-next on this platform.  However, my local tests (using my
> 4.7-rc3 derived kernel) showed there to be no problem.
>
> Building my 4.7-rc3 derived kernel with TI's configuration they
> were using with linux-next similarly failed.  So, it came down to
> a configuration difference.
>
> After trialling several configurations, it turns out that the
> failure is, in part, caused by CONFIG_DEBUG_RODATA being enabled
> on TI's kernel but not mine.  Why should this make any difference?
>
> Well, CONFIG_DEBUG_RODATA has the side effect that the kernel
> contains a lot of additional padding - we pad out to section size
> (1MB) the ELF sections with differing attributes.  This should not
> normally be a problem, except kexec contains this assumption:
>
>                 /* Otherwise, assume the maximum kernel compression ratio
>                  * is 4, and just to be safe, place ramdisk after that */
>                 initrd_base = base + _ALIGN(len * 4, getpagesize());
>
> Now, first things first.  Don't get misled by the comment - it's
> totally false.  That may be what's desired, but that is far from
> what actually happens in reality.
>
> "base" is _not_ the address of the start of the kernel image, but
> is the base address of the start of the region that the kernel is
> to be loaded into - remember that the kernel is normally loaded
> 32k higher than the start of memory.  This 32k offset is _not_
> included in either "base" nor "len".  So, even if we did want to
> assume that there was a maximum compression ratio of 4, the above
> always calculates 32k short of that value.
>
> The other invalid thing here is this whole "maximum kernel compression
> ratio" assumption.  Consider this non-DEBUG_RODATA kernel image:
>
>    text    data     bss     dec     hex filename
> 6583513 2273816  215344 9072673  8a7021 ../build/ks2/vmlinux
>
> This results in an image and zimage of:
> -rwxrwxr-x 1 rmk rmk 8871936 Jun 14 18:02 ../build/ks2/arch/arm/boot/Image
> -rwxrwxr-x 1 rmk rmk 4381592 Jun 14 18:02 ../build/ks2/arch/arm/boot/zImage
>
> which is a ratio of about a 49%.  On entry to the decompressor, the
> compressed image will be relocated above the expected resulting
> kernel size.  So, let's say that it's relocated to 9MB.  This means
> the zImage will occupy around 9MB-14MB above the start of memory.
> Going by the 4x ratio, we place the other images at 16.7MB.  This
> leaves around 2.7MB free.  So that's probably fine... but think
> about this.  We assumed a ratio of 4x, but really we're in a rather
> tight squeeze - we actually have only about 50% of the compressed
> image size spare.
>
> Now let's look at the DEBUG_RODATA case:
>
>    text    data     bss     dec     hex filename
> 6585305 2273952  215344 9074601  8a77a9 ../build/ks2/vmlinux
>
> And the resulting sizes:
> -rwxrwxr-x 1 rmk rmk 15024128 Jun 14 18:49 ../build/ks2/arch/arm/boot/Image
> -rwxrwxr-x 1 rmk rmk  4399040 Jun 14 18:49 ../build/ks2/arch/arm/boot/zImage
>
> That's a compression ratio of about 29%.  Still within the 4x limit,
> but going through the same calculation above shows that we end up
> totally overflowing the available space this time.
>
> That's exactly the same kernel configuration except for
> CONFIG_DEBUG_RODATA - enabling this has almost _doubled_ the
> decompressed image size without affecting the compressed size.
>
> We've known for some time that this ratio of 4x doesn't work - we
> used to use the same assumption in the decompressor when self-
> relocating, and we found that there are images which achieve a
> better compression ratio and make this invalid.  Yet, the 4x thing
> has persisted in kexec code... and buggily too.
>
> Since the kernel now has CONFIG_DEBUG_RODATA by default, this means
> that these kinds of ratio-based assumptions are even more invalid
> than they have been.
>
> Right now, a zImage doesn't advertise the size of its uncompressed
> image, but I think with things like CONFIG_DEBUG_RODATA, we can no
> longer make assumptions like we have done in the past, and we need
> the zImage to provide this information so that the boot environment
> can be setup sanely by boot loaders/kexec rather than relying on
> broken heuristics like this.
>
> Thoughts?

I'm much less familiar with the ARM decompression stub, but is there a
boot image header (like x86 has)? If not, perhaps we can invent one,
and it can carry all the details needed for a bootloader to do the
right things.

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

Re: kexec failures with DEBUG_RODATA

[Kees Cook]

On Tue, Jun 14, 2016 at 10:59 AM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> Guys,
>
> Having added Keystone2 support to kexec, and asking TI to validate
> linux-next with mainline kexec-tools, I received two reports from
> them.
>
> The first was a report of success, but was kexecing a 4.4 kernel
> from linux-next.
>
> The second was a failure report, kexecing current linux-next from
> linux-next on this platform.  However, my local tests (using my
> 4.7-rc3 derived kernel) showed there to be no problem.
>
> Building my 4.7-rc3 derived kernel with TI's configuration they
> were using with linux-next similarly failed.  So, it came down to
> a configuration difference.
>
> After trialling several configurations, it turns out that the
> failure is, in part, caused by CONFIG_DEBUG_RODATA being enabled
> on TI's kernel but not mine.  Why should this make any difference?
>
> Well, CONFIG_DEBUG_RODATA has the side effect that the kernel
> contains a lot of additional padding - we pad out to section size
> (1MB) the ELF sections with differing attributes.  This should not
> normally be a problem, except kexec contains this assumption:
>
>                 /* Otherwise, assume the maximum kernel compression ratio
>                  * is 4, and just to be safe, place ramdisk after that */
>                 initrd_base = base + _ALIGN(len * 4, getpagesize());
>
> Now, first things first.  Don't get misled by the comment - it's
> totally false.  That may be what's desired, but that is far from
> what actually happens in reality.
>
> "base" is _not_ the address of the start of the kernel image, but
> is the base address of the start of the region that the kernel is
> to be loaded into - remember that the kernel is normally loaded
> 32k higher than the start of memory.  This 32k offset is _not_
> included in either "base" nor "len".  So, even if we did want to
> assume that there was a maximum compression ratio of 4, the above
> always calculates 32k short of that value.
>
> The other invalid thing here is this whole "maximum kernel compression
> ratio" assumption.  Consider this non-DEBUG_RODATA kernel image:
>
>    text    data     bss     dec     hex filename
> 6583513 2273816  215344 9072673  8a7021 ../build/ks2/vmlinux
>
> This results in an image and zimage of:
> -rwxrwxr-x 1 rmk rmk 8871936 Jun 14 18:02 ../build/ks2/arch/arm/boot/Image
> -rwxrwxr-x 1 rmk rmk 4381592 Jun 14 18:02 ../build/ks2/arch/arm/boot/zImage
>
> which is a ratio of about a 49%.  On entry to the decompressor, the
> compressed image will be relocated above the expected resulting
> kernel size.  So, let's say that it's relocated to 9MB.  This means
> the zImage will occupy around 9MB-14MB above the start of memory.
> Going by the 4x ratio, we place the other images at 16.7MB.  This
> leaves around 2.7MB free.  So that's probably fine... but think
> about this.  We assumed a ratio of 4x, but really we're in a rather
> tight squeeze - we actually have only about 50% of the compressed
> image size spare.
>
> Now let's look at the DEBUG_RODATA case:
>
>    text    data     bss     dec     hex filename
> 6585305 2273952  215344 9074601  8a77a9 ../build/ks2/vmlinux
>
> And the resulting sizes:
> -rwxrwxr-x 1 rmk rmk 15024128 Jun 14 18:49 ../build/ks2/arch/arm/boot/Image
> -rwxrwxr-x 1 rmk rmk  4399040 Jun 14 18:49 ../build/ks2/arch/arm/boot/zImage
>
> That's a compression ratio of about 29%.  Still within the 4x limit,
> but going through the same calculation above shows that we end up
> totally overflowing the available space this time.
>
> That's exactly the same kernel configuration except for
> CONFIG_DEBUG_RODATA - enabling this has almost _doubled_ the
> decompressed image size without affecting the compressed size.
>
> We've known for some time that this ratio of 4x doesn't work - we
> used to use the same assumption in the decompressor when self-
> relocating, and we found that there are images which achieve a
> better compression ratio and make this invalid.  Yet, the 4x thing
> has persisted in kexec code... and buggily too.
>
> Since the kernel now has CONFIG_DEBUG_RODATA by default, this means
> that these kinds of ratio-based assumptions are even more invalid
> than they have been.
>
> Right now, a zImage doesn't advertise the size of its uncompressed
> image, but I think with things like CONFIG_DEBUG_RODATA, we can no
> longer make assumptions like we have done in the past, and we need
> the zImage to provide this information so that the boot environment
> can be setup sanely by boot loaders/kexec rather than relying on
> broken heuristics like this.
>
> Thoughts?

I'm much less familiar with the ARM decompression stub, but is there a
boot image header (like x86 has)? If not, perhaps we can invent one,
and it can carry all the details needed for a bootloader to do the
right things.

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

kexec failures with DEBUG_RODATA

[Baoquan He]

On 06/14/16 at 11:05am, Kees Cook wrote:
> On Tue, Jun 14, 2016 at 10:59 AM, Russell King - ARM Linux
> <linux@armlinux.org.uk> wrote:
> > Since the kernel now has CONFIG_DEBUG_RODATA by default, this means
> > that these kinds of ratio-based assumptions are even more invalid
> > than they have been.
> >
> > Right now, a zImage doesn't advertise the size of its uncompressed
> > image, but I think with things like CONFIG_DEBUG_RODATA, we can no
> > longer make assumptions like we have done in the past, and we need
> > the zImage to provide this information so that the boot environment
> > can be setup sanely by boot loaders/kexec rather than relying on
> > broken heuristics like this.
> >
> > Thoughts?
> 
> I'm much less familiar with the ARM decompression stub, but is there a
> boot image header (like x86 has)? If not, perhaps we can invent one,
> and it can carry all the details needed for a bootloader to do the
> right things.

Yes, x86 stores addr and size of initrd into boot header. When
decompressing kernel it will choose a safe starting position before the
loaded place according to the max evaluation of decompressing algorithm.

ARM only use a rough 4 times evaluation, sounds too hasty. Simplest way
is to increase times to 8 for now. The final way should be as Kees
suggested.

Thanks
Baoquan

Re: kexec failures with DEBUG_RODATA

[Baoquan He]

On 06/14/16 at 11:05am, Kees Cook wrote:
> On Tue, Jun 14, 2016 at 10:59 AM, Russell King - ARM Linux
> <linux@armlinux.org.uk> wrote:
> > Since the kernel now has CONFIG_DEBUG_RODATA by default, this means
> > that these kinds of ratio-based assumptions are even more invalid
> > than they have been.
> >
> > Right now, a zImage doesn't advertise the size of its uncompressed
> > image, but I think with things like CONFIG_DEBUG_RODATA, we can no
> > longer make assumptions like we have done in the past, and we need
> > the zImage to provide this information so that the boot environment
> > can be setup sanely by boot loaders/kexec rather than relying on
> > broken heuristics like this.
> >
> > Thoughts?
> 
> I'm much less familiar with the ARM decompression stub, but is there a
> boot image header (like x86 has)? If not, perhaps we can invent one,
> and it can carry all the details needed for a bootloader to do the
> right things.

Yes, x86 stores addr and size of initrd into boot header. When
decompressing kernel it will choose a safe starting position before the
loaded place according to the max evaluation of decompressing algorithm.

ARM only use a rough 4 times evaluation, sounds too hasty. Simplest way
is to increase times to 8 for now. The final way should be as Kees
suggested.

Thanks
Baoquan


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

kexec failures with DEBUG_RODATA

[Pratyush Anand]

Hi Russell,

On 14/06/2016:06:59:20 PM, Russell King - ARM Linux wrote:
> Guys,
> 
> Having added Keystone2 support to kexec, and asking TI to validate
> linux-next with mainline kexec-tools, I received two reports from
> them.
> 
> The first was a report of success, but was kexecing a 4.4 kernel
> from linux-next.
> 
> The second was a failure report, kexecing current linux-next from
> linux-next on this platform.  However, my local tests (using my
> 4.7-rc3 derived kernel) showed there to be no problem.
> 
> Building my 4.7-rc3 derived kernel with TI's configuration they
> were using with linux-next similarly failed.  So, it came down to
> a configuration difference.
> 
> After trialling several configurations, it turns out that the
> failure is, in part, caused by CONFIG_DEBUG_RODATA being enabled
> on TI's kernel but not mine.  Why should this make any difference?

> 
> Well, CONFIG_DEBUG_RODATA has the side effect that the kernel
> contains a lot of additional padding - we pad out to section size
> (1MB) the ELF sections with differing attributes.  This should not
> normally be a problem, except kexec contains this assumption:
> 
>                 /* Otherwise, assume the maximum kernel compression ratio
>                  * is 4, and just to be safe, place ramdisk after that */
>                 initrd_base = base + _ALIGN(len * 4, getpagesize());
> 
> Now, first things first.  Don't get misled by the comment - it's
> totally false.  That may be what's desired, but that is far from
> what actually happens in reality.
> 
> "base" is _not_ the address of the start of the kernel image, but
> is the base address of the start of the region that the kernel is
> to be loaded into - remember that the kernel is normally loaded
> 32k higher than the start of memory.  This 32k offset is _not_
> included in either "base" nor "len".  So, even if we did want to
> assume that there was a maximum compression ratio of 4, the above
> always calculates 32k short of that value.
> 
> The other invalid thing here is this whole "maximum kernel compression
> ratio" assumption.  Consider this non-DEBUG_RODATA kernel image:
> 
>    text    data     bss     dec     hex filename
> 6583513 2273816  215344 9072673  8a7021 ../build/ks2/vmlinux
> 
> This results in an image and zimage of:
> -rwxrwxr-x 1 rmk rmk 8871936 Jun 14 18:02 ../build/ks2/arch/arm/boot/Image
> -rwxrwxr-x 1 rmk rmk 4381592 Jun 14 18:02 ../build/ks2/arch/arm/boot/zImage
> 
> which is a ratio of about a 49%.  On entry to the decompressor, the
> compressed image will be relocated above the expected resulting
> kernel size.  So, let's say that it's relocated to 9MB.  This means
> the zImage will occupy around 9MB-14MB above the start of memory.
> Going by the 4x ratio, we place the other images at 16.7MB.  This
> leaves around 2.7MB free.  So that's probably fine... but think
> about this.  We assumed a ratio of 4x, but really we're in a rather
> tight squeeze - we actually have only about 50% of the compressed
> image size spare.
> 
> Now let's look at the DEBUG_RODATA case:
> 
>    text    data     bss     dec     hex filename
> 6585305 2273952  215344 9074601  8a77a9 ../build/ks2/vmlinux
> 
> And the resulting sizes:
> -rwxrwxr-x 1 rmk rmk 15024128 Jun 14 18:49 ../build/ks2/arch/arm/boot/Image
> -rwxrwxr-x 1 rmk rmk  4399040 Jun 14 18:49 ../build/ks2/arch/arm/boot/zImage
> 
> That's a compression ratio of about 29%.  Still within the 4x limit,
> but going through the same calculation above shows that we end up
> totally overflowing the available space this time.
> 
> That's exactly the same kernel configuration except for
> CONFIG_DEBUG_RODATA - enabling this has almost _doubled_ the
> decompressed image size without affecting the compressed size.
> 
> We've known for some time that this ratio of 4x doesn't work - we
> used to use the same assumption in the decompressor when self-
> relocating, and we found that there are images which achieve a
> better compression ratio and make this invalid.  Yet, the 4x thing
> has persisted in kexec code... and buggily too.
> 
> Since the kernel now has CONFIG_DEBUG_RODATA by default, this means
> that these kinds of ratio-based assumptions are even more invalid
> than they have been.
> 
> Right now, a zImage doesn't advertise the size of its uncompressed
> image, but I think with things like CONFIG_DEBUG_RODATA, we can no
> longer make assumptions like we have done in the past, and we need
> the zImage to provide this information so that the boot environment
> can be setup sanely by boot loaders/kexec rather than relying on
> broken heuristics like this.
> 
> Thoughts?

Sure, having a header information would be handy to do it. Other alternative
could be that we define "HAVE_LIBZ" and then we can have something like
kexec-Image-arm.c which handles plane Image.  We can also have something like
get_zlib_decompressed_length() which can give us exact length we need for kernel
and then we can place initrd accordingly in zImage_arm_load().

Other than that:

I see at least another issue clearly in ARM kernel code with CONFIG_DEBUG_RODATA
enabled.  When CONFIG_DEBUG_RODATA is enabled, we can not write text area.
kexec_start_address has been defined in relocate_kernel.S as a text area.
machine_kexec() writes at kexec_start_address with image->start. Similarly there
would be issues for overwriting of kexec_indirection_page, kexec_mach_type and
kexec_boot_atags. If arm mmu mapping configures text pages as RO, then we should
see an abort as soon as we do these writes.

I think there could be two alternatives to fix it (i) We pass all of above
values as function argument to soft_restart() till relocate_new_kernel().  (ii)
Remove kexec_start_address and others from relocate_new_kernel(). Now in
machine_kexec(), copy kexec_start_address and friends directly to
(reboot_code_buffer+relocate_new_kernel_size)


~Pratyush

Re: kexec failures with DEBUG_RODATA

[Pratyush Anand]

Hi Russell,

On 14/06/2016:06:59:20 PM, Russell King - ARM Linux wrote:
> Guys,
> 
> Having added Keystone2 support to kexec, and asking TI to validate
> linux-next with mainline kexec-tools, I received two reports from
> them.
> 
> The first was a report of success, but was kexecing a 4.4 kernel
> from linux-next.
> 
> The second was a failure report, kexecing current linux-next from
> linux-next on this platform.  However, my local tests (using my
> 4.7-rc3 derived kernel) showed there to be no problem.
> 
> Building my 4.7-rc3 derived kernel with TI's configuration they
> were using with linux-next similarly failed.  So, it came down to
> a configuration difference.
> 
> After trialling several configurations, it turns out that the
> failure is, in part, caused by CONFIG_DEBUG_RODATA being enabled
> on TI's kernel but not mine.  Why should this make any difference?

> 
> Well, CONFIG_DEBUG_RODATA has the side effect that the kernel
> contains a lot of additional padding - we pad out to section size
> (1MB) the ELF sections with differing attributes.  This should not
> normally be a problem, except kexec contains this assumption:
> 
>                 /* Otherwise, assume the maximum kernel compression ratio
>                  * is 4, and just to be safe, place ramdisk after that */
>                 initrd_base = base + _ALIGN(len * 4, getpagesize());
> 
> Now, first things first.  Don't get misled by the comment - it's
> totally false.  That may be what's desired, but that is far from
> what actually happens in reality.
> 
> "base" is _not_ the address of the start of the kernel image, but
> is the base address of the start of the region that the kernel is
> to be loaded into - remember that the kernel is normally loaded
> 32k higher than the start of memory.  This 32k offset is _not_
> included in either "base" nor "len".  So, even if we did want to
> assume that there was a maximum compression ratio of 4, the above
> always calculates 32k short of that value.
> 
> The other invalid thing here is this whole "maximum kernel compression
> ratio" assumption.  Consider this non-DEBUG_RODATA kernel image:
> 
>    text    data     bss     dec     hex filename
> 6583513 2273816  215344 9072673  8a7021 ../build/ks2/vmlinux
> 
> This results in an image and zimage of:
> -rwxrwxr-x 1 rmk rmk 8871936 Jun 14 18:02 ../build/ks2/arch/arm/boot/Image
> -rwxrwxr-x 1 rmk rmk 4381592 Jun 14 18:02 ../build/ks2/arch/arm/boot/zImage
> 
> which is a ratio of about a 49%.  On entry to the decompressor, the
> compressed image will be relocated above the expected resulting
> kernel size.  So, let's say that it's relocated to 9MB.  This means
> the zImage will occupy around 9MB-14MB above the start of memory.
> Going by the 4x ratio, we place the other images at 16.7MB.  This
> leaves around 2.7MB free.  So that's probably fine... but think
> about this.  We assumed a ratio of 4x, but really we're in a rather
> tight squeeze - we actually have only about 50% of the compressed
> image size spare.
> 
> Now let's look at the DEBUG_RODATA case:
> 
>    text    data     bss     dec     hex filename
> 6585305 2273952  215344 9074601  8a77a9 ../build/ks2/vmlinux
> 
> And the resulting sizes:
> -rwxrwxr-x 1 rmk rmk 15024128 Jun 14 18:49 ../build/ks2/arch/arm/boot/Image
> -rwxrwxr-x 1 rmk rmk  4399040 Jun 14 18:49 ../build/ks2/arch/arm/boot/zImage
> 
> That's a compression ratio of about 29%.  Still within the 4x limit,
> but going through the same calculation above shows that we end up
> totally overflowing the available space this time.
> 
> That's exactly the same kernel configuration except for
> CONFIG_DEBUG_RODATA - enabling this has almost _doubled_ the
> decompressed image size without affecting the compressed size.
> 
> We've known for some time that this ratio of 4x doesn't work - we
> used to use the same assumption in the decompressor when self-
> relocating, and we found that there are images which achieve a
> better compression ratio and make this invalid.  Yet, the 4x thing
> has persisted in kexec code... and buggily too.
> 
> Since the kernel now has CONFIG_DEBUG_RODATA by default, this means
> that these kinds of ratio-based assumptions are even more invalid
> than they have been.
> 
> Right now, a zImage doesn't advertise the size of its uncompressed
> image, but I think with things like CONFIG_DEBUG_RODATA, we can no
> longer make assumptions like we have done in the past, and we need
> the zImage to provide this information so that the boot environment
> can be setup sanely by boot loaders/kexec rather than relying on
> broken heuristics like this.
> 
> Thoughts?

Sure, having a header information would be handy to do it. Other alternative
could be that we define "HAVE_LIBZ" and then we can have something like
kexec-Image-arm.c which handles plane Image.  We can also have something like
get_zlib_decompressed_length() which can give us exact length we need for kernel
and then we can place initrd accordingly in zImage_arm_load().

Other than that:

I see at least another issue clearly in ARM kernel code with CONFIG_DEBUG_RODATA
enabled.  When CONFIG_DEBUG_RODATA is enabled, we can not write text area.
kexec_start_address has been defined in relocate_kernel.S as a text area.
machine_kexec() writes at kexec_start_address with image->start. Similarly there
would be issues for overwriting of kexec_indirection_page, kexec_mach_type and
kexec_boot_atags. If arm mmu mapping configures text pages as RO, then we should
see an abort as soon as we do these writes.

I think there could be two alternatives to fix it (i) We pass all of above
values as function argument to soft_restart() till relocate_new_kernel().  (ii)
Remove kexec_start_address and others from relocate_new_kernel(). Now in
machine_kexec(), copy kexec_start_address and friends directly to
(reboot_code_buffer+relocate_new_kernel_size)


~Pratyush

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

kexec failures with DEBUG_RODATA

[Russell King - ARM Linux]

On Wed, Jun 15, 2016 at 01:25:08PM +0530, Pratyush Anand wrote:
> Sure, having a header information would be handy to do it. Other alternative
> could be that we define "HAVE_LIBZ" and then we can have something like
> kexec-Image-arm.c which handles plane Image.  We can also have something like
> get_zlib_decompressed_length() which can give us exact length we need for kernel
> and then we can place initrd accordingly in zImage_arm_load().

I really don't want to do that.  There are things that the decompressor
does which make it easier to deal with the zImage than the Image.

> I see at least another issue clearly in ARM kernel code with CONFIG_DEBUG_RODATA
> enabled.  When CONFIG_DEBUG_RODATA is enabled, we can not write text area.
> kexec_start_address has been defined in relocate_kernel.S as a text area.
> machine_kexec() writes at kexec_start_address with image->start. Similarly there
> would be issues for overwriting of kexec_indirection_page, kexec_mach_type and
> kexec_boot_atags. If arm mmu mapping configures text pages as RO, then we should
> see an abort as soon as we do these writes.

That's not a problem, because set_kernel_text_rw() is called immediately
prior to writing, which has the effect of allowing these writes.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: kexec failures with DEBUG_RODATA

[Russell King - ARM Linux]

On Wed, Jun 15, 2016 at 01:25:08PM +0530, Pratyush Anand wrote:
> Sure, having a header information would be handy to do it. Other alternative
> could be that we define "HAVE_LIBZ" and then we can have something like
> kexec-Image-arm.c which handles plane Image.  We can also have something like
> get_zlib_decompressed_length() which can give us exact length we need for kernel
> and then we can place initrd accordingly in zImage_arm_load().

I really don't want to do that.  There are things that the decompressor
does which make it easier to deal with the zImage than the Image.

> I see at least another issue clearly in ARM kernel code with CONFIG_DEBUG_RODATA
> enabled.  When CONFIG_DEBUG_RODATA is enabled, we can not write text area.
> kexec_start_address has been defined in relocate_kernel.S as a text area.
> machine_kexec() writes at kexec_start_address with image->start. Similarly there
> would be issues for overwriting of kexec_indirection_page, kexec_mach_type and
> kexec_boot_atags. If arm mmu mapping configures text pages as RO, then we should
> see an abort as soon as we do these writes.

That's not a problem, because set_kernel_text_rw() is called immediately
prior to writing, which has the effect of allowing these writes.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

kexec failures with DEBUG_RODATA

[Russell King - ARM Linux]

On Tue, Jun 14, 2016 at 11:05:23AM -0700, Kees Cook wrote:
> I'm much less familiar with the ARM decompression stub, but is there a
> boot image header (like x86 has)? If not, perhaps we can invent one,
> and it can carry all the details needed for a bootloader to do the
> right things.

With a bit of tinkering around, I now have this:

00000000 <.data>:
       0:       e1a00000        nop                     ; (mov r0, r0)
       4:       e1a00000        nop                     ; (mov r0, r0)
       8:       e1a00000        nop                     ; (mov r0, r0)
       c:       e1a00000        nop                     ; (mov r0, r0)
      10:       e1a00000        nop                     ; (mov r0, r0)
      14:       e1a00000        nop                     ; (mov r0, r0)
      18:       e1a00000        nop                     ; (mov r0, r0)
      1c:       e1a00000        nop                     ; (mov r0, r0)
      20:       ea00000f        b       0x64

Then follows the existing "header" which we've had there for years:

      24:       016f2818	; LE magic number
      28:       00000000	; LE zImage start address (always zero now)
      2c:       00431fe0	; LE zImage _edata
      30:       04030201	; endian flag

And now comes the new header:

      34:       016f2818	; LE magic number
      38:       00000001	; LE version number (v1)
      3c:       01287000	; LE total space required for decompressor
      40:       00e54000	; LE uncompressed image size

Up to 64 bytes available here for future expansion, currently filled
with zeros.
        ...

Remainder of the zImage code:
      64:       e10f9000        mrs     r9, CPSR

I'm rather on the fence whether we need to give the uncompressed image
size - the important thing is the size of memory that's required for
the decompressor to run, which is sizeof(uncompressed kernel) rounded
up to 256 bytes, and the relocated decompressor image size.

The "total space required for decompressor" is slightly cheating at the
figure - I'm including the uncompressed image rounded up and the entire
compressed image in that size, so it's a safe over-estimate.

I'm not sure there's a need to provide the uncompressed image size, the
boot environment shouldn't have a reason to know that, so I'm tempted to
omit it.

We could dispense with the endian conversions, and push the responsibility
for interpreting that onto the reader of this data: we have the endian
flag in the existing header block, so the boot environment can work out
the endianness of the image and apply fixups as appropriate.

Why generate this in the linker script?  We need the size of the zImage
here, which is only known to the linker.

diff --git a/arch/arm/boot/compressed/Makefile b/arch/arm/boot/compressed/Makefile
index d50430c40045..1d5467e05250 100644
--- a/arch/arm/boot/compressed/Makefile
+++ b/arch/arm/boot/compressed/Makefile
@@ -119,6 +119,10 @@ asflags-y := -DZIMAGE
 KBSS_SZ = $(shell $(CROSS_COMPILE)size $(obj)/../../../../vmlinux | \
 		awk 'END{print $$3}')
 LDFLAGS_vmlinux = --defsym _kernel_bss_size=$(KBSS_SZ)
+
+KERNEL_IMAGE_SIZE = $(shell stat -c '%s' $(obj)/../Image)
+LDFLAGS_vmlinux += --defsym _kernel_image_size=$(KERNEL_IMAGE_SIZE)
+
 # Supply ZRELADDR to the decompressor via a linker symbol.
 ifneq ($(CONFIG_AUTO_ZRELADDR),y)
 LDFLAGS_vmlinux += --defsym zreladdr=$(ZRELADDR)
diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S
index e2e0dcb42ca2..395c60dcc4f7 100644
--- a/arch/arm/boot/compressed/head.S
+++ b/arch/arm/boot/compressed/head.S
@@ -131,11 +131,7 @@ start:
  THUMB(		badr	r12, 1f		)
  THUMB(		bx	r12		)
 
-		.word	_magic_sig	@ Magic numbers to help the loader
-		.word	_magic_start	@ absolute load/run zImage address
-		.word	_magic_end	@ zImage end address
-		.word	0x04030201	@ endianness flag
-
+		.section ".start2", #alloc, #execinstr
  THUMB(		.thumb			)
 1:		__EFI_HEADER
 
diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
index 81c493156ce8..77267724ec8a 100644
--- a/arch/arm/boot/compressed/vmlinux.lds.S
+++ b/arch/arm/boot/compressed/vmlinux.lds.S
@@ -37,6 +37,19 @@ SECTIONS
   .text : {
     _start = .;
     *(.start)
+    _header = .;
+    LONG(ZIMAGE_MAGIC(0x016f2818));	/* Magic numbers to help the loader */
+    LONG(ZIMAGE_MAGIC(_start));		/* absolute load/run zImage address */
+    LONG(ZIMAGE_MAGIC(_edata));		/* zImage end address */
+    LONG(0x04030201);			/* Endianness flag */
+    LONG(ZIMAGE_MAGIC(0x016f2818));	/* Further header indicator */
+    LONG(ZIMAGE_MAGIC(1));		/* Version 1 */
+    LONG(ZIMAGE_MAGIC(((_kernel_image_size + 255) & ~ 255) + \
+			_edata - _text + _end_stack - __bss_start));
+    LONG(ZIMAGE_MAGIC(_kernel_image_size));
+    /* Reserve 64 bytes for the header block */
+    . = _header + 64;
+    *(.start2)
     *(.text)
     *(.text.*)
     *(.fixup)
@@ -72,10 +85,6 @@ SECTIONS
   .pad			: { BYTE(0); . = ALIGN(8); }
   _edata = .;
 
-  _magic_sig = ZIMAGE_MAGIC(0x016f2818);
-  _magic_start = ZIMAGE_MAGIC(_start);
-  _magic_end = ZIMAGE_MAGIC(_edata);
-
   . = BSS_START;
   __bss_start = .;
   .bss			: { *(.bss) }
@@ -83,6 +92,7 @@ SECTIONS
 
   . = ALIGN(8);		/* the stack must be 64-bit aligned */
   .stack		: { *(.stack) }
+  _end_stack = .;
 
   .stab 0		: { *(.stab) }
   .stabstr 0		: { *(.stabstr) }


-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: kexec failures with DEBUG_RODATA

[Russell King - ARM Linux]

On Tue, Jun 14, 2016 at 11:05:23AM -0700, Kees Cook wrote:
> I'm much less familiar with the ARM decompression stub, but is there a
> boot image header (like x86 has)? If not, perhaps we can invent one,
> and it can carry all the details needed for a bootloader to do the
> right things.

With a bit of tinkering around, I now have this:

00000000 <.data>:
       0:       e1a00000        nop                     ; (mov r0, r0)
       4:       e1a00000        nop                     ; (mov r0, r0)
       8:       e1a00000        nop                     ; (mov r0, r0)
       c:       e1a00000        nop                     ; (mov r0, r0)
      10:       e1a00000        nop                     ; (mov r0, r0)
      14:       e1a00000        nop                     ; (mov r0, r0)
      18:       e1a00000        nop                     ; (mov r0, r0)
      1c:       e1a00000        nop                     ; (mov r0, r0)
      20:       ea00000f        b       0x64

Then follows the existing "header" which we've had there for years:

      24:       016f2818	; LE magic number
      28:       00000000	; LE zImage start address (always zero now)
      2c:       00431fe0	; LE zImage _edata
      30:       04030201	; endian flag

And now comes the new header:

      34:       016f2818	; LE magic number
      38:       00000001	; LE version number (v1)
      3c:       01287000	; LE total space required for decompressor
      40:       00e54000	; LE uncompressed image size

Up to 64 bytes available here for future expansion, currently filled
with zeros.
        ...

Remainder of the zImage code:
      64:       e10f9000        mrs     r9, CPSR

I'm rather on the fence whether we need to give the uncompressed image
size - the important thing is the size of memory that's required for
the decompressor to run, which is sizeof(uncompressed kernel) rounded
up to 256 bytes, and the relocated decompressor image size.

The "total space required for decompressor" is slightly cheating at the
figure - I'm including the uncompressed image rounded up and the entire
compressed image in that size, so it's a safe over-estimate.

I'm not sure there's a need to provide the uncompressed image size, the
boot environment shouldn't have a reason to know that, so I'm tempted to
omit it.

We could dispense with the endian conversions, and push the responsibility
for interpreting that onto the reader of this data: we have the endian
flag in the existing header block, so the boot environment can work out
the endianness of the image and apply fixups as appropriate.

Why generate this in the linker script?  We need the size of the zImage
here, which is only known to the linker.

diff --git a/arch/arm/boot/compressed/Makefile b/arch/arm/boot/compressed/Makefile
index d50430c40045..1d5467e05250 100644
--- a/arch/arm/boot/compressed/Makefile
+++ b/arch/arm/boot/compressed/Makefile
@@ -119,6 +119,10 @@ asflags-y := -DZIMAGE
 KBSS_SZ = $(shell $(CROSS_COMPILE)size $(obj)/../../../../vmlinux | \
 		awk 'END{print $$3}')
 LDFLAGS_vmlinux = --defsym _kernel_bss_size=$(KBSS_SZ)
+
+KERNEL_IMAGE_SIZE = $(shell stat -c '%s' $(obj)/../Image)
+LDFLAGS_vmlinux += --defsym _kernel_image_size=$(KERNEL_IMAGE_SIZE)
+
 # Supply ZRELADDR to the decompressor via a linker symbol.
 ifneq ($(CONFIG_AUTO_ZRELADDR),y)
 LDFLAGS_vmlinux += --defsym zreladdr=$(ZRELADDR)
diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S
index e2e0dcb42ca2..395c60dcc4f7 100644
--- a/arch/arm/boot/compressed/head.S
+++ b/arch/arm/boot/compressed/head.S
@@ -131,11 +131,7 @@ start:
  THUMB(		badr	r12, 1f		)
  THUMB(		bx	r12		)
 
-		.word	_magic_sig	@ Magic numbers to help the loader
-		.word	_magic_start	@ absolute load/run zImage address
-		.word	_magic_end	@ zImage end address
-		.word	0x04030201	@ endianness flag
-
+		.section ".start2", #alloc, #execinstr
  THUMB(		.thumb			)
 1:		__EFI_HEADER
 
diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
index 81c493156ce8..77267724ec8a 100644
--- a/arch/arm/boot/compressed/vmlinux.lds.S
+++ b/arch/arm/boot/compressed/vmlinux.lds.S
@@ -37,6 +37,19 @@ SECTIONS
   .text : {
     _start = .;
     *(.start)
+    _header = .;
+    LONG(ZIMAGE_MAGIC(0x016f2818));	/* Magic numbers to help the loader */
+    LONG(ZIMAGE_MAGIC(_start));		/* absolute load/run zImage address */
+    LONG(ZIMAGE_MAGIC(_edata));		/* zImage end address */
+    LONG(0x04030201);			/* Endianness flag */
+    LONG(ZIMAGE_MAGIC(0x016f2818));	/* Further header indicator */
+    LONG(ZIMAGE_MAGIC(1));		/* Version 1 */
+    LONG(ZIMAGE_MAGIC(((_kernel_image_size + 255) & ~ 255) + \
+			_edata - _text + _end_stack - __bss_start));
+    LONG(ZIMAGE_MAGIC(_kernel_image_size));
+    /* Reserve 64 bytes for the header block */
+    . = _header + 64;
+    *(.start2)
     *(.text)
     *(.text.*)
     *(.fixup)
@@ -72,10 +85,6 @@ SECTIONS
   .pad			: { BYTE(0); . = ALIGN(8); }
   _edata = .;
 
-  _magic_sig = ZIMAGE_MAGIC(0x016f2818);
-  _magic_start = ZIMAGE_MAGIC(_start);
-  _magic_end = ZIMAGE_MAGIC(_edata);
-
   . = BSS_START;
   __bss_start = .;
   .bss			: { *(.bss) }
@@ -83,6 +92,7 @@ SECTIONS
 
   . = ALIGN(8);		/* the stack must be 64-bit aligned */
   .stack		: { *(.stack) }
+  _end_stack = .;
 
   .stab 0		: { *(.stab) }
   .stabstr 0		: { *(.stabstr) }


-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

kexec failures with DEBUG_RODATA

[Kees Cook]

On Wed, Jun 15, 2016 at 2:13 PM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Tue, Jun 14, 2016 at 11:05:23AM -0700, Kees Cook wrote:
>> I'm much less familiar with the ARM decompression stub, but is there a
>> boot image header (like x86 has)? If not, perhaps we can invent one,
>> and it can carry all the details needed for a bootloader to do the
>> right things.
>
> With a bit of tinkering around, I now have this:
>
> 00000000 <.data>:
>        0:       e1a00000        nop                     ; (mov r0, r0)
>        4:       e1a00000        nop                     ; (mov r0, r0)
>        8:       e1a00000        nop                     ; (mov r0, r0)
>        c:       e1a00000        nop                     ; (mov r0, r0)
>       10:       e1a00000        nop                     ; (mov r0, r0)
>       14:       e1a00000        nop                     ; (mov r0, r0)
>       18:       e1a00000        nop                     ; (mov r0, r0)
>       1c:       e1a00000        nop                     ; (mov r0, r0)
>       20:       ea00000f        b       0x64
>
> Then follows the existing "header" which we've had there for years:
>
>       24:       016f2818        ; LE magic number
>       28:       00000000        ; LE zImage start address (always zero now)
>       2c:       00431fe0        ; LE zImage _edata
>       30:       04030201        ; endian flag
>
> And now comes the new header:
>
>       34:       016f2818        ; LE magic number

Should this be a different magic from the existing header's magic?

>       38:       00000001        ; LE version number (v1)

Should a "size" follow the version number instead of the explicit 64
bytes of zeros?

>       3c:       01287000        ; LE total space required for decompressor
>       40:       00e54000        ; LE uncompressed image size
>
> Up to 64 bytes available here for future expansion, currently filled
> with zeros.
>         ...
>
> Remainder of the zImage code:
>       64:       e10f9000        mrs     r9, CPSR
>
> I'm rather on the fence whether we need to give the uncompressed image
> size - the important thing is the size of memory that's required for
> the decompressor to run, which is sizeof(uncompressed kernel) rounded
> up to 256 bytes, and the relocated decompressor image size.

I think it's important information since it allows a boot loader to
figure out if there's room in a given range for the result. While
there's no relocation support yet, if we gain it on ARM and we want to
support KASLR, it will be very handy to have the uncompressed size
available.

>
> The "total space required for decompressor" is slightly cheating at the
> figure - I'm including the uncompressed image rounded up and the entire
> compressed image in that size, so it's a safe over-estimate.
>
> I'm not sure there's a need to provide the uncompressed image size, the
> boot environment shouldn't have a reason to know that, so I'm tempted to
> omit it.
>
> We could dispense with the endian conversions, and push the responsibility
> for interpreting that onto the reader of this data: we have the endian
> flag in the existing header block, so the boot environment can work out
> the endianness of the image and apply fixups as appropriate.
>
> Why generate this in the linker script?  We need the size of the zImage
> here, which is only known to the linker.
>
> diff --git a/arch/arm/boot/compressed/Makefile b/arch/arm/boot/compressed/Makefile
> index d50430c40045..1d5467e05250 100644
> --- a/arch/arm/boot/compressed/Makefile
> +++ b/arch/arm/boot/compressed/Makefile
> @@ -119,6 +119,10 @@ asflags-y := -DZIMAGE
>  KBSS_SZ = $(shell $(CROSS_COMPILE)size $(obj)/../../../../vmlinux | \
>                 awk 'END{print $$3}')
>  LDFLAGS_vmlinux = --defsym _kernel_bss_size=$(KBSS_SZ)
> +
> +KERNEL_IMAGE_SIZE = $(shell stat -c '%s' $(obj)/../Image)
> +LDFLAGS_vmlinux += --defsym _kernel_image_size=$(KERNEL_IMAGE_SIZE)
> +
>  # Supply ZRELADDR to the decompressor via a linker symbol.
>  ifneq ($(CONFIG_AUTO_ZRELADDR),y)
>  LDFLAGS_vmlinux += --defsym zreladdr=$(ZRELADDR)
> diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S
> index e2e0dcb42ca2..395c60dcc4f7 100644
> --- a/arch/arm/boot/compressed/head.S
> +++ b/arch/arm/boot/compressed/head.S
> @@ -131,11 +131,7 @@ start:
>   THUMB(                badr    r12, 1f         )
>   THUMB(                bx      r12             )
>
> -               .word   _magic_sig      @ Magic numbers to help the loader
> -               .word   _magic_start    @ absolute load/run zImage address
> -               .word   _magic_end      @ zImage end address
> -               .word   0x04030201      @ endianness flag
> -
> +               .section ".start2", #alloc, #execinstr
>   THUMB(                .thumb                  )
>  1:             __EFI_HEADER
>
> diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
> index 81c493156ce8..77267724ec8a 100644
> --- a/arch/arm/boot/compressed/vmlinux.lds.S
> +++ b/arch/arm/boot/compressed/vmlinux.lds.S
> @@ -37,6 +37,19 @@ SECTIONS
>    .text : {
>      _start = .;
>      *(.start)
> +    _header = .;
> +    LONG(ZIMAGE_MAGIC(0x016f2818));    /* Magic numbers to help the loader */
> +    LONG(ZIMAGE_MAGIC(_start));                /* absolute load/run zImage address */
> +    LONG(ZIMAGE_MAGIC(_edata));                /* zImage end address */
> +    LONG(0x04030201);                  /* Endianness flag */
> +    LONG(ZIMAGE_MAGIC(0x016f2818));    /* Further header indicator */
> +    LONG(ZIMAGE_MAGIC(1));             /* Version 1 */
> +    LONG(ZIMAGE_MAGIC(((_kernel_image_size + 255) & ~ 255) + \
> +                       _edata - _text + _end_stack - __bss_start));
> +    LONG(ZIMAGE_MAGIC(_kernel_image_size));
> +    /* Reserve 64 bytes for the header block */
> +    . = _header + 64;
> +    *(.start2)
>      *(.text)
>      *(.text.*)
>      *(.fixup)
> @@ -72,10 +85,6 @@ SECTIONS
>    .pad                 : { BYTE(0); . = ALIGN(8); }
>    _edata = .;
>
> -  _magic_sig = ZIMAGE_MAGIC(0x016f2818);
> -  _magic_start = ZIMAGE_MAGIC(_start);
> -  _magic_end = ZIMAGE_MAGIC(_edata);
> -
>    . = BSS_START;
>    __bss_start = .;
>    .bss                 : { *(.bss) }
> @@ -83,6 +92,7 @@ SECTIONS
>
>    . = ALIGN(8);                /* the stack must be 64-bit aligned */
>    .stack               : { *(.stack) }
> +  _end_stack = .;
>
>    .stab 0              : { *(.stab) }
>    .stabstr 0           : { *(.stabstr) }

Regardless, this looks good to me!

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

Re: kexec failures with DEBUG_RODATA

[Kees Cook]

On Wed, Jun 15, 2016 at 2:13 PM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> On Tue, Jun 14, 2016 at 11:05:23AM -0700, Kees Cook wrote:
>> I'm much less familiar with the ARM decompression stub, but is there a
>> boot image header (like x86 has)? If not, perhaps we can invent one,
>> and it can carry all the details needed for a bootloader to do the
>> right things.
>
> With a bit of tinkering around, I now have this:
>
> 00000000 <.data>:
>        0:       e1a00000        nop                     ; (mov r0, r0)
>        4:       e1a00000        nop                     ; (mov r0, r0)
>        8:       e1a00000        nop                     ; (mov r0, r0)
>        c:       e1a00000        nop                     ; (mov r0, r0)
>       10:       e1a00000        nop                     ; (mov r0, r0)
>       14:       e1a00000        nop                     ; (mov r0, r0)
>       18:       e1a00000        nop                     ; (mov r0, r0)
>       1c:       e1a00000        nop                     ; (mov r0, r0)
>       20:       ea00000f        b       0x64
>
> Then follows the existing "header" which we've had there for years:
>
>       24:       016f2818        ; LE magic number
>       28:       00000000        ; LE zImage start address (always zero now)
>       2c:       00431fe0        ; LE zImage _edata
>       30:       04030201        ; endian flag
>
> And now comes the new header:
>
>       34:       016f2818        ; LE magic number

Should this be a different magic from the existing header's magic?

>       38:       00000001        ; LE version number (v1)

Should a "size" follow the version number instead of the explicit 64
bytes of zeros?

>       3c:       01287000        ; LE total space required for decompressor
>       40:       00e54000        ; LE uncompressed image size
>
> Up to 64 bytes available here for future expansion, currently filled
> with zeros.
>         ...
>
> Remainder of the zImage code:
>       64:       e10f9000        mrs     r9, CPSR
>
> I'm rather on the fence whether we need to give the uncompressed image
> size - the important thing is the size of memory that's required for
> the decompressor to run, which is sizeof(uncompressed kernel) rounded
> up to 256 bytes, and the relocated decompressor image size.

I think it's important information since it allows a boot loader to
figure out if there's room in a given range for the result. While
there's no relocation support yet, if we gain it on ARM and we want to
support KASLR, it will be very handy to have the uncompressed size
available.

>
> The "total space required for decompressor" is slightly cheating at the
> figure - I'm including the uncompressed image rounded up and the entire
> compressed image in that size, so it's a safe over-estimate.
>
> I'm not sure there's a need to provide the uncompressed image size, the
> boot environment shouldn't have a reason to know that, so I'm tempted to
> omit it.
>
> We could dispense with the endian conversions, and push the responsibility
> for interpreting that onto the reader of this data: we have the endian
> flag in the existing header block, so the boot environment can work out
> the endianness of the image and apply fixups as appropriate.
>
> Why generate this in the linker script?  We need the size of the zImage
> here, which is only known to the linker.
>
> diff --git a/arch/arm/boot/compressed/Makefile b/arch/arm/boot/compressed/Makefile
> index d50430c40045..1d5467e05250 100644
> --- a/arch/arm/boot/compressed/Makefile
> +++ b/arch/arm/boot/compressed/Makefile
> @@ -119,6 +119,10 @@ asflags-y := -DZIMAGE
>  KBSS_SZ = $(shell $(CROSS_COMPILE)size $(obj)/../../../../vmlinux | \
>                 awk 'END{print $$3}')
>  LDFLAGS_vmlinux = --defsym _kernel_bss_size=$(KBSS_SZ)
> +
> +KERNEL_IMAGE_SIZE = $(shell stat -c '%s' $(obj)/../Image)
> +LDFLAGS_vmlinux += --defsym _kernel_image_size=$(KERNEL_IMAGE_SIZE)
> +
>  # Supply ZRELADDR to the decompressor via a linker symbol.
>  ifneq ($(CONFIG_AUTO_ZRELADDR),y)
>  LDFLAGS_vmlinux += --defsym zreladdr=$(ZRELADDR)
> diff --git a/arch/arm/boot/compressed/head.S b/arch/arm/boot/compressed/head.S
> index e2e0dcb42ca2..395c60dcc4f7 100644
> --- a/arch/arm/boot/compressed/head.S
> +++ b/arch/arm/boot/compressed/head.S
> @@ -131,11 +131,7 @@ start:
>   THUMB(                badr    r12, 1f         )
>   THUMB(                bx      r12             )
>
> -               .word   _magic_sig      @ Magic numbers to help the loader
> -               .word   _magic_start    @ absolute load/run zImage address
> -               .word   _magic_end      @ zImage end address
> -               .word   0x04030201      @ endianness flag
> -
> +               .section ".start2", #alloc, #execinstr
>   THUMB(                .thumb                  )
>  1:             __EFI_HEADER
>
> diff --git a/arch/arm/boot/compressed/vmlinux.lds.S b/arch/arm/boot/compressed/vmlinux.lds.S
> index 81c493156ce8..77267724ec8a 100644
> --- a/arch/arm/boot/compressed/vmlinux.lds.S
> +++ b/arch/arm/boot/compressed/vmlinux.lds.S
> @@ -37,6 +37,19 @@ SECTIONS
>    .text : {
>      _start = .;
>      *(.start)
> +    _header = .;
> +    LONG(ZIMAGE_MAGIC(0x016f2818));    /* Magic numbers to help the loader */
> +    LONG(ZIMAGE_MAGIC(_start));                /* absolute load/run zImage address */
> +    LONG(ZIMAGE_MAGIC(_edata));                /* zImage end address */
> +    LONG(0x04030201);                  /* Endianness flag */
> +    LONG(ZIMAGE_MAGIC(0x016f2818));    /* Further header indicator */
> +    LONG(ZIMAGE_MAGIC(1));             /* Version 1 */
> +    LONG(ZIMAGE_MAGIC(((_kernel_image_size + 255) & ~ 255) + \
> +                       _edata - _text + _end_stack - __bss_start));
> +    LONG(ZIMAGE_MAGIC(_kernel_image_size));
> +    /* Reserve 64 bytes for the header block */
> +    . = _header + 64;
> +    *(.start2)
>      *(.text)
>      *(.text.*)
>      *(.fixup)
> @@ -72,10 +85,6 @@ SECTIONS
>    .pad                 : { BYTE(0); . = ALIGN(8); }
>    _edata = .;
>
> -  _magic_sig = ZIMAGE_MAGIC(0x016f2818);
> -  _magic_start = ZIMAGE_MAGIC(_start);
> -  _magic_end = ZIMAGE_MAGIC(_edata);
> -
>    . = BSS_START;
>    __bss_start = .;
>    .bss                 : { *(.bss) }
> @@ -83,6 +92,7 @@ SECTIONS
>
>    . = ALIGN(8);                /* the stack must be 64-bit aligned */
>    .stack               : { *(.stack) }
> +  _end_stack = .;
>
>    .stab 0              : { *(.stab) }
>    .stabstr 0           : { *(.stabstr) }

Regardless, this looks good to me!

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

kexec failures with DEBUG_RODATA

[Russell King - ARM Linux]

On Wed, Jun 15, 2016 at 03:20:05PM -0700, Kees Cook wrote:
> On Wed, Jun 15, 2016 at 2:13 PM, Russell King - ARM Linux
> <linux@armlinux.org.uk> wrote:
> > On Tue, Jun 14, 2016 at 11:05:23AM -0700, Kees Cook wrote:
> >> I'm much less familiar with the ARM decompression stub, but is there a
> >> boot image header (like x86 has)? If not, perhaps we can invent one,
> >> and it can carry all the details needed for a bootloader to do the
> >> right things.
> >
> > With a bit of tinkering around, I now have this:
> >
> > 00000000 <.data>:
> >        0:       e1a00000        nop                     ; (mov r0, r0)
> >        4:       e1a00000        nop                     ; (mov r0, r0)
> >        8:       e1a00000        nop                     ; (mov r0, r0)
> >        c:       e1a00000        nop                     ; (mov r0, r0)
> >       10:       e1a00000        nop                     ; (mov r0, r0)
> >       14:       e1a00000        nop                     ; (mov r0, r0)
> >       18:       e1a00000        nop                     ; (mov r0, r0)
> >       1c:       e1a00000        nop                     ; (mov r0, r0)
> >       20:       ea00000f        b       0x64
> >
> > Then follows the existing "header" which we've had there for years:
> >
> >       24:       016f2818        ; LE magic number
> >       28:       00000000        ; LE zImage start address (always zero now)
> >       2c:       00431fe0        ; LE zImage _edata
> >       30:       04030201        ; endian flag
> >
> > And now comes the new header:
> >
> >       34:       016f2818        ; LE magic number
> 
> Should this be a different magic from the existing header's magic?

What use would a different number be?  It's not something you scan for,
because this block is always at 0x24 bytes into the zImage for a binary
zImage.

> >       38:       00000001        ; LE version number (v1)
> 
> Should a "size" follow the version number instead of the explicit 64
> bytes of zeros?

I don't see the use of a size field.  If we have a size field, then
we need to validate the version number (which tells us the format of
the data) against the size, and it all gets really messy - what if
we have a version number which indicates that we have more data than
the size field indicates?

> >       3c:       01287000        ; LE total space required for decompressor
> >       40:       00e54000        ; LE uncompressed image size
> >
> > Up to 64 bytes available here for future expansion, currently filled
> > with zeros.
> >         ...
> >
> > Remainder of the zImage code:
> >       64:       e10f9000        mrs     r9, CPSR
> >
> > I'm rather on the fence whether we need to give the uncompressed image
> > size - the important thing is the size of memory that's required for
> > the decompressor to run, which is sizeof(uncompressed kernel) rounded
> > up to 256 bytes, and the relocated decompressor image size.
> 
> I think it's important information since it allows a boot loader to
> figure out if there's room in a given range for the result. While
> there's no relocation support yet, if we gain it on ARM and we want to
> support KASLR, it will be very handy to have the uncompressed size
> available.

It's really not useful for that purpose.  The size of the uncompressed
image doesn't tell you that you can touch the next few bytes after the
image.  In fact, you can't, because that's where the decompressor
potentially relocates itself to.

The one which is of importance to the boot environment layout is the
"total space required for decompressor", which is the one which must
be respected.

In fact, the apparent confusion over this reinforces my belief that we
should _not_ give the size of the uncompressed image at all.

The boot environment must be setup such that there is room for the
uncompressed image (aligned currently to 256 bytes) followed by the
size of the compressed image, with any appended DTBs included.
Anything which is located below that is likely to get trampled by
the decompressor.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: kexec failures with DEBUG_RODATA

[Russell King - ARM Linux]

On Wed, Jun 15, 2016 at 03:20:05PM -0700, Kees Cook wrote:
> On Wed, Jun 15, 2016 at 2:13 PM, Russell King - ARM Linux
> <linux@armlinux.org.uk> wrote:
> > On Tue, Jun 14, 2016 at 11:05:23AM -0700, Kees Cook wrote:
> >> I'm much less familiar with the ARM decompression stub, but is there a
> >> boot image header (like x86 has)? If not, perhaps we can invent one,
> >> and it can carry all the details needed for a bootloader to do the
> >> right things.
> >
> > With a bit of tinkering around, I now have this:
> >
> > 00000000 <.data>:
> >        0:       e1a00000        nop                     ; (mov r0, r0)
> >        4:       e1a00000        nop                     ; (mov r0, r0)
> >        8:       e1a00000        nop                     ; (mov r0, r0)
> >        c:       e1a00000        nop                     ; (mov r0, r0)
> >       10:       e1a00000        nop                     ; (mov r0, r0)
> >       14:       e1a00000        nop                     ; (mov r0, r0)
> >       18:       e1a00000        nop                     ; (mov r0, r0)
> >       1c:       e1a00000        nop                     ; (mov r0, r0)
> >       20:       ea00000f        b       0x64
> >
> > Then follows the existing "header" which we've had there for years:
> >
> >       24:       016f2818        ; LE magic number
> >       28:       00000000        ; LE zImage start address (always zero now)
> >       2c:       00431fe0        ; LE zImage _edata
> >       30:       04030201        ; endian flag
> >
> > And now comes the new header:
> >
> >       34:       016f2818        ; LE magic number
> 
> Should this be a different magic from the existing header's magic?

What use would a different number be?  It's not something you scan for,
because this block is always at 0x24 bytes into the zImage for a binary
zImage.

> >       38:       00000001        ; LE version number (v1)
> 
> Should a "size" follow the version number instead of the explicit 64
> bytes of zeros?

I don't see the use of a size field.  If we have a size field, then
we need to validate the version number (which tells us the format of
the data) against the size, and it all gets really messy - what if
we have a version number which indicates that we have more data than
the size field indicates?

> >       3c:       01287000        ; LE total space required for decompressor
> >       40:       00e54000        ; LE uncompressed image size
> >
> > Up to 64 bytes available here for future expansion, currently filled
> > with zeros.
> >         ...
> >
> > Remainder of the zImage code:
> >       64:       e10f9000        mrs     r9, CPSR
> >
> > I'm rather on the fence whether we need to give the uncompressed image
> > size - the important thing is the size of memory that's required for
> > the decompressor to run, which is sizeof(uncompressed kernel) rounded
> > up to 256 bytes, and the relocated decompressor image size.
> 
> I think it's important information since it allows a boot loader to
> figure out if there's room in a given range for the result. While
> there's no relocation support yet, if we gain it on ARM and we want to
> support KASLR, it will be very handy to have the uncompressed size
> available.

It's really not useful for that purpose.  The size of the uncompressed
image doesn't tell you that you can touch the next few bytes after the
image.  In fact, you can't, because that's where the decompressor
potentially relocates itself to.

The one which is of importance to the boot environment layout is the
"total space required for decompressor", which is the one which must
be respected.

In fact, the apparent confusion over this reinforces my belief that we
should _not_ give the size of the uncompressed image at all.

The boot environment must be setup such that there is room for the
uncompressed image (aligned currently to 256 bytes) followed by the
size of the compressed image, with any appended DTBs included.
Anything which is located below that is likely to get trampled by
the decompressor.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

kexec failures with DEBUG_RODATA

[Kees Cook]

On Wed, Jun 15, 2016 at 3:42 PM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> In fact, the apparent confusion over this reinforces my belief that we
> should _not_ give the size of the uncompressed image at all.
>
> The boot environment must be setup such that there is room for the
> uncompressed image (aligned currently to 256 bytes) followed by the
> size of the compressed image, with any appended DTBs included.
> Anything which is located below that is likely to get trampled by
> the decompressor.

Okay, sounds reasonable to me. :)

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

Re: kexec failures with DEBUG_RODATA

[Kees Cook]

On Wed, Jun 15, 2016 at 3:42 PM, Russell King - ARM Linux
<linux@armlinux.org.uk> wrote:
> In fact, the apparent confusion over this reinforces my belief that we
> should _not_ give the size of the uncompressed image at all.
>
> The boot environment must be setup such that there is room for the
> uncompressed image (aligned currently to 256 bytes) followed by the
> size of the compressed image, with any appended DTBs included.
> Anything which is located below that is likely to get trampled by
> the decompressor.

Okay, sounds reasonable to me. :)

-Kees

-- 
Kees Cook
Chrome OS & Brillo Security

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

kexec failures with DEBUG_RODATA

[Russell King - ARM Linux]

On Wed, Jun 15, 2016 at 03:54:38PM -0700, Kees Cook wrote:
> On Wed, Jun 15, 2016 at 3:42 PM, Russell King - ARM Linux
> <linux@armlinux.org.uk> wrote:
> > In fact, the apparent confusion over this reinforces my belief that we
> > should _not_ give the size of the uncompressed image at all.
> >
> > The boot environment must be setup such that there is room for the
> > uncompressed image (aligned currently to 256 bytes) followed by the
> > size of the compressed image, with any appended DTBs included.
> > Anything which is located below that is likely to get trampled by
> > the decompressor.
> 
> Okay, sounds reasonable to me. :)

I should point out that this method should work for a zImage without
an appended DTB, and we have no way to update this header block for
the appended DTB case.

So, an alternative standpoint is that we supply only the uncompressed
image size.  Then, the boot environment needs to understand that they
must allow for the compressed image and any appended DTB on top of that
(which it would see as one - the size of the combined image.)

However, while that may sound like a good idea, we're falling into the
same trap that we've fallen into at the beginning of this thread: the
boot environment has to understand how the decompressor currently works,
and if we were to change that, this we're back to the calculation which
the boot environment is using not matching reality.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: kexec failures with DEBUG_RODATA

[Russell King - ARM Linux]

On Wed, Jun 15, 2016 at 03:54:38PM -0700, Kees Cook wrote:
> On Wed, Jun 15, 2016 at 3:42 PM, Russell King - ARM Linux
> <linux@armlinux.org.uk> wrote:
> > In fact, the apparent confusion over this reinforces my belief that we
> > should _not_ give the size of the uncompressed image at all.
> >
> > The boot environment must be setup such that there is room for the
> > uncompressed image (aligned currently to 256 bytes) followed by the
> > size of the compressed image, with any appended DTBs included.
> > Anything which is located below that is likely to get trampled by
> > the decompressor.
> 
> Okay, sounds reasonable to me. :)

I should point out that this method should work for a zImage without
an appended DTB, and we have no way to update this header block for
the appended DTB case.

So, an alternative standpoint is that we supply only the uncompressed
image size.  Then, the boot environment needs to understand that they
must allow for the compressed image and any appended DTB on top of that
(which it would see as one - the size of the combined image.)

However, while that may sound like a good idea, we're falling into the
same trap that we've fallen into at the beginning of this thread: the
boot environment has to understand how the decompressor currently works,
and if we were to change that, this we're back to the calculation which
the boot environment is using not matching reality.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

kexec failures with DEBUG_RODATA

[Pratyush Anand]

Hi Russell,

On 16/06/2016:12:13:03 AM, Russell King - ARM Linux wrote:
> On Wed, Jun 15, 2016 at 03:54:38PM -0700, Kees Cook wrote:
> > On Wed, Jun 15, 2016 at 3:42 PM, Russell King - ARM Linux
> > <linux@armlinux.org.uk> wrote:
> > > In fact, the apparent confusion over this reinforces my belief that we
> > > should _not_ give the size of the uncompressed image at all.
> > >
> > > The boot environment must be setup such that there is room for the
> > > uncompressed image (aligned currently to 256 bytes) followed by the
> > > size of the compressed image, with any appended DTBs included.
> > > Anything which is located below that is likely to get trampled by
> > > the decompressor.
> > 
> > Okay, sounds reasonable to me. :)
> 
> I should point out that this method should work for a zImage without
> an appended DTB, and we have no way to update this header block for
> the appended DTB case.

Well, I might be missing with my limited knowledge. So, just to clarify, zImage
is "compressed image + uncompressor". In some cases it might have "+DTB".
So, zImage should be located atleast "aligned uncompressed size" above "kernel
base" and, initrd should be placed at "kernel base" +  "aligned uncompressed
size" + "compressed image size" + "uncompressor size" + DTB(if any).

However, I am unable to understand that why can't we have a flag in zImage
header block, which tells that whether a DTB has been appended in the zImage or
not?

> 
> So, an alternative standpoint is that we supply only the uncompressed
> image size.  Then, the boot environment needs to understand that they
> must allow for the compressed image and any appended DTB on top of that
> (which it would see as one - the size of the combined image.)

So, why not we can have all three information in header ie. "uncompressed image
size", "compressed image size" and "DTB size" if appended flag is set.

> 
> However, while that may sound like a good idea, we're falling into the
> same trap that we've fallen into at the beginning of this thread: the
> boot environment has to understand how the decompressor currently works,
> and if we were to change that, this we're back to the calculation which
> the boot environment is using not matching reality.

Currently we are discussing between 4 times to 11 times. So, if we know
*atleast* "uncompressed image size" then this heuristic variation can be
minimized a lot. If we do not provide header, then may be we can provide library
to the kexec-tools which will enable us to know the length of "uncompressed
image" when "compressed image" is provided as input.

~Pratyush

> 
> -- 
> RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
> according to speedtest.net.

Re: kexec failures with DEBUG_RODATA

[Pratyush Anand]

Hi Russell,

On 16/06/2016:12:13:03 AM, Russell King - ARM Linux wrote:
> On Wed, Jun 15, 2016 at 03:54:38PM -0700, Kees Cook wrote:
> > On Wed, Jun 15, 2016 at 3:42 PM, Russell King - ARM Linux
> > <linux@armlinux.org.uk> wrote:
> > > In fact, the apparent confusion over this reinforces my belief that we
> > > should _not_ give the size of the uncompressed image at all.
> > >
> > > The boot environment must be setup such that there is room for the
> > > uncompressed image (aligned currently to 256 bytes) followed by the
> > > size of the compressed image, with any appended DTBs included.
> > > Anything which is located below that is likely to get trampled by
> > > the decompressor.
> > 
> > Okay, sounds reasonable to me. :)
> 
> I should point out that this method should work for a zImage without
> an appended DTB, and we have no way to update this header block for
> the appended DTB case.

Well, I might be missing with my limited knowledge. So, just to clarify, zImage
is "compressed image + uncompressor". In some cases it might have "+DTB".
So, zImage should be located atleast "aligned uncompressed size" above "kernel
base" and, initrd should be placed at "kernel base" +  "aligned uncompressed
size" + "compressed image size" + "uncompressor size" + DTB(if any).

However, I am unable to understand that why can't we have a flag in zImage
header block, which tells that whether a DTB has been appended in the zImage or
not?

> 
> So, an alternative standpoint is that we supply only the uncompressed
> image size.  Then, the boot environment needs to understand that they
> must allow for the compressed image and any appended DTB on top of that
> (which it would see as one - the size of the combined image.)

So, why not we can have all three information in header ie. "uncompressed image
size", "compressed image size" and "DTB size" if appended flag is set.

> 
> However, while that may sound like a good idea, we're falling into the
> same trap that we've fallen into at the beginning of this thread: the
> boot environment has to understand how the decompressor currently works,
> and if we were to change that, this we're back to the calculation which
> the boot environment is using not matching reality.

Currently we are discussing between 4 times to 11 times. So, if we know
*atleast* "uncompressed image size" then this heuristic variation can be
minimized a lot. If we do not provide header, then may be we can provide library
to the kexec-tools which will enable us to know the length of "uncompressed
image" when "compressed image" is provided as input.

~Pratyush

> 
> -- 
> RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
> FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
> according to speedtest.net.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

kexec failures with DEBUG_RODATA

[Russell King - ARM Linux]

On Tue, Jun 21, 2016 at 05:18:49PM +0530, Pratyush Anand wrote:
> On 16/06/2016:12:13:03 AM, Russell King - ARM Linux wrote:
> > I should point out that this method should work for a zImage without
> > an appended DTB, and we have no way to update this header block for
> > the appended DTB case.
> 
> Well, I might be missing with my limited knowledge. So, just to clarify, zImage
> is "compressed image + uncompressor". In some cases it might have "+DTB".
> So, zImage should be located atleast "aligned uncompressed size" above "kernel
> base" and, initrd should be placed at "kernel base" +  "aligned uncompressed
> size" + "compressed image size" + "uncompressor size" + DTB(if any).
> 
> However, I am unable to understand that why can't we have a flag in zImage
> header block, which tells that whether a DTB has been appended in the zImage or
> not?

cat zImage foo.dtb > newImage

obviously doesn't result in the header block inside zImage being modified.
The only way around that is to have a tool which concatenates the two
and updates the header.  At that point, it's no longer simply a
concatenation.

> > So, an alternative standpoint is that we supply only the uncompressed
> > image size.  Then, the boot environment needs to understand that they
> > must allow for the compressed image and any appended DTB on top of that
> > (which it would see as one - the size of the combined image.)
> 
> So, why not we can have all three information in header ie. "uncompressed image
> size", "compressed image size" and "DTB size" if appended flag is set.

How big is the DTB size?  Without a tool to update the header, there's
no way to really know.

> > However, while that may sound like a good idea, we're falling into the
> > same trap that we've fallen into at the beginning of this thread: the
> > boot environment has to understand how the decompressor currently works,
> > and if we were to change that, this we're back to the calculation which
> > the boot environment is using not matching reality.
> 
> Currently we are discussing between 4 times to 11 times. So, if we know
> *atleast* "uncompressed image size" then this heuristic variation can be
> minimized a lot. If we do not provide header, then may be we can provide library
> to the kexec-tools which will enable us to know the length of "uncompressed
> image" when "compressed image" is provided as input.

If we provide the uncompressed size, then we know how big the uncompressed
image will be.  That much is fine, but there's more than that.

What is the padding applied to the uncompressed size?  Where does the
compressed image self-relocate to?  Where does the compressed image
relocate the DTB to?  What is the spacing between the DTB and the
compressed image?

All those are knowledge which is internal to the decompressor code.
If we supply, say, the padded uncompressed size, then we can work
around not knowing the padding in kexec.  However, we still need to
know the other details - so we still end up needing to code into
kexec (or other boot environment) a load of details specific to the
current decompressor implementation.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: kexec failures with DEBUG_RODATA

[Russell King - ARM Linux]

On Tue, Jun 21, 2016 at 05:18:49PM +0530, Pratyush Anand wrote:
> On 16/06/2016:12:13:03 AM, Russell King - ARM Linux wrote:
> > I should point out that this method should work for a zImage without
> > an appended DTB, and we have no way to update this header block for
> > the appended DTB case.
> 
> Well, I might be missing with my limited knowledge. So, just to clarify, zImage
> is "compressed image + uncompressor". In some cases it might have "+DTB".
> So, zImage should be located atleast "aligned uncompressed size" above "kernel
> base" and, initrd should be placed at "kernel base" +  "aligned uncompressed
> size" + "compressed image size" + "uncompressor size" + DTB(if any).
> 
> However, I am unable to understand that why can't we have a flag in zImage
> header block, which tells that whether a DTB has been appended in the zImage or
> not?

cat zImage foo.dtb > newImage

obviously doesn't result in the header block inside zImage being modified.
The only way around that is to have a tool which concatenates the two
and updates the header.  At that point, it's no longer simply a
concatenation.

> > So, an alternative standpoint is that we supply only the uncompressed
> > image size.  Then, the boot environment needs to understand that they
> > must allow for the compressed image and any appended DTB on top of that
> > (which it would see as one - the size of the combined image.)
> 
> So, why not we can have all three information in header ie. "uncompressed image
> size", "compressed image size" and "DTB size" if appended flag is set.

How big is the DTB size?  Without a tool to update the header, there's
no way to really know.

> > However, while that may sound like a good idea, we're falling into the
> > same trap that we've fallen into at the beginning of this thread: the
> > boot environment has to understand how the decompressor currently works,
> > and if we were to change that, this we're back to the calculation which
> > the boot environment is using not matching reality.
> 
> Currently we are discussing between 4 times to 11 times. So, if we know
> *atleast* "uncompressed image size" then this heuristic variation can be
> minimized a lot. If we do not provide header, then may be we can provide library
> to the kexec-tools which will enable us to know the length of "uncompressed
> image" when "compressed image" is provided as input.

If we provide the uncompressed size, then we know how big the uncompressed
image will be.  That much is fine, but there's more than that.

What is the padding applied to the uncompressed size?  Where does the
compressed image self-relocate to?  Where does the compressed image
relocate the DTB to?  What is the spacing between the DTB and the
compressed image?

All those are knowledge which is internal to the decompressor code.
If we supply, say, the padded uncompressed size, then we can work
around not knowing the padding in kexec.  However, we still need to
know the other details - so we still end up needing to code into
kexec (or other boot environment) a load of details specific to the
current decompressor implementation.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

kexec failures with DEBUG_RODATA

[Russell King - ARM Linux]

On Wed, Jun 15, 2016 at 10:13:59PM +0100, Russell King - ARM Linux wrote:
> On Tue, Jun 14, 2016 at 11:05:23AM -0700, Kees Cook wrote:
> > I'm much less familiar with the ARM decompression stub, but is there a
> > boot image header (like x86 has)? If not, perhaps we can invent one,
> > and it can carry all the details needed for a bootloader to do the
> > right things.
> 
> With a bit of tinkering around, I now have this:
> 
> 00000000 <.data>:
>        0:       e1a00000        nop                     ; (mov r0, r0)
>        4:       e1a00000        nop                     ; (mov r0, r0)
>        8:       e1a00000        nop                     ; (mov r0, r0)
>        c:       e1a00000        nop                     ; (mov r0, r0)
>       10:       e1a00000        nop                     ; (mov r0, r0)
>       14:       e1a00000        nop                     ; (mov r0, r0)
>       18:       e1a00000        nop                     ; (mov r0, r0)
>       1c:       e1a00000        nop                     ; (mov r0, r0)
>       20:       ea00000f        b       0x64
> 
> Then follows the existing "header" which we've had there for years:
> 
>       24:       016f2818	; LE magic number
>       28:       00000000	; LE zImage start address (always zero now)
>       2c:       00431fe0	; LE zImage _edata
>       30:       04030201	; endian flag
> 
> And now comes the new header:
> 
>       34:       016f2818	; LE magic number
>       38:       00000001	; LE version number (v1)
>       3c:       01287000	; LE total space required for decompressor
>       40:       00e54000	; LE uncompressed image size
> 
> Up to 64 bytes available here for future expansion, currently filled
> with zeros.
>         ...
> 
> Remainder of the zImage code:
>       64:       e10f9000        mrs     r9, CPSR

Looking at this again, this can't work for another _two_ reasons:

1. Thumb2 kernels - we rely on a relative address to jump to for the
   remainder of the zImage code, using an "adr" instruction.  The offset
   needs to be known at assembly time, but with my approach of moving
   the header into the linker script, this is no longer the case.

2. EFI images need the header offset at 0x3c, which gets in the way of
   us appending to our existing header.

So... I don't think we can solve it this way, and I'm all out of ideas
how to solve this in a sane manner - I'm currently of the opinion that
it _isn't_ solvable given where we are without defining a new format
for a zImage, which is really quite depressing.

I think people are just going to have to get used to using --image-size
to kexec on ARM to work around this problem.  So, let's document it
in kexec's --help - though this could do with something better, but I
fear it'll turn into a multi-line description of the option:

 kexec/arch/arm/kexec-zImage-arm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kexec/arch/arm/kexec-zImage-arm.c b/kexec/arch/arm/kexec-zImage-arm.c
index 9400d1f4..45c587c2 100644
--- a/kexec/arch/arm/kexec-zImage-arm.c
+++ b/kexec/arch/arm/kexec-zImage-arm.c
@@ -122,6 +122,8 @@ void zImage_arm_usage(void)
 		"     --initrd=FILE         Use FILE as the kernel's initial ramdisk.\n"
 		"     --ramdisk=FILE        Use FILE as the kernel's initial ramdisk.\n"
 		"     --dtb=FILE            Use FILE as the fdt blob.\n"
+		"     --image-size=IMAGE_SIZE\n"
+		"                           Reserve IMAGE_SIZE between kernel and initrd.\n"
 		"     --atags               Use ATAGs instead of device-tree.\n"
 		"     --page-offset=PAGE_OFFSET\n"
 		"                           Set PAGE_OFFSET of crash dump vmcore\n"


-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: kexec failures with DEBUG_RODATA

[Russell King - ARM Linux]

On Wed, Jun 15, 2016 at 10:13:59PM +0100, Russell King - ARM Linux wrote:
> On Tue, Jun 14, 2016 at 11:05:23AM -0700, Kees Cook wrote:
> > I'm much less familiar with the ARM decompression stub, but is there a
> > boot image header (like x86 has)? If not, perhaps we can invent one,
> > and it can carry all the details needed for a bootloader to do the
> > right things.
> 
> With a bit of tinkering around, I now have this:
> 
> 00000000 <.data>:
>        0:       e1a00000        nop                     ; (mov r0, r0)
>        4:       e1a00000        nop                     ; (mov r0, r0)
>        8:       e1a00000        nop                     ; (mov r0, r0)
>        c:       e1a00000        nop                     ; (mov r0, r0)
>       10:       e1a00000        nop                     ; (mov r0, r0)
>       14:       e1a00000        nop                     ; (mov r0, r0)
>       18:       e1a00000        nop                     ; (mov r0, r0)
>       1c:       e1a00000        nop                     ; (mov r0, r0)
>       20:       ea00000f        b       0x64
> 
> Then follows the existing "header" which we've had there for years:
> 
>       24:       016f2818	; LE magic number
>       28:       00000000	; LE zImage start address (always zero now)
>       2c:       00431fe0	; LE zImage _edata
>       30:       04030201	; endian flag
> 
> And now comes the new header:
> 
>       34:       016f2818	; LE magic number
>       38:       00000001	; LE version number (v1)
>       3c:       01287000	; LE total space required for decompressor
>       40:       00e54000	; LE uncompressed image size
> 
> Up to 64 bytes available here for future expansion, currently filled
> with zeros.
>         ...
> 
> Remainder of the zImage code:
>       64:       e10f9000        mrs     r9, CPSR

Looking at this again, this can't work for another _two_ reasons:

1. Thumb2 kernels - we rely on a relative address to jump to for the
   remainder of the zImage code, using an "adr" instruction.  The offset
   needs to be known at assembly time, but with my approach of moving
   the header into the linker script, this is no longer the case.

2. EFI images need the header offset at 0x3c, which gets in the way of
   us appending to our existing header.

So... I don't think we can solve it this way, and I'm all out of ideas
how to solve this in a sane manner - I'm currently of the opinion that
it _isn't_ solvable given where we are without defining a new format
for a zImage, which is really quite depressing.

I think people are just going to have to get used to using --image-size
to kexec on ARM to work around this problem.  So, let's document it
in kexec's --help - though this could do with something better, but I
fear it'll turn into a multi-line description of the option:

 kexec/arch/arm/kexec-zImage-arm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/kexec/arch/arm/kexec-zImage-arm.c b/kexec/arch/arm/kexec-zImage-arm.c
index 9400d1f4..45c587c2 100644
--- a/kexec/arch/arm/kexec-zImage-arm.c
+++ b/kexec/arch/arm/kexec-zImage-arm.c
@@ -122,6 +122,8 @@ void zImage_arm_usage(void)
 		"     --initrd=FILE         Use FILE as the kernel's initial ramdisk.\n"
 		"     --ramdisk=FILE        Use FILE as the kernel's initial ramdisk.\n"
 		"     --dtb=FILE            Use FILE as the fdt blob.\n"
+		"     --image-size=IMAGE_SIZE\n"
+		"                           Reserve IMAGE_SIZE between kernel and initrd.\n"
 		"     --atags               Use ATAGs instead of device-tree.\n"
 		"     --page-offset=PAGE_OFFSET\n"
 		"                           Set PAGE_OFFSET of crash dump vmcore\n"


-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

kexec failures with DEBUG_RODATA

[Russell King - ARM Linux]

On Wed, Jun 15, 2016 at 11:42:28PM +0100, Russell King - ARM Linux wrote:
> The boot environment must be setup such that there is room for the
> uncompressed image (aligned currently to 256 bytes) followed by the
> size of the compressed image, with any appended DTBs included.
> Anything which is located below that is likely to get trampled by
> the decompressor.

There's a question that's been lingering, which hasn't been brought up
by anyone yet: should we even support zImage files with an appended
DTB for kexec?

When kexec is operating in DTB mode, it will provide the DTB to the
kernel from the host environment, possibly with some modifications (eg,
to change the command line arguments.)  Since an appended DTB would
override the kexec-provided DTB, this would wipe out those changes,
some of which are necessary for things like crashdump to work.

So, I'm thinking that kexec should not support a zImage with appended
DTB - in fact, I think it should truncate the zImage to be loaded, and
ensure that the word following the zImage does not contain a DTB magic
number, in case the zImage has been built with CONFIG_ARM_APPENDED_DTB
enabled.

Thoughts?

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: kexec failures with DEBUG_RODATA

[Russell King - ARM Linux]

On Wed, Jun 15, 2016 at 11:42:28PM +0100, Russell King - ARM Linux wrote:
> The boot environment must be setup such that there is room for the
> uncompressed image (aligned currently to 256 bytes) followed by the
> size of the compressed image, with any appended DTBs included.
> Anything which is located below that is likely to get trampled by
> the decompressor.

There's a question that's been lingering, which hasn't been brought up
by anyone yet: should we even support zImage files with an appended
DTB for kexec?

When kexec is operating in DTB mode, it will provide the DTB to the
kernel from the host environment, possibly with some modifications (eg,
to change the command line arguments.)  Since an appended DTB would
override the kexec-provided DTB, this would wipe out those changes,
some of which are necessary for things like crashdump to work.

So, I'm thinking that kexec should not support a zImage with appended
DTB - in fact, I think it should truncate the zImage to be loaded, and
ensure that the word following the zImage does not contain a DTB magic
number, in case the zImage has been built with CONFIG_ARM_APPENDED_DTB
enabled.

Thoughts?

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH 1/2] arm: plug a zImage corner case

[Russell King]

If a zImage is built with ARM_APPENDED_DTB enabled, the zImage will
look at the word following the zImage and check whether it contains
the DTB magic number.

Generally, kexec will clear the destination pages before copying the
zImage, but there is a corner case where the zImage is a multiple of
the page size, where the following page will not be touched.  Should
the first word in this page contain the DTB magic number, the data
following will be interpreted as a DTB image.

In order to make this bullet-proof, we must always initialise the word
following the zImage.  Arrange this by specifying the zImage memory
size one word bigger than the buffer size.

Signed-off-by: Russell King <rmk@armlinux.org.uk>
---
 kexec/arch/arm/kexec-zImage-arm.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/kexec/arch/arm/kexec-zImage-arm.c b/kexec/arch/arm/kexec-zImage-arm.c
index 9400d1f4..c8b90f96 100644
--- a/kexec/arch/arm/kexec-zImage-arm.c
+++ b/kexec/arch/arm/kexec-zImage-arm.c
@@ -343,6 +343,7 @@ int zImage_arm_load(int argc, char **argv, const char *buf, off_t len,
 	unsigned long base, kernel_base;
 	unsigned int atag_offset = 0x1000; /* 4k offset from memory start */
 	unsigned int extra_size = 0x8000; /* TEXT_OFFSET */
+	size_t kernel_mem_size;
 	const char *command_line;
 	char *modified_cmdline = NULL;
 	off_t command_line_len;
@@ -470,6 +471,12 @@ int zImage_arm_load(int argc, char **argv, const char *buf, off_t len,
 	}
 
 	/*
+	 * Always extend the zImage by four bytes to ensure that an appended
+	 * DTB image always sees an initialised value after _edata.
+	 */
+	kernel_mem_size = len + 4;
+
+	/*
 	 * If we are loading a dump capture kernel, we need to update kernel
 	 * command line and also add some additional segments.
 	 */
@@ -621,7 +628,7 @@ int zImage_arm_load(int argc, char **argv, const char *buf, off_t len,
 		            dtb_offset, dtb_length);
 	}
 
-	add_segment(info, buf, len, kernel_base, len);
+	add_segment(info, buf, len, kernel_base, kernel_mem_size);
 
 	info->entry = (void*)kernel_base;
 
-- 
1.9.1

[PATCH 1/2] arm: plug a zImage corner case

[Russell King]

If a zImage is built with ARM_APPENDED_DTB enabled, the zImage will
look at the word following the zImage and check whether it contains
the DTB magic number.

Generally, kexec will clear the destination pages before copying the
zImage, but there is a corner case where the zImage is a multiple of
the page size, where the following page will not be touched.  Should
the first word in this page contain the DTB magic number, the data
following will be interpreted as a DTB image.

In order to make this bullet-proof, we must always initialise the word
following the zImage.  Arrange this by specifying the zImage memory
size one word bigger than the buffer size.

Signed-off-by: Russell King <rmk@armlinux.org.uk>
---
 kexec/arch/arm/kexec-zImage-arm.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/kexec/arch/arm/kexec-zImage-arm.c b/kexec/arch/arm/kexec-zImage-arm.c
index 9400d1f4..c8b90f96 100644
--- a/kexec/arch/arm/kexec-zImage-arm.c
+++ b/kexec/arch/arm/kexec-zImage-arm.c
@@ -343,6 +343,7 @@ int zImage_arm_load(int argc, char **argv, const char *buf, off_t len,
 	unsigned long base, kernel_base;
 	unsigned int atag_offset = 0x1000; /* 4k offset from memory start */
 	unsigned int extra_size = 0x8000; /* TEXT_OFFSET */
+	size_t kernel_mem_size;
 	const char *command_line;
 	char *modified_cmdline = NULL;
 	off_t command_line_len;
@@ -470,6 +471,12 @@ int zImage_arm_load(int argc, char **argv, const char *buf, off_t len,
 	}
 
 	/*
+	 * Always extend the zImage by four bytes to ensure that an appended
+	 * DTB image always sees an initialised value after _edata.
+	 */
+	kernel_mem_size = len + 4;
+
+	/*
 	 * If we are loading a dump capture kernel, we need to update kernel
 	 * command line and also add some additional segments.
 	 */
@@ -621,7 +628,7 @@ int zImage_arm_load(int argc, char **argv, const char *buf, off_t len,
 		            dtb_offset, dtb_length);
 	}
 
-	add_segment(info, buf, len, kernel_base, len);
+	add_segment(info, buf, len, kernel_base, kernel_mem_size);
 
 	info->entry = (void*)kernel_base;
 
-- 
1.9.1


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH 2/2] arm: use zImage size from header

[Russell King]

We do not support zImage with an appended DTB as we need to supply our
own DTB to the kernel, and an appended DTB will override any that we
try to supply.  To ensure that we do not load an appended DTB, clamp
the kernel size to the size specified in the zImage header.

Signed-off-by: Russell King <rmk@armlinux.org.uk>
---
 kexec/arch/arm/kexec-zImage-arm.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/kexec/arch/arm/kexec-zImage-arm.c b/kexec/arch/arm/kexec-zImage-arm.c
index c8b90f96..7f02b931 100644
--- a/kexec/arch/arm/kexec-zImage-arm.c
+++ b/kexec/arch/arm/kexec-zImage-arm.c
@@ -28,6 +28,14 @@ off_t initrd_base = 0, initrd_size = 0;
 unsigned int kexec_arm_image_size = 0;
 unsigned long long user_page_offset = (-1ULL);
 
+struct zimage_header {
+	uint32_t instr[9];
+	uint32_t magic;
+#define ZIMAGE_MAGIC cpu_to_le32(0x016f2818)
+	uint32_t start;
+	uint32_t end;
+};
+
 struct android_image {
 	char magic[8];
 	uint32_t kernel_size;
@@ -435,6 +443,34 @@ int zImage_arm_load(int argc, char **argv, const char *buf, off_t len,
 	if (dtb_file)
 		dtb_buf = slurp_file(dtb_file, &dtb_length);
 
+	if (len > 0x34) {
+		const struct zimage_header *hdr;
+		off_t size;
+
+		hdr = (const struct zimage_header *)buf;
+
+		dbgprintf("zImage header: 0x%08x 0x%08x 0x%08x\n",
+			  hdr->magic, hdr->start, hdr->end);
+
+		if (hdr->magic == ZIMAGE_MAGIC) {
+			size = le32_to_cpu(hdr->end) - le32_to_cpu(hdr->start);
+
+			dbgprintf("zImage size 0x%llx, file size 0x%llx\n",
+				  (unsigned long long)size,
+				  (unsigned long long)len);
+
+			if (size > len) {
+				fprintf(stderr,
+					"zImage is truncated - file 0x%llx vs header 0x%llx\n",
+					(unsigned long long)len,
+					(unsigned long long)size);
+				return -1;
+			}
+			if (size < len)
+				len = size;
+		}
+	}
+
 	/* Handle android images, 2048 is the minimum page size */
 	if (len > 2048 && !strncmp(buf, "ANDROID!", 8)) {
 		const struct android_image *aimg = (const void *)buf;
-- 
1.9.1

[PATCH 2/2] arm: use zImage size from header

[Russell King]

We do not support zImage with an appended DTB as we need to supply our
own DTB to the kernel, and an appended DTB will override any that we
try to supply.  To ensure that we do not load an appended DTB, clamp
the kernel size to the size specified in the zImage header.

Signed-off-by: Russell King <rmk@armlinux.org.uk>
---
 kexec/arch/arm/kexec-zImage-arm.c | 36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

diff --git a/kexec/arch/arm/kexec-zImage-arm.c b/kexec/arch/arm/kexec-zImage-arm.c
index c8b90f96..7f02b931 100644
--- a/kexec/arch/arm/kexec-zImage-arm.c
+++ b/kexec/arch/arm/kexec-zImage-arm.c
@@ -28,6 +28,14 @@ off_t initrd_base = 0, initrd_size = 0;
 unsigned int kexec_arm_image_size = 0;
 unsigned long long user_page_offset = (-1ULL);
 
+struct zimage_header {
+	uint32_t instr[9];
+	uint32_t magic;
+#define ZIMAGE_MAGIC cpu_to_le32(0x016f2818)
+	uint32_t start;
+	uint32_t end;
+};
+
 struct android_image {
 	char magic[8];
 	uint32_t kernel_size;
@@ -435,6 +443,34 @@ int zImage_arm_load(int argc, char **argv, const char *buf, off_t len,
 	if (dtb_file)
 		dtb_buf = slurp_file(dtb_file, &dtb_length);
 
+	if (len > 0x34) {
+		const struct zimage_header *hdr;
+		off_t size;
+
+		hdr = (const struct zimage_header *)buf;
+
+		dbgprintf("zImage header: 0x%08x 0x%08x 0x%08x\n",
+			  hdr->magic, hdr->start, hdr->end);
+
+		if (hdr->magic == ZIMAGE_MAGIC) {
+			size = le32_to_cpu(hdr->end) - le32_to_cpu(hdr->start);
+
+			dbgprintf("zImage size 0x%llx, file size 0x%llx\n",
+				  (unsigned long long)size,
+				  (unsigned long long)len);
+
+			if (size > len) {
+				fprintf(stderr,
+					"zImage is truncated - file 0x%llx vs header 0x%llx\n",
+					(unsigned long long)len,
+					(unsigned long long)size);
+				return -1;
+			}
+			if (size < len)
+				len = size;
+		}
+	}
+
 	/* Handle android images, 2048 is the minimum page size */
 	if (len > 2048 && !strncmp(buf, "ANDROID!", 8)) {
 		const struct android_image *aimg = (const void *)buf;
-- 
1.9.1


_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

[PATCH 1/2] arm: plug a zImage corner case

[Simon Horman]

On Thu, Jul 07, 2016 at 03:01:19PM +0100, Russell King wrote:
> If a zImage is built with ARM_APPENDED_DTB enabled, the zImage will
> look at the word following the zImage and check whether it contains
> the DTB magic number.
> 
> Generally, kexec will clear the destination pages before copying the
> zImage, but there is a corner case where the zImage is a multiple of
> the page size, where the following page will not be touched.  Should
> the first word in this page contain the DTB magic number, the data
> following will be interpreted as a DTB image.
> 
> In order to make this bullet-proof, we must always initialise the word
> following the zImage.  Arrange this by specifying the zImage memory
> size one word bigger than the buffer size.
> 
> Signed-off-by: Russell King <rmk@armlinux.org.uk>

Thanks, I have applied this and the next patch.

Re: [PATCH 1/2] arm: plug a zImage corner case

[Simon Horman]

On Thu, Jul 07, 2016 at 03:01:19PM +0100, Russell King wrote:
> If a zImage is built with ARM_APPENDED_DTB enabled, the zImage will
> look at the word following the zImage and check whether it contains
> the DTB magic number.
> 
> Generally, kexec will clear the destination pages before copying the
> zImage, but there is a corner case where the zImage is a multiple of
> the page size, where the following page will not be touched.  Should
> the first word in this page contain the DTB magic number, the data
> following will be interpreted as a DTB image.
> 
> In order to make this bullet-proof, we must always initialise the word
> following the zImage.  Arrange this by specifying the zImage memory
> size one word bigger than the buffer size.
> 
> Signed-off-by: Russell King <rmk@armlinux.org.uk>

Thanks, I have applied this and the next patch.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

kexec failures with DEBUG_RODATA

[Tony Lindgren]

* Russell King - ARM Linux <linux@armlinux.org.uk> [160707 03:25]:
> On Wed, Jun 15, 2016 at 11:42:28PM +0100, Russell King - ARM Linux wrote:
> > The boot environment must be setup such that there is room for the
> > uncompressed image (aligned currently to 256 bytes) followed by the
> > size of the compressed image, with any appended DTBs included.
> > Anything which is located below that is likely to get trampled by
> > the decompressor.
> 
> There's a question that's been lingering, which hasn't been brought up
> by anyone yet: should we even support zImage files with an appended
> DTB for kexec?
> 
> When kexec is operating in DTB mode, it will provide the DTB to the
> kernel from the host environment, possibly with some modifications (eg,
> to change the command line arguments.)  Since an appended DTB would
> override the kexec-provided DTB, this would wipe out those changes,
> some of which are necessary for things like crashdump to work.
> 
> So, I'm thinking that kexec should not support a zImage with appended
> DTB - in fact, I think it should truncate the zImage to be loaded, and
> ensure that the word following the zImage does not contain a DTB magic
> number, in case the zImage has been built with CONFIG_ARM_APPENDED_DTB
> enabled.
> 
> Thoughts?

Sorry for a late reply, I've been offline.

Sounds like a good idea to me as this as we can properly error out
with appendd dtb before anything nasty happens making new kernel
fail. And if somebody really needs it, we can add an option for
--force-appended-dtb.

Regards,

Tony

Re: kexec failures with DEBUG_RODATA

[Tony Lindgren]

* Russell King - ARM Linux <linux@armlinux.org.uk> [160707 03:25]:
> On Wed, Jun 15, 2016 at 11:42:28PM +0100, Russell King - ARM Linux wrote:
> > The boot environment must be setup such that there is room for the
> > uncompressed image (aligned currently to 256 bytes) followed by the
> > size of the compressed image, with any appended DTBs included.
> > Anything which is located below that is likely to get trampled by
> > the decompressor.
> 
> There's a question that's been lingering, which hasn't been brought up
> by anyone yet: should we even support zImage files with an appended
> DTB for kexec?
> 
> When kexec is operating in DTB mode, it will provide the DTB to the
> kernel from the host environment, possibly with some modifications (eg,
> to change the command line arguments.)  Since an appended DTB would
> override the kexec-provided DTB, this would wipe out those changes,
> some of which are necessary for things like crashdump to work.
> 
> So, I'm thinking that kexec should not support a zImage with appended
> DTB - in fact, I think it should truncate the zImage to be loaded, and
> ensure that the word following the zImage does not contain a DTB magic
> number, in case the zImage has been built with CONFIG_ARM_APPENDED_DTB
> enabled.
> 
> Thoughts?

Sorry for a late reply, I've been offline.

Sounds like a good idea to me as this as we can properly error out
with appendd dtb before anything nasty happens making new kernel
fail. And if somebody really needs it, we can add an option for
--force-appended-dtb.

Regards,

Tony

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

libdrm-armada repository

[Joshua Clayton]

Greetings Russell,
I'm publishing an etnaviv yocto layer on github.
One of the components is libdrm-armada, which we get from
git://ftp.arm.linux.org.uk/~rmk/libdrm-armada.git

I don't want to put an unwanted extra burden on your server.

I notice there is also
http://git.arm.linux.org.uk/cgit/libdrm-armada.git

or we can mirror it on github.

Do you have a preference?

Thanks
Joshua Clayton
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: libdrm-armada repository

[Russell King]

On Tue, Aug 02, 2016 at 04:09:58PM -0700, Joshua Clayton wrote:
> Greetings Russell,
> I'm publishing an etnaviv yocto layer on github.
> One of the components is libdrm-armada, which we get from
> git://ftp.arm.linux.org.uk/~rmk/libdrm-armada.git
> 
> I don't want to put an unwanted extra burden on your server.
> 
> I notice there is also
> http://git.arm.linux.org.uk/cgit/libdrm-armada.git

git.arm and ftp.arm are both the same machine, which has recently been
upgraded to something a tad more modern than the previous 13+ year old
machine, so it should now be super-fast.  In addition, I've changed
the DNS slightly.

The preferred URLs are now:

 git://git.armlinux.org.uk/~rmk/libdrm-armada.git

for pulling and cloning from, and the cgit interface

 http://git.armlinux.org.uk/cgit/libdrm-armada.git

Thanks.

-- 
Russell King
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: libdrm-armada repository

[Fabio Estevam]

Hi Joshua,

On Tue, Aug 2, 2016 at 8:09 PM, Joshua Clayton <stillcompiling@gmail.com> wrote:
> Greetings Russell,
> I'm publishing an etnaviv yocto layer on github.

Cool! Could you please let us know when this layer becomes available?

Thanks
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: libdrm-armada repository

[Joshua Clayton]

On 08/02/2016 04:28 PM, Russell King wrote:
> On Tue, Aug 02, 2016 at 04:09:58PM -0700, Joshua Clayton wrote:
>> Greetings Russell,
>> I'm publishing an etnaviv yocto layer on github.
>> One of the components is libdrm-armada, which we get from
>> git://ftp.arm.linux.org.uk/~rmk/libdrm-armada.git
>>
>> I don't want to put an unwanted extra burden on your server.
>>
>> I notice there is also
>> http://git.arm.linux.org.uk/cgit/libdrm-armada.git
> git.arm and ftp.arm are both the same machine, which has recently been
> upgraded to something a tad more modern than the previous 13+ year old
> machine, so it should now be super-fast.  In addition, I've changed
> the DNS slightly.
>
> The preferred URLs are now:
>
>  git://git.armlinux.org.uk/~rmk/libdrm-armada.git
>
> for pulling and cloning from, and the cgit interface
>
>  http://git.armlinux.org.uk/cgit/libdrm-armada.git
>
> Thanks.
>
Thanks for the info.

I've update the url to  git://git.armlinux.org.uk/~rmk/libdrm-armada.git

as it is a download, rather than browser interface.

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: libdrm-armada repository

[Joshua Clayton]

On 08/02/2016 06:38 PM, Fabio Estevam wrote:
> Hi Joshua,
>
> On Tue, Aug 2, 2016 at 8:09 PM, Joshua Clayton <stillcompiling@gmail.com> wrote:
>> Greetings Russell,
>> I'm publishing an etnaviv yocto layer on github.
> Cool! Could you please let us know when this layer becomes available?
>
> Thanks
Sure, Fabio.
It is available now.

https://github.com/d4ddi0/meta-etnaviv

Thus far tested only on imx6q-evi
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel