[PATCH]early: fix possible overlapping data buffer

[PATCH]early: fix possible overlapping data buffer

[chenliu]

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 used both as input string as well as output string. It should remember the written bytes of the previous sprintf and use that as the offset when adding something. Also use snprintf makes the code safer.

Signed-off-by: Chen Liu<chenliunju@gmail.com>
---
 arch/s390/kernel/early.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 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
@@ -94,6 +94,7 @@ static noinline __init void create_kerne
 	unsigned int sinitrd_pfn, einitrd_pfn;
 #endif
 	int response;
+	int hlen;
 	size_t len;
 	char *savesys_ptr;
 	char defsys_cmd[DEFSYS_CMD_SIZE];
@@ -124,7 +125,7 @@ static noinline __init void create_kerne
 	end_pfn = PFN_UP(__pa(&_end));
 	min_size = end_pfn << 2;
 
-	sprintf(defsys_cmd, "DEFSYS %s 00000-%.5X EW %.5X-%.5X SR %.5X-%.5X",
+	hlen = sprintf(defsys_cmd, "DEFSYS %s 00000-%.5X EW %.5X-%.5X SR %.5X-%.5X",
 		kernel_nss_name, stext_pfn - 1, stext_pfn, eshared_pfn - 1,
 		eshared_pfn, end_pfn);
 
@@ -133,13 +134,13 @@ 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,
+		hlen += snprintf(defsys_cmd + hlen, DEFSYS_CMD_SIZE - hlen, " EW %.5X-%.5X",
 		sinitrd_pfn, einitrd_pfn);
 	}
 #endif
 
-	sprintf(defsys_cmd, "%s EW MINSIZE=%.7iK PARMREGS=0-13",
-		defsys_cmd, min_size);
+	hlen += snprintf(defsys_cmd + hlen, DEFSYS_CMD_SIZE - hlen, " EW MINSIZE=%.7iK PARMREGS=0-13",
+		min_size);
 	sprintf(savesys_cmd, "SAVESYS %s \n IPL %s",
 		kernel_nss_name, kernel_nss_name);
 

[PATCH]early: Fix possible overlapping data buffer

[chenliu]

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 used both as input string as well as output string. It should remember the written bytes of the previous sprintf and use that as the offset when adding something. Also use snprintf makes the code safer.

Signed-off-by: Chen Liu<chenliunju@gmail.com>
---
 arch/s390/kernel/early.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 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
@@ -94,6 +94,7 @@ static noinline __init void create_kerne
 	unsigned int sinitrd_pfn, einitrd_pfn;
 #endif
 	int response;
+	int hlen;
 	size_t len;
 	char *savesys_ptr;
 	char defsys_cmd[DEFSYS_CMD_SIZE];
@@ -124,7 +125,7 @@ static noinline __init void create_kerne
 	end_pfn = PFN_UP(__pa(&_end));
 	min_size = end_pfn << 2;
 
-	sprintf(defsys_cmd, "DEFSYS %s 00000-%.5X EW %.5X-%.5X SR %.5X-%.5X",
+	hlen = sprintf(defsys_cmd, "DEFSYS %s 00000-%.5X EW %.5X-%.5X SR %.5X-%.5X",
 		kernel_nss_name, stext_pfn - 1, stext_pfn, eshared_pfn - 1,
 		eshared_pfn, end_pfn);
 
@@ -133,13 +134,13 @@ 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,
+		hlen += snprintf(defsys_cmd + hlen, DEFSYS_CMD_SIZE - hlen, " EW %.5X-%.5X",
 		sinitrd_pfn, einitrd_pfn);
 	}
 #endif
 
-	sprintf(defsys_cmd, "%s EW MINSIZE=%.7iK PARMREGS=0-13",
-		defsys_cmd, min_size);
+	hlen += snprintf(defsys_cmd + hlen, DEFSYS_CMD_SIZE - hlen, " EW MINSIZE=%.7iK PARMREGS=0-13",
+		min_size);
 	sprintf(savesys_cmd, "SAVESYS %s \n IPL %s",
 		kernel_nss_name, kernel_nss_name);
 

Re: [PATCH]early: Fix possible overlapping data buffer

[Heiko Carstens]

On Thu, Mar 10, 2011 at 12:33:37PM -0500, chenliu@asset.uwaterloo.ca wrote:
> This patch fixes bugzilla #12965:
> https://bugzilla.kernel.org/show_bug.cgi?id=12965

Not sure it's worth all the hazzle. After all it's just a theoratical bug,
but...

> 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 used both as input string as well as output string. It
> should remember the written bytes of the previous sprintf and use that as the
> offset when adding something. Also use snprintf makes the code safer.
> 
> Signed-off-by: Chen Liu<chenliunju@gmail.com>

Please make sure your From: and Sign-off email addresses match.
Also it would be nice if you could add some line breaks in your patch
description ;)

Also it would be good to know if you tested the patch. Does saving
an NSS still work with your patch applied?

> ---
>  arch/s390/kernel/early.c |    9 +++++----
>  1 file changed, 5 insertions(+), 4 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
> @@ -94,6 +94,7 @@ static noinline __init void create_kerne
>  	unsigned int sinitrd_pfn, einitrd_pfn;
>  #endif
>  	int response;
> +	int hlen;
>  	size_t len;
>  	char *savesys_ptr;
>  	char defsys_cmd[DEFSYS_CMD_SIZE];
> @@ -124,7 +125,7 @@ static noinline __init void create_kerne
>  	end_pfn = PFN_UP(__pa(&_end));
>  	min_size = end_pfn << 2;
> 
> -	sprintf(defsys_cmd, "DEFSYS %s 00000-%.5X EW %.5X-%.5X SR %.5X-%.5X",
> +	hlen = sprintf(defsys_cmd, "DEFSYS %s 00000-%.5X EW %.5X-%.5X SR %.5X-%.5X",
>  		kernel_nss_name, stext_pfn - 1, stext_pfn, eshared_pfn - 1,
>  		eshared_pfn, end_pfn);

Your patch introduced a new line with more than 80 characters long. You also
broke indentation since we have a multiline function call with parameters here.
In addition you add snprintf calls later. Why not here?
The checkpatch tool might give you some hints.


> @@ -133,13 +134,13 @@ 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,
> +		hlen += snprintf(defsys_cmd + hlen, DEFSYS_CMD_SIZE - hlen, " EW %.5X-%.5X",
>  		sinitrd_pfn, einitrd_pfn);

Line too long. Plus indentation broken (well, it was already broken).
If you add an snprintf() call, then you should also make sure that the
resulting string is '\0' terminated.

>  	}
>  #endif
> 
> -	sprintf(defsys_cmd, "%s EW MINSIZE=%.7iK PARMREGS=0-13",
> -		defsys_cmd, min_size);
> +	hlen += snprintf(defsys_cmd + hlen, DEFSYS_CMD_SIZE - hlen, " EW MINSIZE=%.7iK PARMREGS=0-13",
> +		min_size);

Line too long and indentation broken. Also again you need to make sure that
the string is '\0' terminated.
In addition hlen is unused afterwards. So no need to update it here.

[PATCH]early: Fix possible overlapping data buffer

[chenliu]

Thanks Heiko. I've followed your advice to give a new patch here.
---------------------------------

This patch fixed bugzilla #12965:
https://bugzilla.kernel.org/show_bug.cgi?id=12965

The original code contains some inproper use of sprintf
function where a buffer is used both as input string
as well as output string. It should remember the written
bytes in the previous and use that as the offset for
later writing. Also replace sprintf with snprintf.

Signed-off-by: Chen Liu <chenliunju@gmail.com>
---
 arch/s390/kernel/early.c |   16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 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
@@ -94,6 +94,7 @@ static noinline __init void create_kerne
 	unsigned int sinitrd_pfn, einitrd_pfn;
 #endif
 	int response;
+	int hlen;
 	size_t len;
 	char *savesys_ptr;
 	char defsys_cmd[DEFSYS_CMD_SIZE];
@@ -124,22 +125,27 @@ static noinline __init void create_kerne
 	end_pfn = PFN_UP(__pa(&_end));
 	min_size = end_pfn << 2;
 
-	sprintf(defsys_cmd, "DEFSYS %s 00000-%.5X EW %.5X-%.5X SR %.5X-%.5X",
+	snprintf(defsys_cmd, sizeof(defsys_cmd),
+		"DEFSYS %s 00000-%.5X EW %.5X-%.5X SR %.5X-%.5X",
 		kernel_nss_name, stext_pfn - 1, stext_pfn, eshared_pfn - 1,
 		eshared_pfn, end_pfn);
+	defsys_cmd[DEFSYS_CMD_SIZE - 1] = '\0';
 
 #ifdef CONFIG_BLK_DEV_INITRD
 	if (INITRD_START && INITRD_SIZE) {
 		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);
+		hlen += snprintf(defsys_cmd, DEFSYS_CMD_SIZE - hlen,
+			" EW %.5X-%.5X", defsys_cmd,
+			sinitrd_pfn, einitrd_pfn);
+		defsys_cmd[DEFSYS_CMD_SIZE - 1] = '\0';
 	}
 #endif
 
-	sprintf(defsys_cmd, "%s EW MINSIZE=%.7iK PARMREGS=0-13",
-		defsys_cmd, min_size);
+	snprintf(defsys_cmd + hlen, DEFSYS_CMD_SIZE - hlen,
+		" EW MINSIZE=%.7iK PARMREGS=0-13", min_size);
+	defsys_cmd[DEFSYS_CMD_SIZE - 1] = '\0';
 	sprintf(savesys_cmd, "SAVESYS %s \n IPL %s",
 		kernel_nss_name, kernel_nss_name);
 

Re: [PATCH]early: Fix possible overlapping data buffer

[Heiko Carstens]

On Fri, Mar 11, 2011 at 11:04:42AM -0500, chenliu@asset.uwaterloo.ca wrote:
> Thanks Heiko. I've followed your advice to give a new patch here.
> ---------------------------------
> 
> This patch fixed bugzilla #12965:
> https://bugzilla.kernel.org/show_bug.cgi?id=12965
> 
> The original code contains some inproper use of sprintf
> function where a buffer is used both as input string
> as well as output string. It should remember the written
> bytes in the previous and use that as the offset for
> later writing. Also replace sprintf with snprintf.
> 
> Signed-off-by: Chen Liu <chenliunju@gmail.com>

Hi Chen,

I still want a patch where the From: and Signed-off-by: lines
are identical. Otherwise I won't take the patch.

Also you didn't answer my other question: did you actually
test that after your change the code still works?
If not, then I would have to do that.

The patch looks ok so far (besides two lines that could
be removed).

Thanks,
Heiko

[PATCH]early: Fix possible overlapping data buffer

[chenliu]

Thanks Heiko. This patch hasn't been tested yet. I've modifed
the Signed=off-by to make it identical to From. Here is the
patch:
-------------------------------------------------------
This patch fixed bugzilla #12965:
https://bugzilla.kernel.org/show_bug.cgi?id=12965

The original code contains some inproper use of sprintf
function where a buffer is used both as input string
as well as output string. It should remember the written
bytes in the previous and use that as the offset for
later writing. Also replace sprintf with snprintf.

Signed-off-by: Chen Liu <chenliu@asset.uwaterloo.ca>
---
 arch/s390/kernel/early.c |   16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 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
@@ -94,6 +94,7 @@ static noinline __init void create_kerne
 	unsigned int sinitrd_pfn, einitrd_pfn;
 #endif
 	int response;
+	int hlen;
 	size_t len;
 	char *savesys_ptr;
 	char defsys_cmd[DEFSYS_CMD_SIZE];
@@ -124,22 +125,27 @@ static noinline __init void create_kerne
 	end_pfn = PFN_UP(__pa(&_end));
 	min_size = end_pfn << 2;
 
-	sprintf(defsys_cmd, "DEFSYS %s 00000-%.5X EW %.5X-%.5X SR %.5X-%.5X",
+	snprintf(defsys_cmd, sizeof(defsys_cmd),
+		"DEFSYS %s 00000-%.5X EW %.5X-%.5X SR %.5X-%.5X",
 		kernel_nss_name, stext_pfn - 1, stext_pfn, eshared_pfn - 1,
 		eshared_pfn, end_pfn);
+	defsys_cmd[DEFSYS_CMD_SIZE - 1] = '\0';
 
 #ifdef CONFIG_BLK_DEV_INITRD
 	if (INITRD_START && INITRD_SIZE) {
 		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);
+		hlen += snprintf(defsys_cmd, DEFSYS_CMD_SIZE - hlen,
+			" EW %.5X-%.5X", defsys_cmd,
+			sinitrd_pfn, einitrd_pfn);
+		defsys_cmd[DEFSYS_CMD_SIZE - 1] = '\0';
 	}
 #endif
 
-	sprintf(defsys_cmd, "%s EW MINSIZE=%.7iK PARMREGS=0-13",
-		defsys_cmd, min_size);
+	snprintf(defsys_cmd + hlen, DEFSYS_CMD_SIZE - hlen,
+		" EW MINSIZE=%.7iK PARMREGS=0-13", min_size);
+	defsys_cmd[DEFSYS_CMD_SIZE - 1] = '\0';
 	sprintf(savesys_cmd, "SAVESYS %s \n IPL %s",
 		kernel_nss_name, kernel_nss_name);
 

Re: [PATCH]early: Fix possible overlapping data buffer

[Heiko Carstens]

On Mon, Mar 14, 2011 at 11:25:32AM -0400, chenliu@asset.uwaterloo.ca wrote:
> Thanks Heiko. This patch hasn't been tested yet. I've modifed
> the Signed=off-by to make it identical to From. Here is the
> patch:

Yes, obviously untested (see below).

> Signed-off-by: Chen Liu <chenliu@asset.uwaterloo.ca>
> ---
>  arch/s390/kernel/early.c |   16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 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
> @@ -94,6 +94,7 @@ static noinline __init void create_kerne
>  	unsigned int sinitrd_pfn, einitrd_pfn;
>  #endif
>  	int response;
> +	int hlen;
>  	size_t len;
>  	char *savesys_ptr;
>  	char defsys_cmd[DEFSYS_CMD_SIZE];
> @@ -124,22 +125,27 @@ static noinline __init void create_kerne
>  	end_pfn = PFN_UP(__pa(&_end));
>  	min_size = end_pfn << 2;
> 
> -	sprintf(defsys_cmd, "DEFSYS %s 00000-%.5X EW %.5X-%.5X SR %.5X-%.5X",
> +	snprintf(defsys_cmd, sizeof(defsys_cmd),
> +		"DEFSYS %s 00000-%.5X EW %.5X-%.5X SR %.5X-%.5X",
>  		kernel_nss_name, stext_pfn - 1, stext_pfn, eshared_pfn - 1,
>  		eshared_pfn, end_pfn);
> +	defsys_cmd[DEFSYS_CMD_SIZE - 1] = '\0';
> 
>  #ifdef CONFIG_BLK_DEV_INITRD
>  	if (INITRD_START && INITRD_SIZE) {
>  		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);
> +		hlen += snprintf(defsys_cmd, DEFSYS_CMD_SIZE - hlen,

hlen is unitialized here, since you forgot to save the size in the
snprintf statement above.

> +			" EW %.5X-%.5X", defsys_cmd,

defsys_cmd is still in the input parameter list. The whole point was to
remove it. ;)

Anyway, I fixed it and applied your patch. Thanks!