[PATCH] makedumpfile: Fix string append in dump_log_entry()

[PATCH] makedumpfile: Fix string append in dump_log_entry()

[Petr Tesarik]

To quote the sprintf(3) man page:

    Some programs imprudently rely on code such as the following

        sprintf(buf, "%s some further text", buf);

    to append text to buf.  However, the standards explicitly note that
    the results are undefined if source and destination buffers overlap
    when calling sprintf(), snprintf(), vsprintf(), and vsnprintf().
    Depending on the version of gcc(1) used, and the compiler options
    employed, calls such as the above will not produce the expected results.

The original code is actually miscompiled on openSUSE 13.1.

It's also overkill to call sprintf() for something that can be done
with a simple assignment.

Signed-off-by: Petr Tesarik <ptesarik@suse.cz>
---
 makedumpfile.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/makedumpfile.c b/makedumpfile.c
index 579be61..013fce7 100644
--- a/makedumpfile.c
+++ b/makedumpfile.c
@@ -3866,7 +3866,7 @@ reset_bitmap_of_free_pages(unsigned long node_zones, struct cycle *cycle)
 static int
 dump_log_entry(char *logptr, int fp)
 {
-	char *msg, *p;
+	char *msg, *p, *bufp;
 	unsigned int i, text_len;
 	unsigned long long ts_nsec;
 	char buf[BUFSIZE];
@@ -3881,18 +3881,19 @@ dump_log_entry(char *logptr, int fp)
 
 	msg = logptr + SIZE(printk_log);
 
-	sprintf(buf, "[%5lld.%06ld] ", nanos, rem/1000);
+	bufp = buf;
+	bufp += sprintf(buf, "[%5lld.%06ld] ", nanos, rem/1000);
 
 	for (i = 0, p = msg; i < text_len; i++, p++) {
 		if (isprint(*p) || isspace(*p))
-			sprintf(buf, "%s%c", buf, *p);
+			*bufp++ = *p;
 		else
-			sprintf(buf, "%s\\x%02x", buf, *p);
+			bufp += sprintf(bufp, "\\x%02x", *p);
 	}
 
-	sprintf(buf, "%s\n", buf);
+	*bufp++ = '\n';
 
-	if (write(info->fd_dumpfile, buf, strlen(buf)) < 0)
+	if (write(info->fd_dumpfile, buf, bufp - buf) < 0)
 		return FALSE;
 	else
 		return TRUE;
-- 
1.8.4.5

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

RE: [PATCH] makedumpfile: Fix string append in dump_log_entry()

[Atsushi Kumagai]

Hello Petr,

>To quote the sprintf(3) man page:
>
>    Some programs imprudently rely on code such as the following
>
>        sprintf(buf, "%s some further text", buf);
>
>    to append text to buf.  However, the standards explicitly note that
>    the results are undefined if source and destination buffers overlap
>    when calling sprintf(), snprintf(), vsprintf(), and vsnprintf().
>    Depending on the version of gcc(1) used, and the compiler options
>    employed, calls such as the above will not produce the expected results.
>
>The original code is actually miscompiled on openSUSE 13.1.
>
>It's also overkill to call sprintf() for something that can be done
>with a simple assignment.
>
>Signed-off-by: Petr Tesarik <ptesarik@suse.cz>

Thanks, it seems good to me.

Actually, Nick sent the same patch in last July and we tried to
take care of buffer overflow at the same time as below:

http://lists.infradead.org/pipermail/kexec/2013-August/009430.html

However, this thread has been left open, so I was wondering if you
could take over this work. Of course you can decline this, then I'll
do it later as another patch.


Thanks
Atsushi Kumagai

>---
> makedumpfile.c | 13 +++++++------
> 1 file changed, 7 insertions(+), 6 deletions(-)
>
>diff --git a/makedumpfile.c b/makedumpfile.c
>index 579be61..013fce7 100644
>--- a/makedumpfile.c
>+++ b/makedumpfile.c
>@@ -3866,7 +3866,7 @@ reset_bitmap_of_free_pages(unsigned long node_zones, struct cycle *cycle)
> static int
> dump_log_entry(char *logptr, int fp)
> {
>-	char *msg, *p;
>+	char *msg, *p, *bufp;
> 	unsigned int i, text_len;
> 	unsigned long long ts_nsec;
> 	char buf[BUFSIZE];
>@@ -3881,18 +3881,19 @@ dump_log_entry(char *logptr, int fp)
>
> 	msg = logptr + SIZE(printk_log);
>
>-	sprintf(buf, "[%5lld.%06ld] ", nanos, rem/1000);
>+	bufp = buf;
>+	bufp += sprintf(buf, "[%5lld.%06ld] ", nanos, rem/1000);
>
> 	for (i = 0, p = msg; i < text_len; i++, p++) {
> 		if (isprint(*p) || isspace(*p))
>-			sprintf(buf, "%s%c", buf, *p);
>+			*bufp++ = *p;
> 		else
>-			sprintf(buf, "%s\\x%02x", buf, *p);
>+			bufp += sprintf(bufp, "\\x%02x", *p);
> 	}
>
>-	sprintf(buf, "%s\n", buf);
>+	*bufp++ = '\n';
>
>-	if (write(info->fd_dumpfile, buf, strlen(buf)) < 0)
>+	if (write(info->fd_dumpfile, buf, bufp - buf) < 0)
> 		return FALSE;
> 	else
> 		return TRUE;
>--
>1.8.4.5

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

Re: [PATCH] makedumpfile: Fix string append in dump_log_entry()

[Petr Tesarik]

On Fri, 28 Feb 2014 01:01:33 +0000
Atsushi Kumagai <kumagai-atsushi@mxc.nes.nec.co.jp> wrote:

> Hello Petr,

Hello Kumagai-san,

> >To quote the sprintf(3) man page:
> >
> >    Some programs imprudently rely on code such as the following
> >
> >        sprintf(buf, "%s some further text", buf);
> >
> >    to append text to buf.  However, the standards explicitly note that
> >    the results are undefined if source and destination buffers overlap
> >    when calling sprintf(), snprintf(), vsprintf(), and vsnprintf().
> >    Depending on the version of gcc(1) used, and the compiler options
> >    employed, calls such as the above will not produce the expected results.
> >
> >The original code is actually miscompiled on openSUSE 13.1.
> >
> >It's also overkill to call sprintf() for something that can be done
> >with a simple assignment.
> >
> >Signed-off-by: Petr Tesarik <ptesarik@suse.cz>
> 
> Thanks, it seems good to me.
> 
> Actually, Nick sent the same patch in last July and we tried to
> take care of buffer overflow at the same time as below:
> 
> http://lists.infradead.org/pipermail/kexec/2013-August/009430.html
> 
> However, this thread has been left open, so I was wondering if you
> could take over this work. Of course you can decline this, then I'll
> do it later as another patch.

I don't mind taking over this work, but I don't think it's a good thing
to combine the buffer overflow fix with the sprintf buffer overlap
fix.

What is the expected plan? Are you waiting for me to send a two-patch
series now?

Petr Tesarik

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec

RE: [PATCH] makedumpfile: Fix string append in dump_log_entry()

[Atsushi Kumagai]

>> Hello Petr,
>
>Hello Kumagai-san,
>
>> >To quote the sprintf(3) man page:
>> >
>> >    Some programs imprudently rely on code such as the following
>> >
>> >        sprintf(buf, "%s some further text", buf);
>> >
>> >    to append text to buf.  However, the standards explicitly note that
>> >    the results are undefined if source and destination buffers overlap
>> >    when calling sprintf(), snprintf(), vsprintf(), and vsnprintf().
>> >    Depending on the version of gcc(1) used, and the compiler options
>> >    employed, calls such as the above will not produce the expected results.
>> >
>> >The original code is actually miscompiled on openSUSE 13.1.
>> >
>> >It's also overkill to call sprintf() for something that can be done
>> >with a simple assignment.
>> >
>> >Signed-off-by: Petr Tesarik <ptesarik@suse.cz>
>>
>> Thanks, it seems good to me.
>>
>> Actually, Nick sent the same patch in last July and we tried to
>> take care of buffer overflow at the same time as below:
>>
>> http://lists.infradead.org/pipermail/kexec/2013-August/009430.html
>>
>> However, this thread has been left open, so I was wondering if you
>> could take over this work. Of course you can decline this, then I'll
>> do it later as another patch.
>
>I don't mind taking over this work, but I don't think it's a good thing
>to combine the buffer overflow fix with the sprintf buffer overlap
>fix.
>
>What is the expected plan? Are you waiting for me to send a two-patch
>series now?

Yes, I agree with separating the two fixes.
Thanks for your help !


Atsushi Kumagai

>Petr Tesarik
>
>_______________________________________________
>kexec mailing list
>kexec@lists.infradead.org
>http://lists.infradead.org/mailman/listinfo/kexec

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec