Re: PLEASE REVERT URGENTLY: Re: [PATCH v5 2/3] x86/boot: add acpi rsdp address to setup_header

[Juergen Gross <jgross@suse.com>]

On 10/11/2018 08:16, H. Peter Anvin wrote:
> On 11/9/18 11:02 PM, Juergen Gross wrote:
>>>
>>> Yes. We know that and it is resolved by:
>>>
>>> a) the length field in setup_header;
>>> b) the "sentinel" field which catches legacy non-compliant bootloaders.
>>
>> Doesn't help for boot loaders reading struct setup_header from the
>> kernel image and then writing e.g. 512 bytes back to the setup_header
>> location. The sentinel is cleared and the length field just isn't
>> taken into account. And this is what happened.
>>
> 
> This is insane?! How do they manage to do this... it's not like  this isn't
> written out in plain English to follow. I am, once again, utterly and
> genuinely baffled about how many ways Grub can do things wrong.
> 
> So we should probably add a terminal sentinel field at offset 0x281, which is
> one byte past the longest possible setup_header structure; in fact, we may
> just want to explicitly pad setup_header with zeroes to its final size, if
> nothing else to make it explicit how little space is actually left in there.

How would that help? The garabge data written could have the correct
terminal sentinel value by chance.

That's why I re-used an existing field in setup_header (the version) to
let grub tell the kernel which part of setup_header was written by grub.

That's the only way I could find to let the kernel distinguish between
garbage and actual data.
> It would be enormously helpful if you could find out any more details about
> exactly what they are doing to break things.

That's easy:

The memory layout is:

0x1f1 bytes of data, including the sentinel, the setup_header, and then
more data.

grub did read the kernel's setup_header in the correct size into its
buffer (which contains random garbage before that), intializes the first
0x1f1 including the sentinel byte, and then writes back the buffer, but
using a too large length for that.


Juergen