Inconsistent use of string/non_strings in mmp_struct

PATCH Applied Theodore Y. Ts'o 2 participants 6 messages Dec 31, 2019 – Jan 25, 2020

Share & tools

Theodore Y. Ts'o

author

2019-12-31 22:07 UTC #0 lore raw

While clearing some compiler in e2fsprogs, I noticed that we are a bit inconsistent about the mmp_nodename and mmp_bdevname fields, and whether they are guaranteed to be null terminated or not. The kernel is using them in some printf contexts where it's assumed they are null terminated; but in other places, we have been filling them such that if the string is exactly 64 or 32 bytes, they will *not* necessarily be null terminated.

This is potentially a problem both in the kernel as well as in e2fsprogs. I propose that we solve this problem by assuming that they *are* null terminated. But that means that if there are node names which are exactly 64 bytes long, or block device names which are exactly 32 bytes long, badness could happen.

On the other hand, we kind of have this problem already, since in the kernel, we are using memcmp when comparing mmp_nodename, but in e2fsprogs userspace, we are using gethostbyname and there is no guarantee that bytes beyond the terminating NULL have been cleared. As a result I'm not sure the interlock between e2fsprogs and the kernel works in all cases anyway.

Or we could go the other way, and try to fix all of the locations which are accessing and writing mmp_nodename and mmp_bdevname so that they are considered non-strings which are NULL padded.

Andreas, do you have any preferences here?

- Ted

Andreas Dilger

2020-01-07 23:37 UTC #1 lore raw

Quoted 22 lines · Theodore Y. Ts'o

While clearing some compiler in e2fsprogs, I noticed that we are a bit inconsistent about the mmp_nodename and mmp_bdevname fields, and whether they are guaranteed to be null terminated or not. The kernel is using them in some printf contexts where it's assumed they are null terminated; but in other places, we have been filling them such that if the string is exactly 64 or 32 bytes, they will *not* necessarily be null terminated.
This is potentially a problem both in the kernel as well as in e2fsprogs. I propose that we solve this problem by assuming that they *are* null terminated. But that means that if there are node names which are exactly 64 bytes long, or block device names which are exactly 32 bytes long, badness could happen.
On the other hand, we kind of have this problem already, since in the kernel, we are using memcmp when comparing mmp_nodename, but in e2fsprogs userspace, we are using gethostbyname and there is no guarantee that bytes beyond the terminating NULL have been cleared. As a result I'm not sure the interlock between e2fsprogs and the kernel works in all cases anyway.

Quoted 3 lines

Or we could go the other way, and try to fix all of the locations which are accessing and writing mmp_nodename and mmp_bdevname so that they are considered non-strings which are NULL padded.

Hi Ted,
it is worthwhile to note that mmp_nodename is not really a critical part of the correctness of the MMP checking, but mostly so that admins have a chance to figure out which other node is accessing the filesystem in a complex SAN environment. The one place that it is checked in the kernel with memcmp() it should only ever be "the same" as it was before the MMP thread slept for longer than it should have, so consistency between user and kernel versions or old and new versions do not really matter.

My thinking is that since some of the strings _may_ not be NUL terminated (e.g. from an earlier version of the kernel or user tools) that the safest approach is to assume there is no trailing NUL, and use a printf string format like:

        "%.*s", sizeof(mmp->mmp_nodename), mmp->mmp_nodename,

to ensure it doesn't overflow the string buffer. Fortunately, there are a lot of '\0' values immediately following those strings, so in the worst case, "mmp_nodename" may also print out "mmp_bdevname" (which is likely NUL terminated, or the (likely) 0 high byte of mmp_check_interval, or mmp_pad1, ... so there is very little risk of a very bad string access causing an access beyond the end of the buffer.

I will send patches for both.

Cheers, Andreas

Andreas Dilger

2020-01-14 21:50 UTC #2 lore raw

PATCH 1/2 mmp: don't assume NUL termination for MMP strings

In reply to Theodore Y. Ts'o

Don't assume that mmp_nodename and mmp_bdevname are NUL terminated, since very long node/device names may completely fill the buffers.

Limit string printing to the maximum buffer size for safety, and change the field definitions to __u8 to make it more clear that they are not NUL-terminated strings, as is done with other strings in the superblock that do not have NUL termination.

Signed-off-by Andreas Dilger <[email protected]>

---
 debugfs/debugfs.c    |  6 ++++--
 e2fsck/util.c        |  8 ++++++--
 lib/ext2fs/ext2_fs.h |  6 +++---
 misc/dumpe2fs.c      | 14 ++++++++++----
 misc/util.c          |  7 +++++--
 tests/filter.sed     |  1 +
 6 files changed, 29 insertions(+), 13 deletions(-)

debugfs/debugfs.c +4 -2

index 9b70145..7cfc269 100644
@@ -2446,8 +2446,10 @@ void do_dump_mmp(int argc EXT2FS_ATTR((unused)), char *argv[],
 	fprintf(stdout, "check_interval: %d\n", mmp_s->mmp_check_interval);
 	fprintf(stdout, "sequence: %08x\n", mmp_s->mmp_seq);
 	fprintf(stdout, "time: %lld -- %s", mmp_s->mmp_time, ctime(&t));
-	fprintf(stdout, "node_name: %s\n", mmp_s->mmp_nodename);
-	fprintf(stdout, "device_name: %s\n", mmp_s->mmp_bdevname);
+	fprintf(stdout, "node_name: %.*s\n",
+		(int)sizeof(mmp_s->mmp_nodename), (char *)mmp_s->mmp_nodename);
+	fprintf(stdout, "device_name: %.*s\n",
+		(int)sizeof(mmp_s->mmp_bdevname), (char *)mmp_s->mmp_bdevname);
 	fprintf(stdout, "magic: 0x%x\n", mmp_s->mmp_magic);
 	fprintf(stdout, "checksum: 0x%08x\n", mmp_s->mmp_checksum);
 }

e2fsck/util.c +6 -2

index db6a1cc..07885ab 100644
@@ -777,8 +777,12 @@ void dump_mmp_msg(struct mmp_struct *mmp, const char *fmt, ...)
 		printf("    mmp_sequence: %08x\n", mmp->mmp_seq);
 		printf("    mmp_update_date: %s", ctime(&t));
 		printf("    mmp_update_time: %lld\n", mmp->mmp_time);
-		printf("    mmp_node_name: %s\n", mmp->mmp_nodename);
-		printf("    mmp_device_name: %s\n", mmp->mmp_bdevname);
+		printf("    mmp_node_name: %.*s\n",
+		       (int)sizeof(mmp->mmp_nodename),
+		       (char *)mmp->mmp_nodename);
+		printf("    mmp_device_name: %.*s\n",
+		       (int)sizeof(mmp->mmp_bdevname),
+		       (char *)mmp->mmp_bdevname);
 	}
 }

lib/ext2fs/ext2_fs.h +3 -3

index 3165b38..79816b6 100644
@@ -1098,9 +1098,9 @@ struct ext2_dir_entry_tail {
 struct mmp_struct {
 	__u32	mmp_magic;		/* Magic number for MMP */
 	__u32	mmp_seq;		/* Sequence no. updated periodically */
-	__u64	mmp_time;		/* Time last updated */
-	char	mmp_nodename[64];	/* Node which last updated MMP block */
-	char	mmp_bdevname[32];	/* Bdev which last updated MMP block */
+	__u64	mmp_time;		/* Time last updated (seconds) */
+	__u8	mmp_nodename[64];	/* Node updating MMP block, no NUL? */
+	__u8	mmp_bdevname[32];	/* Bdev updating MMP block, no NUL? */
 	__u16	mmp_check_interval;	/* Changed mmp_check_interval */
 	__u16	mmp_pad1;
 	__u32	mmp_pad2[226];

misc/dumpe2fs.c +10 -4

index 9a6f586..a6053d2 100644
@@ -439,8 +439,12 @@ static int check_mmp(ext2_filsys fs)
 				time_t mmp_time = mmp->mmp_time;
 
 				fprintf(stderr,
-					"%s: MMP last updated by '%s' on %s",
-					program_name, mmp->mmp_nodename,
+					"%s: MMP update by '%.*s%.*s' at %s",
+					program_name,
+					(int)sizeof(mmp->mmp_nodename),
+					(char *)mmp->mmp_nodename,
+					(int)sizeof(mmp->mmp_bdevname),
+					(char *)mmp->mmp_bdevname,
 					ctime(&mmp_time));
 			}
 			retval = 1;

@@ -489,8 +493,10 @@ static void print_mmp_block(ext2_filsys fs)
 	printf("    mmp_sequence: %#08x\n", mmp->mmp_seq);
 	printf("    mmp_update_date: %s", ctime(&mmp_time));
 	printf("    mmp_update_time: %lld\n", mmp->mmp_time);
-	printf("    mmp_node_name: %s\n", mmp->mmp_nodename);
-	printf("    mmp_device_name: %s\n", mmp->mmp_bdevname);
+	printf("    mmp_node_name: %.*s\n",
+	       (int)sizeof(mmp->mmp_nodename), (char *)mmp->mmp_nodename);
+	printf("    mmp_device_name: %.*s\n",
+	       (int)sizeof(mmp->mmp_bdevname), (char *)mmp->mmp_bdevname);
 }
 
 static void parse_extended_opts(const char *opts, blk64_t *superblock,

misc/util.c +5 -2

index 7799158..6239b36 100644
@@ -288,7 +288,10 @@ void dump_mmp_msg(struct mmp_struct *mmp, const char *msg)
 	if (mmp) {
 		time_t t = mmp->mmp_time;
 
-		printf("MMP error info: last update: %s node: %s device: %s\n",
-		       ctime(&t), mmp->mmp_nodename, mmp->mmp_bdevname);
+		printf("MMP error info: node: %.*s, device: %.*s, updated: %s",
+		       (int)sizeof(mmp->mmp_nodename),
+		       (char *)mmp->mmp_nodename,
+		       (int)sizeof(mmp->mmp_bdevname),
+		       (char *)mmp->mmp_bdevname, ctime(&t));
 	}
 }

tests/filter.sed +1 -0

index f37986c..796186e 100644
@@ -37,3 +37,4 @@ s/mmp_node_name: .*/mmp_node_name: test_node/
 s/mmp_update_date: .*/mmp_update_date: test date/
 s/mmp_update_time: .*/mmp_update_time: test_time/
 s/MMP last updated by '.*' on .*/MMP last updated by 'test_node' on test date/
+s/MMP update by '.*' at .*/MMP last updated by 'test_node' on test date/

Signature · 1 line

1.8.0

Andreas Dilger

2020-01-14 21:50 UTC #3 lore raw

PATCH 2/2 mmp: abstract out repeated 'sizeof(buf), buf' usage

The printf("%.*s") format requires both the buffer size and buffer pointer to be specified for each use. Since this is repeatedly given as "(int)sizeof(buf), (char *)buf" for mmp_nodename and mmp_bdevname fields, with typecasts to avoid compiler warnings.

Add a helper macro EXT2_LEN_STR() to avoid repeated boilerplate code.

This can also be used for other superblock buffer fields that may not have NUL-terminated strings (e.g. s_volume_name, s_last_mounted, s_{first,last}_error_func, s_mount_opts) to simplify code and avoid the need for temporary buffers for NUL-termination.

Annotate the superblock string fields that may not be NUL-terminated.

Signed-off-by Andreas Dilger <[email protected]>

---
 debugfs/debugfs.c       |  4 ++--
 e2fsck/unix.c           | 17 ++++++++---------
 e2fsck/util.c           |  6 ++----
 lib/blkid/probe.c       |  2 +-
 lib/e2p/ls.c            | 39 +++++++++++++++++----------------------
 lib/ext2fs/ext2_fs.h    | 11 ++++++-----
 lib/support/plausible.c | 12 ++++--------
 misc/dumpe2fs.c         | 10 ++++------
 misc/e2label.c          |  2 +-
 misc/findsuper.c        |  5 +++--
 misc/mke2fs.c           |  6 ++----
 misc/tune2fs.c          |  3 +--
 misc/util.c             |  6 ++----
 13 files changed, 53 insertions(+), 70 deletions(-)

debugfs/debugfs.c +2 -2

index 7cfc269..9e22c68 100644
@@ -2447,9 +2447,9 @@ void do_dump_mmp(int argc EXT2FS_ATTR((unused)), char *argv[],
 	fprintf(stdout, "sequence: %08x\n", mmp_s->mmp_seq);
 	fprintf(stdout, "time: %lld -- %s", mmp_s->mmp_time, ctime(&t));
 	fprintf(stdout, "node_name: %.*s\n",
-		(int)sizeof(mmp_s->mmp_nodename), (char *)mmp_s->mmp_nodename);
+		EXT2_LEN_STR(mmp_s->mmp_nodename));
 	fprintf(stdout, "device_name: %.*s\n",
-		(int)sizeof(mmp_s->mmp_bdevname), (char *)mmp_s->mmp_bdevname);
+		EXT2_LEN_STR(mmp_s->mmp_bdevname));
 	fprintf(stdout, "magic: 0x%x\n", mmp_s->mmp_magic);
 	fprintf(stdout, "checksum: 0x%08x\n", mmp_s->mmp_checksum);
 }

e2fsck/unix.c +8 -9

index b3ef0f2..b144c38 100644
@@ -1697,11 +1697,10 @@ failure:
 	 * Set the device name, which is used whenever we print error
 	 * or informational messages to the user.
 	 */
-	if (ctx->device_name == 0 &&
-	    (sb->s_volume_name[0] != 0)) {
+	if (ctx->device_name == 0 && sb->s_volume_name[0])
 		ctx->device_name = string_copy(ctx, sb->s_volume_name,
 					       sizeof(sb->s_volume_name));
-	}
+
 	if (ctx->device_name == 0)
 		ctx->device_name = string_copy(ctx, ctx->filesystem_name, 0);
 	for (cp = ctx->device_name; *cp; cp++)

@@ -1709,19 +1708,19 @@ failure:
 			*cp = '_';
 
 	if (ctx->problem_logf) {
-		char buf[48];
 
 		fprintf(ctx->problem_logf, "<filesystem dev=\"%s\"",
 			ctx->filesystem_name);
 		if (!uuid_is_null(sb->s_uuid)) {
+			char buf[48];
+
 			uuid_unparse(sb->s_uuid, buf);
 			fprintf(ctx->problem_logf, " uuid=\"%s\"", buf);
 		}
-		if (sb->s_volume_name[0]) {
-			memset(buf, 0, sizeof(buf));
-			strncpy(buf, sb->s_volume_name, sizeof(buf));
-			fprintf(ctx->problem_logf, " label=\"%s\"", buf);
-		}
+		if (sb->s_volume_name[0])
+			fprintf(ctx->problem_logf, " label=\"%.*s\"",
+				EXT2_LEN_STR(sb->s_volume_name));
+
 		fputs("/>\n", ctx->problem_logf);
 	}

e2fsck/util.c +2 -4

index 07885ab..314c989 100644
@@ -778,11 +778,9 @@ void dump_mmp_msg(struct mmp_struct *mmp, const char *fmt, ...)
 		printf("    mmp_update_date: %s", ctime(&t));
 		printf("    mmp_update_time: %lld\n", mmp->mmp_time);
 		printf("    mmp_node_name: %.*s\n",
-		       (int)sizeof(mmp->mmp_nodename),
-		       (char *)mmp->mmp_nodename);
+		       EXT2_LEN_STR(mmp->mmp_nodename));
 		printf("    mmp_device_name: %.*s\n",
-		       (int)sizeof(mmp->mmp_bdevname),
-		       (char *)mmp->mmp_bdevname);
+		       EXT2_LEN_STR(mmp->mmp_bdevname));
 	}
 }

lib/blkid/probe.c +1 -1

index d720144..f8687cd 100644
@@ -144,7 +144,7 @@ static void get_ext2_info(blkid_dev dev, struct blkid_magic *id,
 		   blkid_le32(es->s_feature_incompat),
 		   blkid_le32(es->s_feature_ro_compat)));
 
-	if (strlen(es->s_volume_name))
+	if (es->s_volume_name[0])
 		label = es->s_volume_name;
 	blkid_set_tag(dev, "LABEL", label, sizeof(es->s_volume_name));

lib/e2p/ls.c +17 ~~-22~~

index 5a44617..1521099 100644
@@ -224,7 +224,7 @@ static const char *quota_type2prefix(enum quota_type qtype)
 void list_super2(struct ext2_super_block * sb, FILE *f)
 {
 	int inode_blocks_per_group;
-	char buf[80], *str;
+	char *str;
 	time_t	tm;
 	enum quota_type qtype;

@@ -232,18 +232,16 @@ void list_super2(struct ext2_super_block * sb, FILE *f)
 				    EXT2_INODE_SIZE(sb)) +
 				   EXT2_BLOCK_SIZE(sb) - 1) /
 				  EXT2_BLOCK_SIZE(sb));
-	if (sb->s_volume_name[0]) {
-		memset(buf, 0, sizeof(buf));
-		strncpy(buf, sb->s_volume_name, sizeof(sb->s_volume_name));
-	} else
-		strcpy(buf, "<none>");
-	fprintf(f, "Filesystem volume name:   %s\n", buf);
-	if (sb->s_last_mounted[0]) {
-		memset(buf, 0, sizeof(buf));
-		strncpy(buf, sb->s_last_mounted, sizeof(sb->s_last_mounted));
-	} else
-		strcpy(buf, "<not available>");
-	fprintf(f, "Last mounted on:          %s\n", buf);
+	if (sb->s_volume_name[0])
+		fprintf(f, "Filesystem volume name:   %.*s\n",
+			EXT2_LEN_STR(sb->s_volume_name));
+	else
+		fprintf(f, "Filesystem volume name:   <none>\n");
+	if (sb->s_last_mounted[0])
+		fprintf(f, "Last mounted on:          %.*s\n",
+			EXT2_LEN_STR(sb->s_last_mounted));
+	else
+		fprintf(f, "Last mounted on:          <not available>\n");
 	fprintf(f, "Filesystem UUID:          %s\n", e2p_uuid2str(sb->s_uuid));
 	fprintf(f, "Filesystem magic number:  0x%04X\n", sb->s_magic);
 	fprintf(f, "Filesystem revision #:    %d", sb->s_rev_level);

@@ -259,7 +257,8 @@ void list_super2(struct ext2_super_block * sb, FILE *f)
 	print_super_flags(sb, f);
 	print_mntopts(sb, f);
 	if (sb->s_mount_opts[0])
-		fprintf(f, "Mount options:            %s\n", sb->s_mount_opts);
+		fprintf(f, "Mount options:            %.*s\n",
+			EXT2_LEN_STR(sb->s_mount_opts));
 	fprintf(f, "Filesystem state:        ");
 	print_fs_state (f, sb->s_state);
 	fprintf(f, "\n");

@@ -419,10 +418,8 @@ void list_super2(struct ext2_super_block * sb, FILE *f)
 	if (sb->s_first_error_time) {
 		tm = sb->s_first_error_time;
 		fprintf(f, "First error time:         %s", ctime(&tm));
-		memset(buf, 0, sizeof(buf));
-		strncpy(buf, (char *)sb->s_first_error_func,
-			sizeof(sb->s_first_error_func));
-		fprintf(f, "First error function:     %s\n", buf);
+		fprintf(f, "First error function:     %.*s\n",
+			EXT2_LEN_STR(sb->s_first_error_func));
 		fprintf(f, "First error line #:       %u\n",
 			sb->s_first_error_line);
 		fprintf(f, "First error inode #:      %u\n",

@@ -433,10 +430,8 @@ void list_super2(struct ext2_super_block * sb, FILE *f)
 	if (sb->s_last_error_time) {
 		tm = sb->s_last_error_time;
 		fprintf(f, "Last error time:          %s", ctime(&tm));
-		memset(buf, 0, sizeof(buf));
-		strncpy(buf, (char *)sb->s_last_error_func,
-			sizeof(sb->s_last_error_func));
-		fprintf(f, "Last error function:      %s\n", buf);
+		fprintf(f, "Last error function:      %.*s\n",
+			EXT2_LEN_STR(sb->s_last_error_func));
 		fprintf(f, "Last error line #:        %u\n",
 			sb->s_last_error_line);
 		fprintf(f, "Last error inode #:       %u\n",

lib/ext2fs/ext2_fs.h +6 -5

index 79816b6..6fe5cab 100644
@@ -682,8 +682,8 @@ struct ext2_super_block {
 /*060*/	__u32	s_feature_incompat;	/* incompatible feature set */
 	__u32	s_feature_ro_compat;	/* readonly-compatible feature set */
 /*068*/	__u8	s_uuid[16];		/* 128-bit uuid for volume */
-/*078*/	char	s_volume_name[EXT2_LABEL_LEN];	/* volume name */
-/*088*/	char	s_last_mounted[64];	/* directory where last mounted */
+/*078*/	__u8	s_volume_name[EXT2_LABEL_LEN];	/* volume name, no NUL? */
+/*088*/	__u8	s_last_mounted[64];	/* directory last mounted on, no NUL? */
 /*0c8*/	__u32	s_algorithm_usage_bitmap; /* For compression */
 	/*
 	 * Performance hints.  Directory preallocation should only

@@ -731,15 +731,15 @@ struct ext2_super_block {
 	__u32	s_first_error_time;	/* first time an error happened */
 	__u32	s_first_error_ino;	/* inode involved in first error */
 /*1a0*/	__u64	s_first_error_block;	/* block involved in first error */
-	__u8	s_first_error_func[32];	/* function where the error happened */
+	__u8	s_first_error_func[32];	/* function where error hit, no NUL? */
 /*1c8*/	__u32	s_first_error_line;	/* line number where error happened */
 	__u32	s_last_error_time;	/* most recent time of an error */
 /*1d0*/	__u32	s_last_error_ino;	/* inode involved in last error */
 	__u32	s_last_error_line;	/* line number where error happened */
 	__u64	s_last_error_block;	/* block involved of last error */
-/*1e0*/	__u8	s_last_error_func[32];	/* function where the error happened */
+/*1e0*/	__u8	s_last_error_func[32];	/* function where error hit, no NUL? */
 #define EXT4_S_ERR_END ext4_offsetof(struct ext2_super_block, s_mount_opts)
-/*200*/	__u8	s_mount_opts[64];
+/*200*/	__u8	s_mount_opts[64];	/* default mount options, no NUL? */
 /*240*/	__u32	s_usr_quota_inum;	/* inode number of user quota file */
 	__u32	s_grp_quota_inum;	/* inode number of group quota file */
 	__u32	s_overhead_blocks;	/* overhead blocks/clusters in fs */

@@ -763,6 +763,7 @@ struct ext2_super_block {
 };
 
 #define EXT4_S_ERR_LEN (EXT4_S_ERR_END - EXT4_S_ERR_START)
+#define EXT2_LEN_STR(buf) (int)sizeof(buf), (char *)buf
 
 /*
  * Codes for operating systems

lib/support/plausible.c +4 -8

index a726898..024f205 100644
@@ -101,7 +101,6 @@ static void print_ext2_info(const char *device)
 	ext2_filsys		fs;
 	errcode_t		retval;
 	time_t			tm;
-	char			buf[80];
 
 	retval = ext2fs_open2(device, 0, EXT2_FLAG_64BITS, 0, 0,
 			      unix_io_manager, &fs);

@@ -111,13 +110,10 @@ static void print_ext2_info(const char *device)
 
 	if (sb->s_mtime) {
 		tm = sb->s_mtime;
-		if (sb->s_last_mounted[0]) {
-			memset(buf, 0, sizeof(buf));
-			strncpy(buf, sb->s_last_mounted,
-				sizeof(sb->s_last_mounted));
-			printf(_("\tlast mounted on %s on %s"), buf,
-			       ctime(&tm));
-		} else
+		if (sb->s_last_mounted[0])
+			printf(_("\tlast mounted on %.*s on %s"),
+			       EXT2_LEN_STR(sb->s_last_mounted), ctime(&tm));
+		else
 			printf(_("\tlast mounted on %s"), ctime(&tm));
 	} else if (sb->s_mkfs_time) {
 		tm = sb->s_mkfs_time;

misc/dumpe2fs.c +4 -6

index a6053d2..5c955a9 100644
@@ -441,10 +441,8 @@ static int check_mmp(ext2_filsys fs)
 				fprintf(stderr,
 					"%s: MMP update by '%.*s%.*s' at %s",
 					program_name,
-					(int)sizeof(mmp->mmp_nodename),
-					(char *)mmp->mmp_nodename,
-					(int)sizeof(mmp->mmp_bdevname),
-					(char *)mmp->mmp_bdevname,
+					EXT2_LEN_STR(mmp->mmp_nodename),
+					EXT2_LEN_STR(mmp->mmp_bdevname),
 					ctime(&mmp_time));
 			}
 			retval = 1;

@@ -494,9 +492,9 @@ static void print_mmp_block(ext2_filsys fs)
 	printf("    mmp_update_date: %s", ctime(&mmp_time));
 	printf("    mmp_update_time: %lld\n", mmp->mmp_time);
 	printf("    mmp_node_name: %.*s\n",
-	       (int)sizeof(mmp->mmp_nodename), (char *)mmp->mmp_nodename);
+	       EXT2_LEN_STR(mmp->mmp_nodename));
 	printf("    mmp_device_name: %.*s\n",
-	       (int)sizeof(mmp->mmp_bdevname), (char *)mmp->mmp_bdevname);
+	       EXT2_LEN_STR(mmp->mmp_bdevname));
 }
 
 static void parse_extended_opts(const char *opts, blk64_t *superblock,

misc/e2label.c +1 -1

index 526a62c..910ccc7 100644
@@ -81,7 +81,7 @@ static void print_label (char *dev)
 	char label[VOLNAMSZ+1];
 
 	open_e2fs (dev, O_RDONLY);
-	strncpy(label, sb.s_volume_name, VOLNAMSZ);
+	snprintf(label, sizeof(label), "%.*s", EXT2_LEN_STR(sb.s_volume_name));
 	label[VOLNAMSZ] = 0;
 	printf("%s\n", label);
 }

misc/findsuper.c +3 -2

index ff20b98..765295c 100644
@@ -251,7 +251,7 @@ int main(int argc, char *argv[])
 			sb_offset = 0;
 		if (jnl_copy && !print_jnl_copies)
 			continue;
-		printf("\r%11Lu %11Lu%s %11Lu%s %9u %5Lu %4u%s %s %02x%02x%02x%02x %s\n",
+		printf("\r%11Lu %11Lu%s %11Lu%s %9u %5Lu %4u%s %s %02x%02x%02x%02x %.*s\n",
 		       sk, sk - ext2.s_block_group_nr * grpsize - sb_offset,
 		       jnl_copy ? "*":" ",
 		       sk + ext2fs_blocks_count(&ext2) * bsize -

@@ -259,7 +259,8 @@ int main(int argc, char *argv[])
 		       jnl_copy ? "*" : " ", ext2fs_blocks_count(&ext2), bsize,
 		       ext2.s_block_group_nr, jnl_copy ? "*" : " ", s,
 		       ext2.s_uuid[0], ext2.s_uuid[1],
-		       ext2.s_uuid[2], ext2.s_uuid[3], ext2.s_volume_name);
+		       ext2.s_uuid[2], ext2.s_uuid[3],
+		       EXT2_LEN_STR(ext2.s_volume_name));
 	}
 	printf(_("\n%11Lu: finished with errno %d\n"), sk, errno);
 	close(fd);

misc/mke2fs.c +2 -4

index ffea823..bbae131 100644
@@ -655,7 +655,6 @@ write_superblock:
 static void show_stats(ext2_filsys fs)
 {
 	struct ext2_super_block *s = fs->super;
-	char 			buf[80];
         char                    *os;
 	blk64_t			group_block;
 	dgrp_t			i;

@@ -673,9 +672,8 @@ static void show_stats(ext2_filsys fs)
 		fprintf(stderr, _("warning: %llu blocks unused.\n\n"),
 		       ext2fs_blocks_count(&fs_param) - ext2fs_blocks_count(s));
 
-	memset(buf, 0, sizeof(buf));
-	strncpy(buf, s->s_volume_name, sizeof(s->s_volume_name));
-	printf(_("Filesystem label=%s\n"), buf);
+	printf(_("Filesystem label=%.*s\n"), EXT2_LEN_STR(s->s_volume_name));
+
 	os = e2p_os2string(fs->super->s_creator_os);
 	if (os)
 		printf(_("OS type: %s\n"), os);

misc/tune2fs.c +1 -2

index a0448f6..dd5d40a 100644
@@ -2990,8 +2990,7 @@ retry_open:
 
 	if (print_label) {
 		/* For e2label emulation */
-		printf("%.*s\n", (int) sizeof(sb->s_volume_name),
-		       sb->s_volume_name);
+		printf("%.*s\n", EXT2_LEN_STR(sb->s_volume_name));
 		remove_error_table(&et_ext2_error_table);
 		goto closefs;
 	}

misc/util.c +2 -4

index 6239b36..dcd2f0a 100644
@@ -289,9 +289,7 @@ void dump_mmp_msg(struct mmp_struct *mmp, const char *msg)
 		time_t t = mmp->mmp_time;
 
 		printf("MMP error info: node: %.*s, device: %.*s, updated: %s",
-		       (int)sizeof(mmp->mmp_nodename),
-		       (char *)mmp->mmp_nodename,
-		       (int)sizeof(mmp->mmp_bdevname),
-		       (char *)mmp->mmp_bdevname, ctime(&t));
+		       EXT2_LEN_STR(mmp->mmp_nodename),
+		       EXT2_LEN_STR(mmp->mmp_bdevname), ctime(&t));
 	}
 }

Signature · 1 line

1.8.0

Theodore Y. Ts'o

author Applied

2020-01-25 06:44 UTC #4 lore raw

Re: [PATCH 2/2] mmp: abstract out repeated 'sizeof(buf), buf' usage

Quoted 7 lines · Andreas Dilger

The printf("%.*s") format requires both the buffer size and buffer pointer to be specified for each use. Since this is repeatedly given as "(int)sizeof(buf), (char *)buf" for mmp_nodename and mmp_bdevname fields, with typecasts to avoid compiler warnings.
Add a helper macro EXT2_LEN_STR() to avoid repeated boilerplate code.

Quoted 8 lines

This can also be used for other superblock buffer fields that may not have NUL-terminated strings (e.g. s_volume_name, s_last_mounted, s_{first,last}_error_func, s_mount_opts) to simplify code and avoid the need for temporary buffers for NUL-termination.
Annotate the superblock string fields that may not be NUL-terminated.
Signed-off-by Andreas Dilger <[email protected]>

Applied, thanks.

- Ted

Theodore Y. Ts'o

author Applied

2020-01-25 06:43 UTC #5 lore raw

Re: [PATCH 1/2] mmp: don't assume NUL termination for MMP strings

In reply to Andreas Dilger

Quoted 9 lines · Andreas Dilger

Don't assume that mmp_nodename and mmp_bdevname are NUL terminated, since very long node/device names may completely fill the buffers.
Limit string printing to the maximum buffer size for safety, and change the field definitions to __u8 to make it more clear that they are not NUL-terminated strings, as is done with other strings in the superblock that do not have NUL termination.
Signed-off-by Andreas Dilger <[email protected]>

Applied, thanks.

- Ted