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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:	Sat, 5 Mar 2011 06:21:31 +0100
From:	Heiko Carstens <heiko.carstens@...ibm.com>
To:	chenliu@...et.uwaterloo.ca
Cc:	schwidefsky@...ibm.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2]early: fix possible overlapping data buffer

On Fri, Mar 04, 2011 at 10:10:11AM -0500, chenliu@...et.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@...il.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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/