[PATCH] tomoyo: Use scnprintf() for avoiding potential buffer overflow

[PATCH] tomoyo: Use scnprintf() for avoiding potential buffer overflow

[Takashi Iwai]

Since snprintf() returns the would-be-output size instead of the
actual output size, the succeeding calls may go beyond the given
buffer limit.  Fix it by replacing with scnprintf().

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 security/tomoyo/audit.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/security/tomoyo/audit.c b/security/tomoyo/audit.c
index 3c96e8402e94..aedc93959067 100644
--- a/security/tomoyo/audit.c
+++ b/security/tomoyo/audit.c
@@ -162,7 +162,7 @@ static char *tomoyo_print_header(struct tomoyo_request_info *r)
 
 	tomoyo_convert_time(ktime_get_real_seconds(), &stamp);
 
-	pos = snprintf(buffer, tomoyo_buffer_len - 1,
+	pos = scnprintf(buffer, tomoyo_buffer_len - 1,
 		       "#%04u/%02u/%02u %02u:%02u:%02u# profile=%u mode=%s granted=%s (global-pid=%u) task={ pid=%u ppid=%u uid=%u gid=%u euid=%u egid=%u suid=%u sgid=%u fsuid=%u fsgid=%u }",
 		       stamp.year, stamp.month, stamp.day, stamp.hour,
 		       stamp.min, stamp.sec, r->profile, tomoyo_mode[r->mode],
@@ -193,7 +193,7 @@ static char *tomoyo_print_header(struct tomoyo_request_info *r)
 		dev = stat->dev;
 		mode = stat->mode;
 		if (i & 1) {
-			pos += snprintf(buffer + pos,
+			pos += scnprintf(buffer + pos,
 					tomoyo_buffer_len - 1 - pos,
 					" path%u.parent={ uid=%u gid=%u ino=%lu perm=0%o }",
 					(i >> 1) + 1,
@@ -203,7 +203,7 @@ static char *tomoyo_print_header(struct tomoyo_request_info *r)
 					stat->mode & S_IALLUGO);
 			continue;
 		}
-		pos += snprintf(buffer + pos, tomoyo_buffer_len - 1 - pos,
+		pos += scnprintf(buffer + pos, tomoyo_buffer_len - 1 - pos,
 				" path%u={ uid=%u gid=%u ino=%lu major=%u minor=%u perm=0%o type=%s",
 				(i >> 1) + 1,
 				from_kuid(&init_user_ns, stat->uid),
@@ -213,12 +213,12 @@ static char *tomoyo_print_header(struct tomoyo_request_info *r)
 				mode & S_IALLUGO, tomoyo_filetype(mode));
 		if (S_ISCHR(mode) || S_ISBLK(mode)) {
 			dev = stat->rdev;
-			pos += snprintf(buffer + pos,
+			pos += scnprintf(buffer + pos,
 					tomoyo_buffer_len - 1 - pos,
 					" dev_major=%u dev_minor=%u",
 					MAJOR(dev), MINOR(dev));
 		}
-		pos += snprintf(buffer + pos, tomoyo_buffer_len - 1 - pos,
+		pos += scnprintf(buffer + pos, tomoyo_buffer_len - 1 - pos,
 				" }");
 	}
 no_obj_info:
@@ -276,17 +276,17 @@ char *tomoyo_init_log(struct tomoyo_request_info *r, int len, const char *fmt,
 	if (!buf)
 		goto out;
 	len--;
-	pos = snprintf(buf, len, "%s", header);
+	pos = scnprintf(buf, len, "%s", header);
 	if (realpath) {
 		struct linux_binprm *bprm = r->ee->bprm;
 
-		pos += snprintf(buf + pos, len - pos,
+		pos += scnprintf(buf + pos, len - pos,
 				" exec={ realpath=\"%s\" argc=%d envc=%d %s }",
 				realpath, bprm->argc, bprm->envc, bprm_info);
 	} else if (symlink)
-		pos += snprintf(buf + pos, len - pos, " symlink.target=\"%s\"",
+		pos += scnprintf(buf + pos, len - pos, " symlink.target=\"%s\"",
 				symlink);
-	pos += snprintf(buf + pos, len - pos, "\n%s\n", domainname);
+	pos += scnprintf(buf + pos, len - pos, "\n%s\n", domainname);
 	vsnprintf(buf + pos, len - pos, fmt, args);
 out:
 	kfree(realpath);
-- 
2.16.4

Re: [PATCH] tomoyo: Use scnprintf() for avoiding potential buffer overflow

[Tetsuo Handa]

On 2020/03/11 18:36, Takashi Iwai wrote:
> Since snprintf() returns the would-be-output size instead of the
> actual output size, the succeeding calls may go beyond the given
> buffer limit.  Fix it by replacing with scnprintf().
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  security/tomoyo/audit.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)

Thanks for a patch. But current code will simply hit WARN_ON_ONCE() in vsnprintf()
if the would-be-output size went beyond the given buffer limit, and we have never
hit that warning from this function. That is, the buffer limit is large enough,
and the last byte is guaranteed to be '\0'.

Re: [PATCH] tomoyo: Use scnprintf() for avoiding potential buffer overflow

[Takashi Iwai]

On Wed, 11 Mar 2020 11:20:44 +0100,
Tetsuo Handa wrote:
> 
> On 2020/03/11 18:36, Takashi Iwai wrote:
> > Since snprintf() returns the would-be-output size instead of the
> > actual output size, the succeeding calls may go beyond the given
> > buffer limit.  Fix it by replacing with scnprintf().
> > 
> > Signed-off-by: Takashi Iwai <tiwai@suse.de>
> > ---
> >  security/tomoyo/audit.c | 18 +++++++++---------
> >  1 file changed, 9 insertions(+), 9 deletions(-)
> 
> Thanks for a patch. But current code will simply hit WARN_ON_ONCE() in vsnprintf()
> if the would-be-output size went beyond the given buffer limit, and we have never
> hit that warning from this function. That is, the buffer limit is large enough,
> and the last byte is guaranteed to be '\0'.

Right, I don't think this actually hitting the overflow, either.
If the code is intended to rely on the sanity check in vsnprintf(),
it's fine.  But I find it a bit fragile and would prefer more explicit
check in the caller side instead.


thanks,

Takashi