early: fix possible overlapping data buffer

PATCH In review [email protected] 2 participants 2 messages Mar 4, 2011 – Mar 5, 2011 s390

lore

Share & tools

lore

[email protected]

author

2011-03-04 15:10 UTC #0 lore raw

PATCH 1/2 early: fix possible overlapping data buffer

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

---
 arch/s390/kernel/early.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

arch/s390/kernel/early.c +0 -0

arch/s390/kernel/early.c +2 -2

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

Heiko Carstens

2011-03-05 05:21 UTC #1 lore raw

Quoted 18 lines · On Fri, Mar 04, 2011 at 10:10:11AM -0500, [email protected] 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 <[email protected]>
---
 arch/s390/kernel/early.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

arch/s390/kernel/early.c static noinline __init void create_kerne · line 139

Quoted 9 lines

arch/s390/kernel/early.c +2 -2

@@ -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.