tomoyo: Use scnprintf() for avoiding potential buffer overflow
diff mbox series

Message ID 20200311093627.25409-1-tiwai@suse.de
State New
Headers show
Series
  • tomoyo: Use scnprintf() for avoiding potential buffer overflow
Related show

Commit Message

Takashi Iwai March 11, 2020, 9:36 a.m. UTC
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(-)

Comments

Tetsuo Handa March 11, 2020, 10:20 a.m. UTC | #1
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'.
Takashi Iwai March 11, 2020, 11:06 a.m. UTC | #2
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

Patch
diff mbox series

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