lkml.org 
[lkml]   [2006]   [Aug]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH] Fix the EDD code misparsing the command line
    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

    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2006-08-29 03:29    [W:0.034 / U:1.392 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site