Fixes for low memory allocation machinery in early boot code

Fixes for low memory allocation machinery in early boot code

[Daniel Kiper]

Hey,

So, as I promised in other thread I am sending more info about my investigation
related to low memory allocation for trampoline and other early boot data.

Starting from the beginning it looks that there are "soft" limits enforced
in BIOS early boot code looking for usable low memory region. Hight limit
is set at 640 KiB and low at 256 KiB. This means that if a value from a given
source which describes low memory region (i.e. EBDA base segment, base memory
size, multiboot protocol) is out of bounds then we try to get new value from
next one (I mean source). However, at the end there are no checks that assure
us that we got what we expected. So, I think that at first we should add "hard"
checks here. This means that if we get value out of earlier mentioned bounds
then we should print relevant message on serial console and halt the system.

Additionally, my investigation has shown that there are no bound checks in
low memory allocation machinery for trampoline (by the way, in BIOS path we
allocate 64 KiB for trampoline but in EFI code we properly calculate its size;
so, I think we should do the same calculation in BIOS path), stack and boot data
taken from multiboot protocol. Hence, relevant fixes should be added here too.

Moreover I think that at least allocation machinery with additional checks
described in last paragraph can be used on EFI platforms when Xen is booted
via multiboot2 protocol. However, then high limit should be defined as 1 MiB.
Though I think that low limit, 256 KiB, should stay as is.

So, I think that we should prepare following patches:
  - allocate properly calculated amount of memory for trampoline,
  - define high/low limit as a constants and use them,
  - add bounds checks for chosen low memory region, and bounds
    checks in allocation machinery for trampoline and stack,
  - add bounds checks to allocator in reloc.c.

I have a feeling that this fixes are not very critical, however, nice to have.
So, looking at code before and after my "x86: multiboot2 protocol support" patch
series I think that we can apply them on top of it. Then relevant code changes
will be much easier to analyze and cleaner, especially in reloc.c. However, if
ease of backporting of low memory allocator patches is more important then they
should precede "x86: multiboot2 protocol support" patch series.

What do you think about that?

Daniel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: Fixes for low memory allocation machinery in early boot code

[Jan Beulich]

>>> On 14.09.16 at 10:23, <daniel.kiper@oracle.com> wrote:
> Starting from the beginning it looks that there are "soft" limits enforced
> in BIOS early boot code looking for usable low memory region. Hight limit
> is set at 640 KiB and low at 256 KiB. This means that if a value from a given
> source which describes low memory region (i.e. EBDA base segment, base memory
> size, multiboot protocol) is out of bounds then we try to get new value from
> next one (I mean source). However, at the end there are no checks that assure
> us that we got what we expected. So, I think that at first we should add "hard"
> checks here. This means that if we get value out of earlier mentioned bounds
> then we should print relevant message on serial console and halt the system.

I disagree. I think that the best effort approach (what you name "soft"
checks) are quite okay in the legacy BIOS case: Even if the BIOS has
screwed things up, we can still _try_ to come up nevertheless.

This is quite different for (imo) much more sophisticated EFI
environments: We know they are screwed too, but we can't as
easily ignore them using certain pieces of memory.

> Additionally, my investigation has shown that there are no bound checks in
> low memory allocation machinery for trampoline (by the way, in BIOS path we
> allocate 64 KiB for trampoline but in EFI code we properly calculate its size;
> so, I think we should do the same calculation in BIOS path), stack and boot data
> taken from multiboot protocol. Hence, relevant fixes should be added here too.

Would be nice, yes, but we need to weigh the need to presumably do
at least some of this in assembly code (for now at least) against the
potential gain.

> Moreover I think that at least allocation machinery with additional checks
> described in last paragraph can be used on EFI platforms when Xen is booted
> via multiboot2 protocol. However, then high limit should be defined as 1 MiB.
> Though I think that low limit, 256 KiB, should stay as is.

Why 1Mb? The 640k limit derives from hardware aspects, but doesn't
depend on the software environment.

> So, I think that we should prepare following patches:
>   - allocate properly calculated amount of memory for trampoline,
>   - define high/low limit as a constants and use them,
>   - add bounds checks for chosen low memory region, and bounds
>     checks in allocation machinery for trampoline and stack,
>   - add bounds checks to allocator in reloc.c.
> 
> I have a feeling that this fixes are not very critical, however, nice to have.

Indeed. I'd just like to avoid that new code (read: your mb2 series)
gets introduced with similar issues. Just like the original EFI code at
least tries to properly allocate the trampoline space.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: Fixes for low memory allocation machinery in early boot code

[Daniel Kiper]

On Fri, Sep 16, 2016 at 06:15:10AM -0600, Jan Beulich wrote:
> >>> On 14.09.16 at 10:23, <daniel.kiper@oracle.com> wrote:
> > Starting from the beginning it looks that there are "soft" limits enforced
> > in BIOS early boot code looking for usable low memory region. Hight limit
> > is set at 640 KiB and low at 256 KiB. This means that if a value from a given
> > source which describes low memory region (i.e. EBDA base segment, base memory
> > size, multiboot protocol) is out of bounds then we try to get new value from
> > next one (I mean source). However, at the end there are no checks that assure
> > us that we got what we expected. So, I think that at first we should add "hard"
> > checks here. This means that if we get value out of earlier mentioned bounds
> > then we should print relevant message on serial console and halt the system.
>
> I disagree. I think that the best effort approach (what you name "soft"
> checks) are quite okay in the legacy BIOS case: Even if the BIOS has
> screwed things up, we can still _try_ to come up nevertheless.
>
> This is quite different for (imo) much more sophisticated EFI
> environments: We know they are screwed too, but we can't as
> easily ignore them using certain pieces of memory.
>
> > Additionally, my investigation has shown that there are no bound checks in
> > low memory allocation machinery for trampoline (by the way, in BIOS path we
> > allocate 64 KiB for trampoline but in EFI code we properly calculate its size;
> > so, I think we should do the same calculation in BIOS path), stack and boot data
> > taken from multiboot protocol. Hence, relevant fixes should be added here too.
>
> Would be nice, yes, but we need to weigh the need to presumably do
> at least some of this in assembly code (for now at least) against the
> potential gain.
>
> > Moreover I think that at least allocation machinery with additional checks
> > described in last paragraph can be used on EFI platforms when Xen is booted
> > via multiboot2 protocol. However, then high limit should be defined as 1 MiB.
> > Though I think that low limit, 256 KiB, should stay as is.
>
> Why 1Mb? The 640k limit derives from hardware aspects, but doesn't
> depend on the software environment.

I do not expect that anything which is not memory will reside between 640 KiB
and 1 MiB on EFI platforms. So, if firmware says that it is usable why we cannot
use it? And I have a feeling that this idea lead to currently existing checks
around trampoline allocation code for EFI. Though, as I saw EFI platforms
usually does not expose region 640 KiB and 1 MiB as usable. However, this
does not mean that they cannot in the future.

> > So, I think that we should prepare following patches:
> >   - allocate properly calculated amount of memory for trampoline,
> >   - define high/low limit as a constants and use them,
> >   - add bounds checks for chosen low memory region, and bounds
> >     checks in allocation machinery for trampoline and stack,
> >   - add bounds checks to allocator in reloc.c.
> >
> > I have a feeling that this fixes are not very critical, however, nice to have.
>
> Indeed. I'd just like to avoid that new code (read: your mb2 series)
> gets introduced with similar issues. Just like the original EFI code at
> least tries to properly allocate the trampoline space.

OK, I have a feeling that you wish to postpone most of proposed changes for later.
I can understand that. However, if we wish check that there is sufficient amount
of memory available for trampoline we should decide which value to use. Should
we use 64 KiB from BIOS Xen assembly code or trampoline real size like in Xen
EFI code? Trampoline real size is much more sensible for me but this requires
some changes in assembly code allocating trampoline. We can allocate trampoline
real size on both EFI and BIOS platforms. Or leave BIOS case as is and use
trampoline real size just only on EFI platforms.

Daniel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: Fixes for low memory allocation machinery in early boot code

[Jan Beulich]

>>> On 20.09.16 at 14:11, <daniel.kiper@oracle.com> wrote:
> On Fri, Sep 16, 2016 at 06:15:10AM -0600, Jan Beulich wrote:
>> >>> On 14.09.16 at 10:23, <daniel.kiper@oracle.com> wrote:
>> > Additionally, my investigation has shown that there are no bound checks in
>> > low memory allocation machinery for trampoline (by the way, in BIOS path we
>> > allocate 64 KiB for trampoline but in EFI code we properly calculate its size;
>> > so, I think we should do the same calculation in BIOS path), stack and boot data
>> > taken from multiboot protocol. Hence, relevant fixes should be added here too.
>>
>> Would be nice, yes, but we need to weigh the need to presumably do
>> at least some of this in assembly code (for now at least) against the
>> potential gain.
>>
>> > Moreover I think that at least allocation machinery with additional checks
>> > described in last paragraph can be used on EFI platforms when Xen is booted
>> > via multiboot2 protocol. However, then high limit should be defined as 1 MiB.
>> > Though I think that low limit, 256 KiB, should stay as is.
>>
>> Why 1Mb? The 640k limit derives from hardware aspects, but doesn't
>> depend on the software environment.
> 
> I do not expect that anything which is not memory will reside between 640 KiB
> and 1 MiB on EFI platforms. So, if firmware says that it is usable why we cannot
> use it? And I have a feeling that this idea lead to currently existing checks
> around trampoline allocation code for EFI. Though, as I saw EFI platforms
> usually does not expose region 640 KiB and 1 MiB as usable. However, this
> does not mean that they cannot in the future.

Hmm, when the region (or part of it) is reported as available, then I
guess we could use it. But as to there being RAM - I doubt it, since
for the next little while, EFI or not, we're talking about PC compatible
systems, which don't normally have RAM in that range.

>> > So, I think that we should prepare following patches:
>> >   - allocate properly calculated amount of memory for trampoline,
>> >   - define high/low limit as a constants and use them,
>> >   - add bounds checks for chosen low memory region, and bounds
>> >     checks in allocation machinery for trampoline and stack,
>> >   - add bounds checks to allocator in reloc.c.
>> >
>> > I have a feeling that this fixes are not very critical, however, nice to have.
>>
>> Indeed. I'd just like to avoid that new code (read: your mb2 series)
>> gets introduced with similar issues. Just like the original EFI code at
>> least tries to properly allocate the trampoline space.
> 
> OK, I have a feeling that you wish to postpone most of proposed changes for later.
> I can understand that. However, if we wish check that there is sufficient amount
> of memory available for trampoline we should decide which value to use. Should
> we use 64 KiB from BIOS Xen assembly code or trampoline real size like in Xen
> EFI code? Trampoline real size is much more sensible for me but this requires
> some changes in assembly code allocating trampoline.

Why? Current EFI code uses the actual size too. Hence I think you
could do so as well in your new additions, leaving the legacy code
alone until you or someone else means to overhaul it.

> We can allocate trampoline
> real size on both EFI and BIOS platforms. Or leave BIOS case as is and use
> trampoline real size just only on EFI platforms.

Exactly.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: Fixes for low memory allocation machinery in early boot code

[Daniel Kiper]

On Tue, Sep 20, 2016 at 07:23:06AM -0600, Jan Beulich wrote:
> >>> On 20.09.16 at 14:11, <daniel.kiper@oracle.com> wrote:
> > On Fri, Sep 16, 2016 at 06:15:10AM -0600, Jan Beulich wrote:
> >> >>> On 14.09.16 at 10:23, <daniel.kiper@oracle.com> wrote:
> >> > Additionally, my investigation has shown that there are no bound checks in
> >> > low memory allocation machinery for trampoline (by the way, in BIOS path we
> >> > allocate 64 KiB for trampoline but in EFI code we properly calculate its size;
> >> > so, I think we should do the same calculation in BIOS path), stack and boot data
> >> > taken from multiboot protocol. Hence, relevant fixes should be added here too.
> >>
> >> Would be nice, yes, but we need to weigh the need to presumably do
> >> at least some of this in assembly code (for now at least) against the
> >> potential gain.
> >>
> >> > Moreover I think that at least allocation machinery with additional checks
> >> > described in last paragraph can be used on EFI platforms when Xen is booted
> >> > via multiboot2 protocol. However, then high limit should be defined as 1 MiB.
> >> > Though I think that low limit, 256 KiB, should stay as is.
> >>
> >> Why 1Mb? The 640k limit derives from hardware aspects, but doesn't
> >> depend on the software environment.
> >
> > I do not expect that anything which is not memory will reside between 640 KiB
> > and 1 MiB on EFI platforms. So, if firmware says that it is usable why we cannot
> > use it? And I have a feeling that this idea lead to currently existing checks
> > around trampoline allocation code for EFI. Though, as I saw EFI platforms
> > usually does not expose region 640 KiB and 1 MiB as usable. However, this
> > does not mean that they cannot in the future.
>
> Hmm, when the region (or part of it) is reported as available, then I
> guess we could use it. But as to there being RAM - I doubt it, since
> for the next little while, EFI or not, we're talking about PC compatible
> systems, which don't normally have RAM in that range.

If we drop BIOS and move to EFI then compatibility with PC drops to tiny
fraction of original platform. So, in this context lack of VGA or anything
like that above 640 KiB is not an issue IMO and memory there would not be
very big surprise for me. However, if you care I would ask why do you use
1 MiB limit instead of 640 KiB in xen/arch/x86/efi/efi-boot.h? I do not
say this is huge mistake but I am curious why not 640 KiB? I suppose that
there was a reason for it but I cannot find anything about that in comments
or commit messages.

Daniel

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel

Re: Fixes for low memory allocation machinery in early boot code

[Jan Beulich]

>>> On 20.09.16 at 20:26, <daniel.kiper@oracle.com> wrote:
> However, if you care I would ask why do you use
> 1 MiB limit instead of 640 KiB in xen/arch/x86/efi/efi-boot.h? I do not
> say this is huge mistake but I am curious why not 640 KiB? I suppose that
> there was a reason for it but I cannot find anything about that in comments
> or commit messages.

You mean in efi_arch_process_memory_map()? Because here's we're
processing a memory map, i.e. we have something in hand which
already abstracts things. As a result, all we care about is finding a
region that fits our needs, without having to care about avoiding
certain areas.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
https://lists.xen.org/xen-devel