Re: [PATCH] Fix the EDD code misparsing the command line

Petr Vandrovec
2006-08-29T01:25:53Z

H. Peter Anvin wrote:
>
>
> ------------------------------------------------------------------------
>
> The EDD code would scan the command line as a fixed array, without
> taking account of either whitespace, null-termination, the old
> command-line protocol, late overrides early, or the fact that the
> command line may not be reachable from INITSEG.
>
> This should fix those problems, and enable us to use a longer command
> line.
>
> Signed-off-by: H. Peter Anvin <hpa@zytor.com>
>
>
> diff --git a/arch/i386/boot/edd.S b/arch/i386/boot/edd.S
> index 4b84ea2..03712a0 100644
> --- a/arch/i386/boot/edd.S
> +++ b/arch/i386/boot/edd.S

> + andl %esi, %esi
> + jz old_cl # Old boot protocol?
> +
> +# Convert to a real-mode pointer in fs:si
> + movl %esi, %eax
> + shrl $4, %eax
> + movw %ax, %fs
> + andw $0xf, %si
> + jmp have_cl_pointer
> +
> +# Old-style boot protocol?
> +old_cl:
> + push %ds # aka INITSEG
> + pop %fs
> +
> + cmpw $0xa33f, (0x20)
> + jne done_cl # No command line at all?
> + movw (0x22), %si # Pointer relative to INITSEG

Perhaps you should convert ds:si to flat pointer and then this flat pointer to
fs:si using method above, to avoid problems with dword access with si > 0xfffc
or word access with si > 0xfffe ?

> +
> +# fs:si has the pointer to the command line now
> +have_cl_pointer:
> +
> # loop through kernel command line one byte at a time
> -cl_loop:
> - cmpl $EDD_CL_EQUALS, (%si)
> +cl_atspace:
> + movl %fs:(%si), %eax

This looks fine for new boot protocol, but what if old boot protocol puts
command line so that its last byte is at INITSEG:0xffff ? You get #GP here,
then, although command line is correctly zero terminated and does not overflow
segment.

> + andb %al, %al # End of line?
> + jz done_cl
> + cmpl $EDD_CL_EQUALS, %eax
> jz found_edd_equals
> - incl %esi
> - loop cl_loop
> - jmp done_cl
> + cmpb $0x20, %al # <= space consider whitespace
> + ja cl_skipword
> + incw %si
> + jnz cl_atspace

You already died with #GP when si was 0xfffd or bigger above, so this ZF test is
probably not quite needed.

> + jmp done_cl # Wraparound...
> +
> +cl_skipword:
> + movb %fs:(%si), %al # End of string?
> + andb %al, %al
> + jz done_cl
> + cmpb $0x20, %al
> + jbe cl_atspace
> + incw %si
> + jnz cl_skipword
> + jmp done_cl # Wraparound...
> +
> found_edd_equals:
> # only looking at first two characters after equals
> - addl $4, %esi
> - cmpw $EDD_CL_OFF, (%si) # edd=of
> - jz do_edd_off
> - cmpw $EDD_CL_SKIP, (%si) # edd=sk
> - jz do_edd_skipmbr
> - jmp done_cl
> +# late overrides early on the command line, so keep going after finding something
> + movw %fs:4(%si), %ax

If si is 0xfffb here, bad things happen. I know, things I've pointed out should
not be problem in real world, and new code is definitely better than old one,
but if you already have code to avoid endless loop if command line points to
64KB array of 0xFF let's do that right, no?
Thanks,
Petr Vandrovec