[PATCH] Avoid buffer overflow on strncat usage

[PATCH] Avoid buffer overflow on strncat usage

[Dirk Müller]

strncat() does not want the total size but the maximum length.

Signed-off-by: Dirk Mueller <dmueller@suse.com>
---
 kexec/fs2dt.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/kexec/fs2dt.c b/kexec/fs2dt.c
index 73c1fb9..65a8b66 100644
--- a/kexec/fs2dt.c
+++ b/kexec/fs2dt.c
@@ -649,8 +649,8 @@ static void putnode(void)
                        goto no_debug;
                }
                strncpy(filename, "/proc/device-tree/", MAXPATH);
-               strncat(filename, buff, MAXPATH);
-               strncat(filename, "/compatible", MAXPATH);
+               strncat(filename, buff, MAXPATH-strlen(filename)-1);
+               strncat(filename, "/compatible", MAXPATH-strlen(filename)-1);
                fd = open(filename, O_RDONLY);
                if (fd == -1) {
                        printf("Unable to find %s printing from
purgatory is diabled\n",
-- 
1.8.4.1

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

Re: [PATCH] Avoid buffer overflow on strncat usage

[Simon Horman]

On Mon, Feb 03, 2014 at 06:54:52PM +0100, Dirk Müller wrote:
> strncat() does not want the total size but the maximum length.
> 
> Signed-off-by: Dirk Mueller <dmueller@suse.com>
> ---
>  kexec/fs2dt.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/kexec/fs2dt.c b/kexec/fs2dt.c
> index 73c1fb9..65a8b66 100644
> --- a/kexec/fs2dt.c
> +++ b/kexec/fs2dt.c
> @@ -649,8 +649,8 @@ static void putnode(void)
>                         goto no_debug;
>                 }
>                 strncpy(filename, "/proc/device-tree/", MAXPATH);
> -               strncat(filename, buff, MAXPATH);
> -               strncat(filename, "/compatible", MAXPATH);
> +               strncat(filename, buff, MAXPATH-strlen(filename)-1);
> +               strncat(filename, "/compatible", MAXPATH-strlen(filename)-1);

I don't think you need the -1 as filename will have a trailing '\0'
which is not counted in the return value of strlen()

Also, could you please put a space on each side of each '-'?

Thanks

>                 fd = open(filename, O_RDONLY);
>                 if (fd == -1) {
>                         printf("Unable to find %s printing from
> purgatory is diabled\n",
> -- 
> 1.8.4.1
> 
> _______________________________________________
> 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

Re: [PATCH] Avoid buffer overflow on strncat usage

[Dirk Müller]

[-- Attachment #1: Type: text/plain, Size: 440 bytes --]

Hi Simon,

> I don't think you need the -1 as filename will have a trailing '\0'
> which is not counted in the return value of strlen()

Thats true, but strncat always writes a trailing NUL, and to avoid
that this one overflows the buffer, you need to subtract -1.

However, this code in particular can be rewrittten to use snprintf(),
which avoids
the overflow and is also a bit more readable.

How about the attached patch?

Thanks,
Dirk

[-- Attachment #2: 0001-Avoid-buffer-overflow-on-strncat-usage.patch --]
[-- Type: text/x-patch, Size: 1500 bytes --]

From 01d6e4371f2c49459f3600a2bbadbc66a94f870b Mon Sep 17 00:00:00 2001
From: Dirk Mueller <dmueller@suse.com>
Date: Mon, 3 Feb 2014 18:50:04 +0100
Subject: [PATCH] Avoid buffer overflow on strncat usage

strncat() does not want the total size but the maximum length
(without trailing NUL) that can still be added. Switch over
to snprintf which is both more readable and avoids this issue.

Signed-off-by: Dirk Mueller <dmueller@suse.com>
---
 kexec/fs2dt.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/kexec/fs2dt.c b/kexec/fs2dt.c
index 73c1fb9..5e6b98d 100644
--- a/kexec/fs2dt.c
+++ b/kexec/fs2dt.c
@@ -619,8 +619,7 @@ static void putnode(void)
 		 * code can print 'I'm in purgatory' message. Currently only
 		 * pseries/hvcterminal is supported.
 		 */
-		strcpy(filename, pathname);
-		strncat(filename, "linux,stdout-path", MAXPATH);
+		snprintf(filename, MAXPATH, "%slinux,stdout-path", pathname);
 		fd = open(filename, O_RDONLY);
 		if (fd == -1) {
 			printf("Unable to find %s, printing from purgatory is diabled\n",
@@ -648,9 +647,7 @@ static void putnode(void)
 														filename);
 			goto no_debug;
 		}
-		strncpy(filename, "/proc/device-tree/", MAXPATH);
-		strncat(filename, buff, MAXPATH);
-		strncat(filename, "/compatible", MAXPATH);
+		snprintf(filename, MAXPATH, "/proc/device-tree/%s/compatible", buff);
 		fd = open(filename, O_RDONLY);
 		if (fd == -1) {
 			printf("Unable to find %s printing from purgatory is diabled\n",
-- 
1.8.4.1


[-- Attachment #3: Type: text/plain, Size: 143 bytes --]

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

Re: [PATCH] Avoid buffer overflow on strncat usage

[Simon Horman]

On Tue, Feb 04, 2014 at 01:03:42PM +0100, Dirk Müller wrote:
> Hi Simon,
> 
> > I don't think you need the -1 as filename will have a trailing '\0'
> > which is not counted in the return value of strlen()
> 
> Thats true, but strncat always writes a trailing NUL, and to avoid
> that this one overflows the buffer, you need to subtract -1.
> 
> However, this code in particular can be rewrittten to use snprintf(),
> which avoids
> the overflow and is also a bit more readable.
> 
> How about the attached patch?

Looks good, I have applied it.


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