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

[Petr Vandrovec <vandrove@vc.cvut.cz>]

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