Re: [PATCH 1/8] x86/boot: Drop incorrect mapping at l2_xenmap[0]

[Andrew Cooper <amc96@srcf.net>]

On 30/11/2021 11:22, Jan Beulich wrote:
> On 30.11.2021 12:14, Andrew Cooper wrote:
>> On 30/11/2021 10:33, Jan Beulich wrote:
>>> On 30.11.2021 11:04, Andrew Cooper wrote:
>>>> It has been 4 years since the default load address changed from 1M to 2M, and
>>>> _stext ceased residing in l2_xenmap[0].  We should not be inserting an unused
>>>> mapping.
>>>>
>>>> To ensure we don't create mappings accidentally, loop from 0 and obey
>>>> _PAGE_PRESENT on all entries.
>>>>
>>>> Fixes: 7ed93f3a0dff ("x86: change default load address from 1 MiB to 2 MiB")
>>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>>
>>> I guess this may be worth backporting despite not having any immediate
>>> adverse effect.
>> I'd say so, yes.  I too can't see an adverse effect right now, but I'm
>> definitely wary of stray executable mappings lying around.
>>
>>
>> In principle, it would be nice to reclaim the 2M of space (which only
>> exists for the MB1 path IIRC), but then we're getting into a position
>> where xen_phys_start isn't really that any more.
> Well, xen_phys_base might be slightly more accurate, but apart from that
> I do think that we reclaim that space (as much as we did reclaim the 1Mb
> before the change of the default load address):
>
>     if ( efi_boot_mem_unused(&eb_start, &eb_end) )
>     {
>         reserve_e820_ram(&boot_e820, __pa(_stext), __pa(eb_start));
>         reserve_e820_ram(&boot_e820, __pa(eb_end), __pa(__2M_rwdata_end));
>     }
>     else
>         reserve_e820_ram(&boot_e820, __pa(_stext), __pa(__2M_rwdata_end));

That means there are zero safety barriers between a bad function pointer
and executing arbitrary guest memory, doesn't it...

My "adverse effect" comment was under the impression that we just left
the range unused.

~Andrew