[RFC] x86/boot: Don't use BDA value if it's suspiciously small

[RFC] x86/boot: Don't use BDA value if it's suspiciously small

[Sylvain Munaut]

If we have an multiboot value and the value we got from the BDA
seems too small, use the safe one

Signed-off-by: Sylvain Munaut <s.munaut@whatever-company.com>
---
I need this when using linux-as-a-bootloader (i.e. kexec into Xen) because
the BDA is just zero at that point (not entirely sure why tbh).

This is the simplest patch I could come up with and that shouldn't change
anything for system currently booting. But if the multiboot infos are
present, I'm not sure why not use that preferentially since the values
from the BDA / EBDA could just be random garbage while the multiboot header
has at least some minimal validation (checksum + magic).

An error message if no sane value ( i.e. > 64k at min ) can be found at
all could be printed too. Took me some time to trace this down and some
serial output would have been welcome :)

Comments welcome wrt to what people think is best and I can re-spin this.

 xen/arch/x86/boot/head.S | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
index 85770e8..d79fcc5 100644
--- a/xen/arch/x86/boot/head.S
+++ b/xen/arch/x86/boot/head.S
@@ -108,6 +108,8 @@ __start:
         shl     $10-4,%edx
         cmp     %eax,%edx           /* compare with BDA value */
         cmovb   %edx,%eax           /* and use the smaller */
+        cmp     $0x1000,%eax        /* or if the BDA value is too small */
+        cmovb   %edx,%eax           /* (and probably not valid) */
 
 2:      /* Reserve 64kb for the trampoline */
         sub     $0x1000,%eax
-- 
2.1.4


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

Re: [RFC] x86/boot: Don't use BDA value if it's suspiciously small

[Andrew Cooper]

On 26/08/16 10:09, Sylvain Munaut wrote:
> If we have an multiboot value and the value we got from the BDA
> seems too small, use the safe one
>
> Signed-off-by: Sylvain Munaut <s.munaut@whatever-company.com>
> ---
> I need this when using linux-as-a-bootloader (i.e. kexec into Xen) because
> the BDA is just zero at that point (not entirely sure why tbh).

Does Linux reclam and reuse the BDA by this point in its boot?

>
> This is the simplest patch I could come up with and that shouldn't change
> anything for system currently booting. But if the multiboot infos are
> present, I'm not sure why not use that preferentially since the values
> from the BDA / EBDA could just be random garbage while the multiboot header
> has at least some minimal validation (checksum + magic).

If we have mutliboot memory information available (should be all of the 
time), we should use that in preference to peeking at the BDA/EBDA

>
> An error message if no sane value ( i.e. > 64k at min ) can be found at
> all could be printed too. Took me some time to trace this down and some
> serial output would have been welcome :)

All of this code is rather hairy.  Unfortunately, there is no good 
logging method available at this point.

~Andrew

>
> Comments welcome wrt to what people think is best and I can re-spin this.
>
>   xen/arch/x86/boot/head.S | 2 ++
>   1 file changed, 2 insertions(+)
>
> diff --git a/xen/arch/x86/boot/head.S b/xen/arch/x86/boot/head.S
> index 85770e8..d79fcc5 100644
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -108,6 +108,8 @@ __start:
>           shl     $10-4,%edx
>           cmp     %eax,%edx           /* compare with BDA value */
>           cmovb   %edx,%eax           /* and use the smaller */
> +        cmp     $0x1000,%eax        /* or if the BDA value is too small */
> +        cmovb   %edx,%eax           /* (and probably not valid) */
>   
>   2:      /* Reserve 64kb for the trampoline */
>           sub     $0x1000,%eax


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

Re: [RFC] x86/boot: Don't use BDA value if it's suspiciously small

[Jan Beulich]

>>> On 26.08.16 at 11:09, <s.munaut@whatever-company.com> wrote:
> --- a/xen/arch/x86/boot/head.S
> +++ b/xen/arch/x86/boot/head.S
> @@ -108,6 +108,8 @@ __start:
>          shl     $10-4,%edx
>          cmp     %eax,%edx           /* compare with BDA value */
>          cmovb   %edx,%eax           /* and use the smaller */
> +        cmp     $0x1000,%eax        /* or if the BDA value is too small */
> +        cmovb   %edx,%eax           /* (and probably not valid) */

Considering there is a bounds check of the EBDA values a few
lines up from here (against 0x4000) I don't think I see how this
code can help, assuming the given explanation is applicable.

In any event is bounding by 0x1000 likely not enough, as placing
the trampoline at address zero is unlikely to be a good idea.

Jan


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

Re: [RFC] x86/boot: Don't use BDA value if it's suspiciously small

[Sylvain Munaut]

Hi,

On Fri, Aug 26, 2016 at 3:06 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 26.08.16 at 11:09, <s.munaut@whatever-company.com> wrote:
>> --- a/xen/arch/x86/boot/head.S
>> +++ b/xen/arch/x86/boot/head.S
>> @@ -108,6 +108,8 @@ __start:
>>          shl     $10-4,%edx
>>          cmp     %eax,%edx           /* compare with BDA value */
>>          cmovb   %edx,%eax           /* and use the smaller */
>> +        cmp     $0x1000,%eax        /* or if the BDA value is too small */
>> +        cmovb   %edx,%eax           /* (and probably not valid) */
>
> Considering there is a bounds check of the EBDA values a few
> lines up from here (against 0x4000) I don't think I see how this
> code can help, assuming the given explanation is applicable.

Because in my case, the EBDA value is rejected, it then falls back to
reading the "base memory size" from the BDA, but that also reads as
0x00.
Then because 0 is smaller than the value from multiboot, it actually
takes that. Then decrements it by 0x1000 (which doesn't go well) and
try to place the trampoline there ...


> In any event is bounding by 0x1000 likely not enough, as placing
> the trampoline at address zero is unlikely to be a good idea.

I just took that value since that was the lower bound used for the
multiboot value.

What would be reasonable as a lower bound for validity check ?


Cheers,

   Sylvain

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

Re: [RFC] x86/boot: Don't use BDA value if it's suspiciously small

[Jan Beulich]

>>> On 26.08.16 at 15:10, <s.munaut@whatever-company.com> wrote:
> Hi,
> 
> On Fri, Aug 26, 2016 at 3:06 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 26.08.16 at 11:09, <s.munaut@whatever-company.com> wrote:
>>> --- a/xen/arch/x86/boot/head.S
>>> +++ b/xen/arch/x86/boot/head.S
>>> @@ -108,6 +108,8 @@ __start:
>>>          shl     $10-4,%edx
>>>          cmp     %eax,%edx           /* compare with BDA value */
>>>          cmovb   %edx,%eax           /* and use the smaller */
>>> +        cmp     $0x1000,%eax        /* or if the BDA value is too small */
>>> +        cmovb   %edx,%eax           /* (and probably not valid) */
>>
>> Considering there is a bounds check of the EBDA values a few
>> lines up from here (against 0x4000) I don't think I see how this
>> code can help, assuming the given explanation is applicable.
> 
> Because in my case, the EBDA value is rejected, it then falls back to
> reading the "base memory size" from the BDA, but that also reads as
> 0x00.

Ah, but that is a basic boot environment violation. Any sane OS
should be permitted to refuse to boot without any low memory.

> Then because 0 is smaller than the value from multiboot, it actually
> takes that. Then decrements it by 0x1000 (which doesn't go well) and
> try to place the trampoline there ...

And it would certainly make sense to try to handle this underflow
case.

>> In any event is bounding by 0x1000 likely not enough, as placing
>> the trampoline at address zero is unlikely to be a good idea.
> 
> I just took that value since that was the lower bound used for the
> multiboot value.
> 
> What would be reasonable as a lower bound for validity check ?

At the very least we shouldn't overlap with the BDA (starting at
0040:0000 and iirc covering up to 256 bytes, which is why DOS
never used any memory below 0050:0000).

Jan


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

Re: [RFC] x86/boot: Don't use BDA value if it's suspiciously small

[Sylvain Munaut]

Hi,


> At the very least we shouldn't overlap with the BDA (starting at
> 0040:0000 and iirc covering up to 256 bytes, which is why DOS
> never used any memory below 0050:0000).

Mmm, I misread the assembly the low limit applied to the multi boot
value was 0x4000 and not 0x1000 ...

Would this logic be acceptable :

high_limit = 0xa0000
low_limit  = 0x40000

if (MBI_MEMLIMITS available and >= low_limit)
 -> use that - 64k

if (low_limit <= EBDA location < high_limit)
 -> use that - 64k

if (BDA memory size >= low_limit)
 -> use that - 64k

if all failed:
 print_err("No low memory available")

(in pseudo code, didn't really want to code it in assembly if it's not
acceptable :p)


If that looks ok, I can write and test a patch implementing that.

Cheers,

    Sylvain Munaut

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

Re: [RFC] x86/boot: Don't use BDA value if it's suspiciously small

[Jan Beulich]

>>> On 26.08.16 at 16:21, <s.munaut@whatever-company.com> wrote:
> Hi,
> 
> 
>> At the very least we shouldn't overlap with the BDA (starting at
>> 0040:0000 and iirc covering up to 256 bytes, which is why DOS
>> never used any memory below 0050:0000).
> 
> Mmm, I misread the assembly the low limit applied to the multi boot
> value was 0x4000 and not 0x1000 ...
> 
> Would this logic be acceptable :
> 
> high_limit = 0xa0000
> low_limit  = 0x40000
> 
> if (MBI_MEMLIMITS available and >= low_limit)
>  -> use that - 64k
> 
> if (low_limit <= EBDA location < high_limit)
>  -> use that - 64k
> 
> if (BDA memory size >= low_limit)
>  -> use that - 64k
> 
> if all failed:
>  print_err("No low memory available")
> 
> (in pseudo code, didn't really want to code it in assembly if it's not
> acceptable :p)
> 
> 
> If that looks ok, I can write and test a patch implementing that.

I'm not sure. I'd like to see the current logic altered as little as
possible, and what you suggest above is more than that minimum.
Ideally you'd just cap things at the lower end, if they turn out too
small. But then again - all bets are off anyway when all three
values are unreasonably low. So another question: Can you
detect whether we get booted from Linux (i.e. do they provide
any kind of identification anywhere)?

Jan


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

Re: [RFC] x86/boot: Don't use BDA value if it's suspiciously small

[Sylvain Munaut]

> I'm not sure. I'd like to see the current logic altered as little as
> possible, and what you suggest above is more than that minimum.

Then, that would be more like the very first patch I posted but just
change the 0x1000 low limit to 0x4000.


> So another question: Can you
> detect whether we get booted from Linux (i.e. do they provide
> any kind of identification anywhere)?

The multiboot info struct has the boot loader name optional field and
kexec does fill it with "kexec x.y.z".


Cheers,

    Sylvain

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

Re: [RFC] x86/boot: Don't use BDA value if it's suspiciously small

[Jan Beulich]

>>> On 26.08.16 at 16:53, <s.munaut@whatever-company.com> wrote:
>>  I'm not sure. I'd like to see the current logic altered as little as
>> possible, and what you suggest above is more than that minimum.
> 
> Then, that would be more like the very first patch I posted but just
> change the 0x1000 low limit to 0x4000.

I thought I had made clear that I think this then ought to be 0x1050.

>> So another question: Can you
>> detect whether we get booted from Linux (i.e. do they provide
>> any kind of identification anywhere)?
> 
> The multiboot info struct has the boot loader name optional field and
> kexec does fill it with "kexec x.y.z".

Perhaps better to simply ignore the two BDA values in that special
case then? Andrew, what do you think?

Jan


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

Re: [RFC] x86/boot: Don't use BDA value if it's suspiciously small

[Sylvain Munaut]

On Fri, Aug 26, 2016 at 5:10 PM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 26.08.16 at 16:53, <s.munaut@whatever-company.com> wrote:
>>>  I'm not sure. I'd like to see the current logic altered as little as
>>> possible, and what you suggest above is more than that minimum.
>>
>> Then, that would be more like the very first patch I posted but just
>> change the 0x1000 low limit to 0x4000.
>
> I thought I had made clear that I think this then ought to be 0x1050.

The current code validates EBDA location and the value from the MBI
against 0x4000 as the lower limit (I was wrong previously when saying
it was 0x1000), so I thought I wouldn't introduce yet another magic
constant limit and just use the same value.
But if you'd rather go with 0x1050, I'll use that.


Cheers,

    Sylvain

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