Re: [PATCH] Fix FreeDOS command booting large files (near or above 64 KiB)

[Daniel Kiper]

Adding Vladimir...

On Fri, Dec 20, 2019 at 04:04:21PM +0100, C. Masloch wrote:
> While testing the 86-DOS lDebug [1] booting from GRUB2, newer versions of the
> debugger would fail to load when booted using GRUB's freedos command. The
> behaviour observed in a qemu i386 machine was that the ROM-BIOS's boot load
> would start anew, instead of loading the selected debugger as kernel.
>
> It came to light that there was a size limit: Kernel files that were 58880
> bytes (E600h) long or shorter succeeded to boot, while files that were 64000
> bytes or longer failed in the manner described.
>
> Eventually it turned out that the relocator16 stub succeeded whenever it was
> placed completely within the first 64 KiB of the Low Memory Area. The chunk
> for the relocator is allocated with a minimum address of 0x8010 and a maximum
> address just below 0xA0000 [2]. That means if the kernel is, for instance,
> E600h bytes long, then the kernel will be allocated memory starting at 00600h
> (the fixed FreeDOS kernel load address) up to E600h + 00600h = 0EC00h, which
> leaves 1400h (5120) bytes for the relocator to stay in the first 64 KiB.
> If the kernel is 64000 bytes (FA00h) long, then the relocator must go to
> FA00h + 00600h = 10000h at least which is outside the first 64 KiB.
>
> The problem is that the relocator16 initialises the DS register with a
> "pseudo real mode" descriptor, which is defined with a segment limit of
> 64 KiB and a segment base of zero. After that, the relocator addressed
> parts of itself (implicitly) using the DS register, with an offset from
> ESI, which holds the linear address of the relocator's base [3]. With the
> larger kernel files this would lead to accessing data beyond the 64 KiB
> segment limit, presumably leading to a fault and perhaps a subsequent
> triple-fault or such.
>
> This patch fixes the relocator to set the segment base of the descriptors
> to the base address of the relocator; then, the subsequent accesses to
> the relocator's variables are done without the ESI register as an index.
> This does not interfere with the relocator's or its target's normal
> operation; the segment limits are still loaded with 64 KiB and all the
> segment bases are subsequently reset by the relocator anyway.
>
> Current versions of the debugger to test are uploaded to [4]. The file
> ldebugnh.com (LZ4-compressed and built with -D_EXTHELP=0) at 58368 bytes
> loads successfully, whereas ldebug.com at 64000 bytes fails. Loading one
> of these files requires setting root to a FAT FS partition and using the
> freedos command to specify the file as kernel:
>
> set root='(hd0,msdos1)'
> freedos /ldebug.com
> boot
>
> Booting the file using the multiboot command (which uses a WIP entrypoint
> of the debugger) works, as it does not use GRUB's relocator16 but instead
> includes a loader in the kernel itself, which drops it back to 86 Mode.
>
> [1]: https://hg.ulukai.org/ecm/ldebug
> [2]: 
> http://git.savannah.gnu.org/cgit/grub.git/tree/grub-core/lib/i386/relocator.c?id=495781f5ed1b48bf27f16c53940d6700c181c74c#n127
> [3]: 
> http://git.savannah.gnu.org/cgit/grub.git/tree/grub-core/lib/i386/relocator16.S?id=495781f5ed1b48bf27f16c53940d6700c181c74c#n97
> [4]: https://ulukai.org/ecm/lDebug-5479a7988d21-nohelp.zip
>
> Signed-off-by: C. Masloch <address@hidden>

Reviewed-by: Daniel Kiper <address@hidden>

If there are no objections in a week or so I will push it.

Daniel

> ---
>  grub-core/lib/i386/relocator16.S | 26 +++++++++++++++++++-------
>  1 file changed, 19 insertions(+), 7 deletions(-)
>
> diff --git a/grub-core/lib/i386/relocator16.S 
> b/grub-core/lib/i386/relocator16.S
> index 371a2ed69..e9238119b 100644
> --- a/grub-core/lib/i386/relocator16.S
> +++ b/grub-core/lib/i386/relocator16.S
> @@ -38,15 +38,21 @@ VARIABLE(grub_relocator16_start)
>  #ifdef __APPLE__
>       LOCAL(cs_base_bytes12_offset) = LOCAL (cs_base_bytes12) - LOCAL (base)
>       LOCAL(cs_base_byte3_offset) = LOCAL (cs_base_byte3) - LOCAL (base)
> +     LOCAL(ds_base_bytes12_offset) = LOCAL (ds_base_bytes12) - LOCAL (base)
> +     LOCAL(ds_base_byte3_offset) = LOCAL (ds_base_byte3) - LOCAL (base)
>       movl    %esi, %eax
>       movw    %ax, (LOCAL(cs_base_bytes12_offset)) (RSI, 1)
> +     movw    %ax, (LOCAL(ds_base_bytes12_offset)) (RSI, 1)
>       shrl    $16, %eax
>       movb    %al, (LOCAL (cs_base_byte3_offset)) (RSI, 1)
> +     movb    %al, (LOCAL (ds_base_byte3_offset)) (RSI, 1)
>  #else
>       movl    %esi, %eax
>       movw    %ax, (LOCAL (cs_base_bytes12) - LOCAL (base)) (RSI, 1)
> +     movw    %ax, (LOCAL (ds_base_bytes12) - LOCAL (base)) (RSI, 1)
>       shrl    $16, %eax
>       movb    %al, (LOCAL (cs_base_byte3) - LOCAL (base)) (RSI, 1)
> +     movb    %al, (LOCAL (ds_base_byte3) - LOCAL (base)) (RSI, 1)
>  #endif
>
>       RELOAD_GDT
> @@ -88,15 +94,15 @@ VARIABLE(grub_relocator16_start)
>       LOCAL(segment_offset) = LOCAL (segment) - LOCAL (base)
>       LOCAL(idt_offset) = LOCAL(relocator16_idt) - LOCAL (base)
>       LOCAL(cont2_offset) = LOCAL (cont2) - LOCAL(base)
> -     movw    %ax, LOCAL(segment_offset) (%esi, 1)
> -     lidt LOCAL(idt_offset) (%esi, 1)
> +     movw    %ax, (LOCAL(segment_offset))
> +     lidt (LOCAL(idt_offset))
>
>       /* jump to a 16 bit segment */
>       ljmp    $PSEUDO_REAL_CSEG, $(LOCAL(cont2_offset))
>  #else
> -     movw    %ax, (LOCAL (segment) - LOCAL (base)) (%esi, 1)
> +     movw    %ax, (LOCAL (segment) - LOCAL (base))
>
> -     lidt (EXT_C(grub_relocator16_idt) - LOCAL (base)) (%esi, 1)
> +     lidt (EXT_C(grub_relocator16_idt) - LOCAL (base))
>
>       /* jump to a 16 bit segment */
>       ljmp    $PSEUDO_REAL_CSEG, $(LOCAL (cont2) - LOCAL(base))
> @@ -311,11 +317,17 @@ LOCAL(cs_base_byte3):
>       .byte   0x9E, 0, 0
>
>       /* -- 16 bit real mode DS --
> -      * base = 0x00000000, limit 0x0FFFF (1 B Granularity), present
> +      * base = filled by code, limit 0x0FFFF (1 B Granularity), present
>        * type = 16 bit data read/write, DPL = 0
>        */
> -     .word   0xFFFF, 0
> -     .byte   0, 0x92, 0, 0
> +     .word   0xFFFF
> +LOCAL(ds_base_bytes12):
> +     .word   0
> +LOCAL(ds_base_byte3):
> +     .byte   0
> +
> +     .byte   0x92, 0, 0
> +
>  LOCAL(gdt_end):
>
>  #ifdef __APPLE__
> --
> 2.11.0