Inconsistent use of string/non_strings in mmp_struct

Inconsistent use of string/non_strings in mmp_struct

[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.

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

Re: Inconsistent use of string/non_strings in mmp_struct

[Andreas Dilger]

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

On Dec 31, 2019, at 3:07 PM, Theodore Y. Ts'o <tytso@mit.edu> wrote:
> 
> 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.

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






[-- Attachment #2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 873 bytes --]

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

[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 <adilger@dilger.ca>
---
 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(-)

diff --git a/debugfs/debugfs.c b/debugfs/debugfs.c
index 9b70145..7cfc269 100644
--- a/debugfs/debugfs.c
+++ b/debugfs/debugfs.c
@@ -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);
 }
diff --git a/e2fsck/util.c b/e2fsck/util.c
index db6a1cc..07885ab 100644
--- a/e2fsck/util.c
+++ b/e2fsck/util.c
@@ -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);
 	}
 }
 
diff --git a/lib/ext2fs/ext2_fs.h b/lib/ext2fs/ext2_fs.h
index 3165b38..79816b6 100644
--- a/lib/ext2fs/ext2_fs.h
+++ b/lib/ext2fs/ext2_fs.h
@@ -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];
diff --git a/misc/dumpe2fs.c b/misc/dumpe2fs.c
index 9a6f586..a6053d2 100644
--- a/misc/dumpe2fs.c
+++ b/misc/dumpe2fs.c
@@ -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,
diff --git a/misc/util.c b/misc/util.c
index 7799158..6239b36 100644
--- a/misc/util.c
+++ b/misc/util.c
@@ -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));
 	}
 }
diff --git a/tests/filter.sed b/tests/filter.sed
index f37986c..796186e 100644
--- a/tests/filter.sed
+++ b/tests/filter.sed
@@ -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/
-- 
1.8.0

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

[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.

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 <adilger@dilger.ca>
---
 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(-)

diff --git a/debugfs/debugfs.c b/debugfs/debugfs.c
index 7cfc269..9e22c68 100644
--- a/debugfs/debugfs.c
+++ b/debugfs/debugfs.c
@@ -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);
 }
diff --git a/e2fsck/unix.c b/e2fsck/unix.c
index b3ef0f2..b144c38 100644
--- a/e2fsck/unix.c
+++ b/e2fsck/unix.c
@@ -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);
 	}
 
diff --git a/e2fsck/util.c b/e2fsck/util.c
index 07885ab..314c989 100644
--- a/e2fsck/util.c
+++ b/e2fsck/util.c
@@ -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));
 	}
 }
 
diff --git a/lib/blkid/probe.c b/lib/blkid/probe.c
index d720144..f8687cd 100644
--- a/lib/blkid/probe.c
+++ b/lib/blkid/probe.c
@@ -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));
 
diff --git a/lib/e2p/ls.c b/lib/e2p/ls.c
index 5a44617..1521099 100644
--- a/lib/e2p/ls.c
+++ b/lib/e2p/ls.c
@@ -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",
diff --git a/lib/ext2fs/ext2_fs.h b/lib/ext2fs/ext2_fs.h
index 79816b6..6fe5cab 100644
--- a/lib/ext2fs/ext2_fs.h
+++ b/lib/ext2fs/ext2_fs.h
@@ -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
diff --git a/lib/support/plausible.c b/lib/support/plausible.c
index a726898..024f205 100644
--- a/lib/support/plausible.c
+++ b/lib/support/plausible.c
@@ -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;
diff --git a/misc/dumpe2fs.c b/misc/dumpe2fs.c
index a6053d2..5c955a9 100644
--- a/misc/dumpe2fs.c
+++ b/misc/dumpe2fs.c
@@ -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,
diff --git a/misc/e2label.c b/misc/e2label.c
index 526a62c..910ccc7 100644
--- a/misc/e2label.c
+++ b/misc/e2label.c
@@ -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);
 }
diff --git a/misc/findsuper.c b/misc/findsuper.c
index ff20b98..765295c 100644
--- a/misc/findsuper.c
+++ b/misc/findsuper.c
@@ -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);
diff --git a/misc/mke2fs.c b/misc/mke2fs.c
index ffea823..bbae131 100644
--- a/misc/mke2fs.c
+++ b/misc/mke2fs.c
@@ -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);
diff --git a/misc/tune2fs.c b/misc/tune2fs.c
index a0448f6..dd5d40a 100644
--- a/misc/tune2fs.c
+++ b/misc/tune2fs.c
@@ -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;
 	}
diff --git a/misc/util.c b/misc/util.c
index 6239b36..dcd2f0a 100644
--- a/misc/util.c
+++ b/misc/util.c
@@ -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));
 	}
 }
-- 
1.8.0

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

[Theodore Y. Ts'o]

On Tue, Jan 14, 2020 at 02:42:18PM -0700, Andreas Dilger wrote:
> 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 <adilger@dilger.ca>

Applied, thanks.

						- Ted

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

[Theodore Y. Ts'o]

On Tue, Jan 14, 2020 at 02:42:17PM -0700, Andreas Dilger wrote:
> 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 <adilger@dilger.ca>

Applied, thanks.

					- Ted