early: Fix possible overlapping data buffer

PATCH In review [email protected] 2 participants 2 messages Mar 11, 2011 – Mar 14, 2011 s390

lore

Share & tools

lore

[email protected]

author

2011-03-11 16:04 UTC #0 lore raw

PATCH 1/1 early: Fix possible overlapping data buffer

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 <[email protected]>

---
 arch/s390/kernel/early.c |   16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

arch/s390/kernel/early.c +11 -5

@@ -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);

Heiko Carstens

2011-03-14 10:02 UTC #1 lore raw

Quoted 6 lines · On Fri, Mar 11, 2011 at 11:04:42AM -0500, [email protected] 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

Quoted 7 lines

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 <[email protected]>

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