Re: [PATCH 1/2]early: fix possible overlapping data buffer

[Heiko Carstens <heiko.carstens@de.ibm.com>]

On Fri, Mar 04, 2011 at 10:10:11AM -0500, chenliu@asset.uwaterloo.ca wrote:
> This patch fixes bugzilla #12965:
> https://bugzilla.kernel.org/show_bug.cgi?id=12965
> 
> The original code contains dangerous uses of sprintf functions like
> sprintf(defsys_cmd, "%s EW MINSIZE=%.7iK PARMREGS=0-13", defsys_cmd, min_size),
> where defsys_cmd is defined as a fixed length buffer. Replace them with
> snprintf can fix such issues.
> 
> Signed-off-by: Chen Liu <chenliunju@gmail.com>
> ---
>  arch/s390/kernel/early.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/s390/kernel/early.c b/arch/s390/kernel/early.c
> 
> --- a/arch/s390/kernel/early.c
> +++ b/arch/s390/kernel/early.c
> @@ -133,8 +133,8 @@ static noinline __init void create_kerne
>  		sinitrd_pfn = PFN_DOWN(__pa(INITRD_START));
>  		einitrd_pfn = PFN_UP(__pa(INITRD_START + INITRD_SIZE));
>  		min_size = einitrd_pfn << 2;
> -		sprintf(defsys_cmd, "%s EW %.5X-%.5X", defsys_cmd,
> -		sinitrd_pfn, einitrd_pfn);
> +		snprintf(defsys_cmd, sizeof(defsys_cmd), "%s EW %.5X-%.5X",
> +			 defsys_cmd, sinitrd_pfn, einitrd_pfn);
>  	}
>  #endif

This is not the right fix for the bugzilla. What is complained about is that
defsys_cmd is used both as input string as well as output string.
The result in this case is not really defined since all input strings must
be of type const char *, which in this case is obviously not true.
It just happens to work because the format string consumes the input string
first.
Right fix would be to remember the written bytes of the previous sprintf to
the string and use that as offset when adding something.
In _addition_ all sprintfs could be converted to snprintfs as well.