reiserfs: check/extend buffer length for printing functions

If format string and/or error string are larger than 1024, it will cause memory overflow.

So need check the format string buffer length before process it.

Also need use (v)snprintf() instread of (v)sprintf() for error buffer to be sure of maximize length limitation.

Normally, the error buffer length need be much larger than format buffer length, so extend the error buffer length to 4096.

When adding new code, also need let them within 80 column.

Signed-off-by Chen Gang <[email protected]>

---
 fs/reiserfs/prints.c |   90 ++++++++++++++++++++++++++++++-------------------
 1 files changed, 55 insertions(+), 35 deletions(-)

fs/reiserfs/prints.c +55 -35

index c0b1112..6b7581e 100644
@@ -10,8 +10,13 @@
 
 #include <stdarg.h>
 
-static char error_buf[1024];
-static char fmt_buf[1024];
+#define REISERFS_MAX_FMT_BUF		1024
+#define REISERFS_MAX_ERROR_BUF		4096
+#define REISERFS_ERR_BUF_LEFT(pos, base)	\
+				(REISERFS_MAX_ERROR_BUF - ((pos) - (base)))
+
+static char error_buf[REISERFS_MAX_FMT_BUF];
+static char fmt_buf[REISERFS_MAX_ERROR_BUF];
 static char off_buf[80];
 
 static char *reiserfs_cpu_offset(struct cpu_key *key)

@@ -76,72 +81,74 @@ static char *le_type(struct reiserfs_key *key)
 }
 
 /* %k */
-static void sprintf_le_key(char *buf, struct reiserfs_key *key)
+static void sprintf_le_key(char *buf, int left, struct reiserfs_key *key)
 {
 	if (key)
-		sprintf(buf, "[%d %d %s %s]", le32_to_cpu(key->k_dir_id),
+		snprintf(buf, left, "[%d %d %s %s]", le32_to_cpu(key->k_dir_id),
 			le32_to_cpu(key->k_objectid), le_offset(key),
 			le_type(key));
 	else
-		sprintf(buf, "[NULL]");
+		snprintf(buf, left, "[NULL]");
 }
 
 /* %K */
-static void sprintf_cpu_key(char *buf, struct cpu_key *key)
+static void sprintf_cpu_key(char *buf, int left, struct cpu_key *key)
 {
 	if (key)
-		sprintf(buf, "[%d %d %s %s]", key->on_disk_key.k_dir_id,
+		snprintf(buf, left, "[%d %d %s %s]", key->on_disk_key.k_dir_id,
 			key->on_disk_key.k_objectid, reiserfs_cpu_offset(key),
 			cpu_type(key));
 	else
-		sprintf(buf, "[NULL]");
+		snprintf(buf, left, "[NULL]");
 }
 
-static void sprintf_de_head(char *buf, struct reiserfs_de_head *deh)
+static void sprintf_de_head(char *buf, int left, struct reiserfs_de_head *deh)
 {
 	if (deh)
-		sprintf(buf,
+		snprintf(buf, left,
 			"[offset=%d dir_id=%d objectid=%d location=%d state=%04x]",
 			deh_offset(deh), deh_dir_id(deh), deh_objectid(deh),
 			deh_location(deh), deh_state(deh));
 	else
-		sprintf(buf, "[NULL]");
+		snprintf(buf, left, "[NULL]");
 
 }
 
-static void sprintf_item_head(char *buf, struct item_head *ih)
+static void sprintf_item_head(char *buf, int left, struct item_head *ih)
 {
 	if (ih) {
-		strcpy(buf,
+		snprintf(buf, left, "%s",
 		       (ih_version(ih) == KEY_FORMAT_3_6) ? "*3.6* " : "*3.5*");
-		sprintf_le_key(buf + strlen(buf), &(ih->ih_key));
-		sprintf(buf + strlen(buf), ", item_len %d, item_location %d, "
-			"free_space(entry_count) %d",
+		sprintf_le_key(buf + strlen(buf), left - strlen(buf),
+			       &(ih->ih_key));
+		snprintf(buf + strlen(buf), left - strlen(buf),
+			 ", item_len %d, item_location %d, free_space(entry_count) %d",
 			ih_item_len(ih), ih_location(ih), ih_free_space(ih));
 	} else
-		sprintf(buf, "[NULL]");
+		snprintf(buf, left, "[NULL]");
 }
 
-static void sprintf_direntry(char *buf, struct reiserfs_dir_entry *de)
+static void sprintf_direntry(char *buf, int left, struct reiserfs_dir_entry *de)
 {
 	char name[20];
 
 	memcpy(name, de->de_name, de->de_namelen > 19 ? 19 : de->de_namelen);
 	name[de->de_namelen > 19 ? 19 : de->de_namelen] = 0;
-	sprintf(buf, "\"%s\"==>[%d %d]", name, de->de_dir_id, de->de_objectid);
+	snprintf(buf, left, "\"%s\"==>[%d %d]",
+		 name, de->de_dir_id, de->de_objectid);
 }
 
-static void sprintf_block_head(char *buf, struct buffer_head *bh)
+static void sprintf_block_head(char *buf, int left, struct buffer_head *bh)
 {
-	sprintf(buf, "level=%d, nr_items=%d, free_space=%d rdkey ",
+	snprintf(buf, left, "level=%d, nr_items=%d, free_space=%d rdkey ",
 		B_LEVEL(bh), B_NR_ITEMS(bh), B_FREE_SPACE(bh));
 }
 
-static void sprintf_buffer_head(char *buf, struct buffer_head *bh)
+static void sprintf_buffer_head(char *buf, int left, struct buffer_head *bh)
 {
 	char b[BDEVNAME_SIZE];
 
-	sprintf(buf,
+	snprintf(buf, left,
 		"dev %s, size %zd, blocknr %llu, count %d, state 0x%lx, page %p, (%s, %s, %s)",
 		bdevname(bh->b_bdev, b), bh->b_size,
 		(unsigned long long)bh->b_blocknr, atomic_read(&(bh->b_count)),

@@ -151,9 +158,9 @@ static void sprintf_buffer_head(char *buf, struct buffer_head *bh)
 		buffer_locked(bh) ? "LOCKED" : "UNLOCKED");
 }
 
-static void sprintf_disk_child(char *buf, struct disk_child *dc)
+static void sprintf_disk_child(char *buf, int left, struct disk_child *dc)
 {
-	sprintf(buf, "[dc_number=%d, dc_size=%u]", dc_block_number(dc),
+	snprintf(buf, left, "[dc_number=%d, dc_size=%u]", dc_block_number(dc),
 		dc_size(dc));
 }
 

@@ -190,8 +197,16 @@ static void prepare_error_buf(const char *fmt, va_list args)
 	char *fmt1 = fmt_buf;
 	char *k;
 	char *p = error_buf;
+	int left = REISERFS_ERR_BUF_LEFT(p, error_buf);
 	int what;
 
+	if (strlen(fmt) >= REISERFS_MAX_FMT_BUF) {
+		printk(KERN_CRIT
+			"REISERFS error (format buffer too long, more than %d): %s\n",
+			REISERFS_MAX_FMT_BUF, fmt);
+		return;
+	}
+
 	spin_lock(&error_lock);
 
 	strcpy(fmt1, fmt);

@@ -199,46 +214,51 @@ static void prepare_error_buf(const char *fmt, va_list args)
 	while ((k = is_there_reiserfs_struct(fmt1, &what)) != NULL) {
 		*k = 0;
 
-		p += vsprintf(p, fmt1, args);
+		p += vsnprintf(p, left, fmt1, args);
+		left = REISERFS_ERR_BUF_LEFT(p, error_buf);
 
 		switch (what) {
 		case 'k':
-			sprintf_le_key(p, va_arg(args, struct reiserfs_key *));
+			sprintf_le_key(p, left,
+				       va_arg(args, struct reiserfs_key *));
 			break;
 		case 'K':
-			sprintf_cpu_key(p, va_arg(args, struct cpu_key *));
+			sprintf_cpu_key(p, left,
+					va_arg(args, struct cpu_key *));
 			break;
 		case 'h':
-			sprintf_item_head(p, va_arg(args, struct item_head *));
+			sprintf_item_head(p, left,
+					  va_arg(args, struct item_head *));
 			break;
 		case 't':
-			sprintf_direntry(p,
+			sprintf_direntry(p, left,
 					 va_arg(args,
 						struct reiserfs_dir_entry *));
 			break;
 		case 'y':
-			sprintf_disk_child(p,
+			sprintf_disk_child(p, left,
 					   va_arg(args, struct disk_child *));
 			break;
 		case 'z':
-			sprintf_block_head(p,
+			sprintf_block_head(p, left,
 					   va_arg(args, struct buffer_head *));
 			break;
 		case 'b':
-			sprintf_buffer_head(p,
+			sprintf_buffer_head(p, left,
 					    va_arg(args, struct buffer_head *));
 			break;
 		case 'a':
-			sprintf_de_head(p,
+			sprintf_de_head(p, left,
 					va_arg(args,
 					       struct reiserfs_de_head *));
 			break;
 		}
 
 		p += strlen(p);
+		left = REISERFS_ERR_BUF_LEFT(p, error_buf);
 		fmt1 = k + 2;
 	}
-	vsprintf(p, fmt1, args);
+	vsnprintf(p, left, fmt1, args);
 	spin_unlock(&error_lock);
 
 }

fs/reiserfs/prints.c static void prepare_error_buf(const char *fmt, va_list args) · line 217

Quoted 8 lines

fs/reiserfs/prints.c +1 -1

 	strcpy(fmt1, fmt);

@@ -199,46 +214,51 @@ static void prepare_error_buf(const char *fmt, va_list args)
 	while ((k = is_there_reiserfs_struct(fmt1, &what)) != NULL) {
 		*k = 0;
 
-		p += vsprintf(p, fmt1, args);
+		p += vsnprintf(p, left, fmt1, args);

Quoted 12 lines

fs/reiserfs/prints.c +4 -2

+		left = REISERFS_ERR_BUF_LEFT(p, error_buf);
 
 		switch (what) {
 		case 'k':
-			sprintf_le_key(p, va_arg(args, struct reiserfs_key *));
+			sprintf_le_key(p, left,
+				       va_arg(args, struct reiserfs_key *));
 			break;
 		case 'K':
-			sprintf_cpu_key(p, va_arg(args, struct cpu_key *));
+			sprintf_cpu_key(p, left,
 

Quoted 6 lines · Chen Gang

Quoted 6 lines

fs/reiserfs/prints.c +0 -0

 	strcpy(fmt1, fmt);

@@ -199,46 +214,51 @@ static void prepare_error_buf(const char *fmt, va_list args)
 	while ((k = is_there_reiserfs_struct(fmt1, &what)) != NULL) {
 		*k = 0;
 

Quoted 2 lines

fs/reiserfs/prints.c static void prepare_error_buf(const char *fmt, va_list args) · line 217

Quoted 2 lines

fs/reiserfs/prints.c +1 -1

-		p += vsprintf(p, fmt1, args);
+		p += vsnprintf(p, left, fmt1, args);

At least, need use vscnprintf() instead of vsnprintf(), since we need the real written length return.

Quoted 7 lines · Chen Gang

Quoted 7 lines · Chen Gang

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

(quoted diff) +0 -0

 	strcpy(fmt1, fmt);

@@ -199,46 +214,51 @@ static void prepare_error_buf(const char *fmt, va_list args)
 	while ((k = is_there_reiserfs_struct(fmt1, &what)) != NULL) {
 		*k = 0;
 

Quoted 5 lines

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Review static void prepare_error_buf(const char *fmt, va_list args) · line 217

Quoted 2 lines

(quoted diff) +1 -1

-		p += vsprintf(p, fmt1, args);
+		p += vsnprintf(p, left, fmt1, args);

At least, need use vscnprintf() instead of vsnprintf(), since we need the real written length return.

		n = vsnprintf(p, left, ....);
		left -= n;
		if (left <= 0) /* overflow */
			break;	/* or whatever's suitable here */
		p += n;

Quoted 13 lines · Al Viro

Quoted 7 lines · Chen Gang

Quoted 7 lines · Chen Gang

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

(quoted diff) +0 -0

 	strcpy(fmt1, fmt);

@@ -199,46 +214,51 @@ static void prepare_error_buf(const char *fmt, va_list args)
 	while ((k = is_there_reiserfs_struct(fmt1, &what)) != NULL) {
 		*k = 0;
 

Quoted 5 lines

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Review static void prepare_error_buf(const char *fmt, va_list args) · line 217

Quoted 2 lines

(quoted diff) +1 -1

-		p += vsprintf(p, fmt1, args);
+		p += vsnprintf(p, left, fmt1, args);

At least, need use vscnprintf() instead of vsnprintf(), since we need the real written length return.

Quoted 7 lines

		n = vsnprintf(p, left, ....);
		left -= n;
		if (left <= 0) /* overflow */
			break;	/* or whatever's suitable here */
		p += n;

Yeah, it is really a better fix. :-)

And now I am just testing, and find another issue about it, I am just analyzing it it.

For next-20130717, let reiserfs build-in, when "mount /dev/sda11 /mnt/sda11" (assume sda11 is reiserfs filesystem).

I modify the code like this (just only use vsnprintf instead of vsprintf):

--------------------------diff begin------------------------------

fs/reiserfs/prints.c +4 -4

index c0b1112..3a38a62 100644
@@ -10,7 +10,7 @@
 
 #include <stdarg.h>
 
-static char error_buf[1024];
+static char error_buf[13];
 static char fmt_buf[1024];
 static char off_buf[80];
 

@@ -195,7 +195,7 @@ static void prepare_error_buf(const char *fmt, va_list args)
 	spin_lock(&error_lock);
 
 	strcpy(fmt1, fmt);
-
+#if 0
 	while ((k = is_there_reiserfs_struct(fmt1, &what)) != NULL) {
 		*k = 0;
 

@@ -238,7 +238,8 @@ static void prepare_error_buf(const char *fmt, va_list args)
 		p += strlen(p);
 		fmt1 = k + 2;
 	}
-	vsprintf(p, fmt1, args);
+#endif
+	vsnprintf(p, 13, fmt1, args);
 	spin_unlock(&error_lock);
 
 }
 
--------------------------diff end--------------------------------

The output has '<7>':

[root@dhcp122 ~]# dmesg

[   38.797073] REISERFS (device sda11): found reiser
[   38.797089] REISERFS warning (device sda11):  reiserfs_fill_super: CONFIG_REISE
[   38.797095] REISERFS warning (device sda11):  reiserfs_fill_super: - it is slow
[   38.797098] REISERFS (device sda11): using orderereiserfs: using flush barriers
[   38.800507] REISERFS (device sda11): journal para
[   38.801158] REISERFS (device sda11): checking tra<7>[   38.801165] REISERFS debug (device sda11): journal-1153
[   38.801405] REISERFS debug (device sda11): journal-1206
[   38.801410] REISERFS debug (device sda11): journal-1299
[   38.817621] REISERFS (device sda11): Using r5 has
[   38.817906] SELinux: initialized (dev sda11, type reiserfs), uses genfs_contexts

Welcome any suggestions or completions.

Thanks.

Quoted 22 lines · Chen Gang

Quoted 20 lines · Al Viro

Quoted 7 lines · Chen Gang

Quoted 7 lines · Chen Gang

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

Quoted 6 lines

(quoted diff) +0 -0

 	strcpy(fmt1, fmt);

@@ -199,46 +214,51 @@ static void prepare_error_buf(const char *fmt, va_list args)
 	while ((k = is_there_reiserfs_struct(fmt1, &what)) != NULL) {
 		*k = 0;
 

Quoted 5 lines

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

Quoted 2 lines

Review static void prepare_error_buf(const char *fmt, va_list args) · line 217

Quoted 2 lines

(quoted diff) +1 -1

-		p += vsprintf(p, fmt1, args);
+		p += vsnprintf(p, left, fmt1, args);

At least, need use vscnprintf() instead of vsnprintf(), since we need the real written length return.

		n = vsnprintf(p, left, ....);
		left -= n;
		if (left <= 0) /* overflow */
			break;	/* or whatever's suitable here */
		p += n;

Quoted 5 lines

Yeah, it is really a better fix. :-)

And now I am just testing, and find another issue about it, I am just analyzing it it.

even only change vsprintf() to vsnprintf(), it will report '<7' !!

----------------------------diff begin---------------------------------

fs/reiserfs/prints.c +2 -2

index c0b1112..9dd79be 100644
@@ -238,7 +238,8 @@ static void prepare_error_buf(const char *fmt, va_list args)
 		p += strlen(p);
 		fmt1 = k + 2;
 	}
-	vsprintf(p, fmt1, args);
+
+	vsnprintf(p, 13, fmt1, args);
 	spin_unlock(&error_lock);
 
 }
 
----------------------------diff end-----------------------------------

This patch seems related with this new issue. So After I finish analyzing it (get root cause), then send the patch v2 for this patch.

Thanks.

Quoted 64 lines

For next-20130717, let reiserfs build-in, when "mount /dev/sda11 /mnt/sda11" (assume sda11 is reiserfs filesystem).

I modify the code like this (just only use vsnprintf instead of vsprintf):

--------------------------diff begin------------------------------

fs/reiserfs/prints.c +4 -4

index c0b1112..3a38a62 100644
@@ -10,7 +10,7 @@
 
 #include <stdarg.h>
 
-static char error_buf[1024];
+static char error_buf[13];
 static char fmt_buf[1024];
 static char off_buf[80];
 

@@ -195,7 +195,7 @@ static void prepare_error_buf(const char *fmt, va_list args)
 	spin_lock(&error_lock);
 
 	strcpy(fmt1, fmt);
-
+#if 0
 	while ((k = is_there_reiserfs_struct(fmt1, &what)) != NULL) {
 		*k = 0;
 

@@ -238,7 +238,8 @@ static void prepare_error_buf(const char *fmt, va_list args)
 		p += strlen(p);
 		fmt1 = k + 2;
 	}
-	vsprintf(p, fmt1, args);
+#endif
+	vsnprintf(p, 13, fmt1, args);
 	spin_unlock(&error_lock);
 
 }
 
--------------------------diff end--------------------------------

The output has '<7>':

[root@dhcp122 ~]# dmesg

[   38.797073] REISERFS (device sda11): found reiser
[   38.797089] REISERFS warning (device sda11):  reiserfs_fill_super: CONFIG_REISE
[   38.797095] REISERFS warning (device sda11):  reiserfs_fill_super: - it is slow
[   38.797098] REISERFS (device sda11): using orderereiserfs: using flush barriers
[   38.800507] REISERFS (device sda11): journal para
[   38.801158] REISERFS (device sda11): checking tra<7>[   38.801165] REISERFS debug (device sda11): journal-1153
[   38.801405] REISERFS debug (device sda11): journal-1206
[   38.801410] REISERFS debug (device sda11): journal-1299
[   38.817621] REISERFS (device sda11): Using r5 has
[   38.817906] SELinux: initialized (dev sda11, type reiserfs), uses genfs_contexts

Welcome any suggestions or completions.

Thanks.