[PATCH 0/8] mtd-utils: fixes for various issues reported by static analysis

[PATCH 0/8] mtd-utils: fixes for various issues reported by static analysis

[David Oberhollenzer]

This is the next round of patches for mtd-utils that attempt to fix
some of the issues reported by Coverity Scan and gcc warnings.

For this round I mostly tried to getthe obvious stuff out ouf the way
before touching some code where it is less obvious what it is actually
supposed to do. Nevertheless, please tell me if my fixes break something
or my assumptions about the intended behaviour are flawed. Otherwise I
will merge this at the end of the week.

Thanks,

David



______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 1/8] mtd-utils: Fix printf format specifies with the wrong type

[David Oberhollenzer]

Signed-off-by: David Oberhollenzer <david.oberhollenzer@sigma-star.at>
---
 misc-utils/flashcp.c        | 8 ++++----
 tests/ubi-tests/io_paral.c  | 4 ++--
 tests/ubi-tests/io_read.c   | 2 +-
 tests/ubi-tests/io_update.c | 4 ++--
 4 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/misc-utils/flashcp.c b/misc-utils/flashcp.c
index b6ad2f9..1270422 100644
--- a/misc-utils/flashcp.c
+++ b/misc-utils/flashcp.c
@@ -337,12 +337,12 @@ int main (int argc,char *argv[])
 			if (result < 0)
 			{
 				log_printf (LOG_ERROR,
-						"While writing data to 0x%.8x-0x%.8x on %s: %m\n",
+						"While writing data to 0x%.8lx-0x%.8lx on %s: %m\n",
 						written,written + i,device);
 				exit (EXIT_FAILURE);
 			}
 			log_printf (LOG_ERROR,
-					"Short write count returned while writing to x%.8x-0x%.8x on %s: %d/%llu bytes written to flash\n",
+					"Short write count returned while writing to x%.8lx-0x%.8lx on %s: %lu/%llu bytes written to flash\n",
 					written,written + i,device,written + result,(unsigned long long)filestat.st_size);
 			exit (EXIT_FAILURE);
 		}
@@ -372,7 +372,7 @@ int main (int argc,char *argv[])
 		if (size < BUFSIZE) i = size;
 		if (flags & FLAG_VERBOSE)
 			log_printf (LOG_NORMAL,
-					"\rVerifying data: %dk/%lluk (%llu%%)",
+					"\rVerifying data: %luk/%lluk (%llu%%)",
 					KB (written + i),
 					KB ((unsigned long long)filestat.st_size),
 					PERCENTAGE (written + i,(unsigned long long)filestat.st_size));
@@ -387,7 +387,7 @@ int main (int argc,char *argv[])
 		if (memcmp (src,dest,i))
 		{
 			log_printf (LOG_ERROR,
-					"File does not seem to match flash data. First mismatch at 0x%.8x-0x%.8x\n",
+					"File does not seem to match flash data. First mismatch at 0x%.8lx-0x%.8lx\n",
 					written,written + i);
 			exit (EXIT_FAILURE);
 		}
diff --git a/tests/ubi-tests/io_paral.c b/tests/ubi-tests/io_paral.c
index 4040b3e..b0884fe 100644
--- a/tests/ubi-tests/io_paral.c
+++ b/tests/ubi-tests/io_paral.c
@@ -207,7 +207,7 @@ static void *write_thread(void *ptr)
 		ret = pwrite(fd, wbuf, dev_info.leb_size, offs);
 		if (ret != dev_info.leb_size) {
 			failed("pwrite");
-			errorm("cannot write %d bytes to offs %lld, wrote %d",
+			errorm("cannot write %d bytes to offs %ld, wrote %d",
 				dev_info.leb_size, offs, ret);
 			break;
 		}
@@ -216,7 +216,7 @@ static void *write_thread(void *ptr)
 		ret = pread(fd, rbuf, dev_info.leb_size, offs);
 		if (ret != dev_info.leb_size) {
 			failed("read");
-			errorm("failed to read %d bytes at offset %d "
+			errorm("failed to read %d bytes at offset %ld "
 			       "of volume %d", dev_info.leb_size, offs,
 			       vol_id);
 			break;
diff --git a/tests/ubi-tests/io_read.c b/tests/ubi-tests/io_read.c
index f944a86..a6cc8f5 100644
--- a/tests/ubi-tests/io_read.c
+++ b/tests/ubi-tests/io_read.c
@@ -233,7 +233,7 @@ static int test_read2(const struct ubi_vol_info *vol_info, int len)
 			continue;
 
 		if (test_read3(vol_info, len, offsets[i])) {
-			errorm("offset = %d", offsets[i]);
+			errorm("offset = %ld", offsets[i]);
 			return -1;
 		}
 	}
diff --git a/tests/ubi-tests/io_update.c b/tests/ubi-tests/io_update.c
index f48df1d..d093da5 100644
--- a/tests/ubi-tests/io_update.c
+++ b/tests/ubi-tests/io_update.c
@@ -189,11 +189,11 @@ static int test_update1(struct ubi_vol_info *vol_info, int leb_change)
 			ret = read(fd, buf1, test_len);
 		if (ret < 0) {
 			failed("read");
-			errorm("failed to read %d bytes", test_len);
+			errorm("failed to read %lld bytes", test_len);
 			goto close;
 		}
 		if (ret != test_len) {
-			errorm("failed to read %d bytes, read %d", test_len, ret);
+			errorm("failed to read %lld bytes, read %d", test_len, ret);
 			goto close;
 		}
 		if (memcmp(buf, buf1, test_len)) {
-- 
2.24.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/8] mtd-utils: Fix printf format specifies with the wrong type

[Alexander Dahl]

Hello David,

Am Dienstag, 28. Januar 2020, 18:27:08 CET schrieb David Oberhollenzer:
> Signed-off-by: David Oberhollenzer <david.oberhollenzer@sigma-star.at>
> ---
>  misc-utils/flashcp.c        | 8 ++++----
>  tests/ubi-tests/io_paral.c  | 4 ++--
>  tests/ubi-tests/io_read.c   | 2 +-
>  tests/ubi-tests/io_update.c | 4 ++--
>  4 files changed, 9 insertions(+), 9 deletions(-)
> 
> diff --git a/misc-utils/flashcp.c b/misc-utils/flashcp.c
> index b6ad2f9..1270422 100644
> --- a/misc-utils/flashcp.c
> +++ b/misc-utils/flashcp.c
> @@ -337,12 +337,12 @@ int main (int argc,char *argv[])
>  			if (result < 0)
>  			{
>  				log_printf (LOG_ERROR,
> -						"While writing data to 0x%.8x-0x%.8x on %s: %m\n",
> +						"While writing data to 0x%.8lx-0x%.8lx on %s: %m\n",
>  						written,written + i,device);
>  				exit (EXIT_FAILURE);
>  			}
>  			log_printf (LOG_ERROR,
> -					"Short write count returned while writing to x%.8x-0x%.8x on 
%s:
> %d/%llu bytes written to flash\n", +					"Short write count 
returned while
> writing to x%.8lx-0x%.8lx on %s: %lu/%llu bytes written to flash\n",
> written,written + i,device,written + result,(unsigned long
> long)filestat.st_size); exit (EXIT_FAILURE);
>  		}

The correct length specifier for variables of type size_t would be 'z' so the 
format specifier would be %zx or %zu. The 'l' for long might work on one 
platform but lead to a warning on another. We face this regularly when 
compiling the same source for 64 bit x86 and 32 bit arm.

> @@ -372,7 +372,7 @@ int main (int argc,char *argv[])
>  		if (size < BUFSIZE) i = size;
>  		if (flags & FLAG_VERBOSE)
>  			log_printf (LOG_NORMAL,
> -					"\rVerifying data: %dk/%lluk (%llu%%)",
> +					"\rVerifying data: %luk/%lluk (%llu%%)",
>  					KB (written + i),
>  					KB ((unsigned long long)filestat.st_size),
>  					PERCENTAGE (written + i,(unsigned long long)filestat.st_size));
> @@ -387,7 +387,7 @@ int main (int argc,char *argv[])
>  		if (memcmp (src,dest,i))
>  		{
>  			log_printf (LOG_ERROR,
> -					"File does not seem to match flash data. First mismatch at
> 0x%.8x-0x%.8x\n", +					"File does not seem to match flash data. 
First
> mismatch at 0x%.8lx-0x%.8lx\n", written,written + i);
>  			exit (EXIT_FAILURE);
>  		}

Same with that ones.

I'm not sure of those 'written + i' though, might be different due to integer 
promotion.

Greets
Alex

> diff --git a/tests/ubi-tests/io_paral.c b/tests/ubi-tests/io_paral.c
> index 4040b3e..b0884fe 100644
> --- a/tests/ubi-tests/io_paral.c
> +++ b/tests/ubi-tests/io_paral.c
> @@ -207,7 +207,7 @@ static void *write_thread(void *ptr)
>  		ret = pwrite(fd, wbuf, dev_info.leb_size, offs);
>  		if (ret != dev_info.leb_size) {
>  			failed("pwrite");
> -			errorm("cannot write %d bytes to offs %lld, wrote %d",
> +			errorm("cannot write %d bytes to offs %ld, wrote %d",
>  				dev_info.leb_size, offs, ret);
>  			break;
>  		}
> @@ -216,7 +216,7 @@ static void *write_thread(void *ptr)
>  		ret = pread(fd, rbuf, dev_info.leb_size, offs);
>  		if (ret != dev_info.leb_size) {
>  			failed("read");
> -			errorm("failed to read %d bytes at offset %d "
> +			errorm("failed to read %d bytes at offset %ld "
>  			       "of volume %d", dev_info.leb_size, offs,
>  			       vol_id);
>  			break;
> diff --git a/tests/ubi-tests/io_read.c b/tests/ubi-tests/io_read.c
> index f944a86..a6cc8f5 100644
> --- a/tests/ubi-tests/io_read.c
> +++ b/tests/ubi-tests/io_read.c
> @@ -233,7 +233,7 @@ static int test_read2(const struct ubi_vol_info
> *vol_info, int len) continue;
> 
>  		if (test_read3(vol_info, len, offsets[i])) {
> -			errorm("offset = %d", offsets[i]);
> +			errorm("offset = %ld", offsets[i]);
>  			return -1;
>  		}
>  	}
> diff --git a/tests/ubi-tests/io_update.c b/tests/ubi-tests/io_update.c
> index f48df1d..d093da5 100644
> --- a/tests/ubi-tests/io_update.c
> +++ b/tests/ubi-tests/io_update.c
> @@ -189,11 +189,11 @@ static int test_update1(struct ubi_vol_info *vol_info,
> int leb_change) ret = read(fd, buf1, test_len);
>  		if (ret < 0) {
>  			failed("read");
> -			errorm("failed to read %d bytes", test_len);
> +			errorm("failed to read %lld bytes", test_len);
>  			goto close;
>  		}
>  		if (ret != test_len) {
> -			errorm("failed to read %d bytes, read %d", test_len, ret);
> +			errorm("failed to read %lld bytes, read %d", test_len, ret);
>  			goto close;
>  		}
>  		if (memcmp(buf, buf1, test_len)) {



______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

Re: [PATCH 1/8] mtd-utils: Fix printf format specifies with the wrong type

[David Oberhollenzer]

On 1/29/20 8:36 AM, Alexander Dahl wrote:
> The correct length specifier for variables of type size_t would be 'z' so the 
> format specifier would be %zx or %zu. The 'l' for long might work on one 
> platform but lead to a warning on another. We face this regularly when 
> compiling the same source for 64 bit x86 and 32 bit arm.
> 
...
> Same with that ones.

Thanks! I tried to quickly check the actual types by doing a quick backwards
search but somehow must have missed that, probably relying to much on the
gcc & scan warnings agreeing with each other. Obviously the warning would
say "unsigned long" on x86_64 because it's typedef'd to that on x86_64. I'll
make sure to also double check again on a 32 bit VM.

Thanks,

David

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 2/8] mtd-utils: Fix potential negative arguments passed to close(2)

[David Oberhollenzer]

Many tools open a file descriptor, close it a the end and have some
form of error path in between that jumps to the end.

In some cases, if opening the file fails the error path is taken and
the utility ends up closing one or more invalid file descriptors. It's
technically not a real issue but something that pretty much any static
analysis tool barks at.

Signed-off-by: David Oberhollenzer <david.oberhollenzer@sigma-star.at>
---
 nand-utils/nanddump.c                  | 3 ++-
 nand-utils/nandwrite.c                 | 3 ++-
 nor-utils/rfddump.c                    | 2 +-
 tests/fs-tests/stress/atoms/fwrite00.c | 4 +++-
 4 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/nand-utils/nanddump.c b/nand-utils/nanddump.c
index 841ed67..62699e0 100644
--- a/nand-utils/nanddump.c
+++ b/nand-utils/nanddump.c
@@ -549,7 +549,8 @@ int main(int argc, char * const argv[])
 
 closeall:
 	close(fd);
-	close(ofd);
+	if (ofd > 0 && ofd != STDOUT_FILENO)
+		close(ofd);
 	free(oobbuf);
 	free(readbuf);
 	exit(EXIT_FAILURE);
diff --git a/nand-utils/nandwrite.c b/nand-utils/nandwrite.c
index 8f21593..e8a210c 100644
--- a/nand-utils/nandwrite.c
+++ b/nand-utils/nandwrite.c
@@ -605,7 +605,8 @@ int main(int argc, char * const argv[])
 	failed = false;
 
 closeall:
-	close(ifd);
+	if (ifd > 0 && ifd != STDIN_FILENO)
+		close(ifd);
 	libmtd_close(mtd_desc);
 	free(filebuf);
 	close(fd);
diff --git a/nor-utils/rfddump.c b/nor-utils/rfddump.c
index 4ad2f91..01ab4c2 100644
--- a/nor-utils/rfddump.c
+++ b/nor-utils/rfddump.c
@@ -324,7 +324,7 @@ int main(int argc, char *argv[])
 	return 0;
 
 err:
-	if (out_fd)
+	if (out_fd > 0)
 		close(out_fd);
 
 	close(fd);
diff --git a/tests/fs-tests/stress/atoms/fwrite00.c b/tests/fs-tests/stress/atoms/fwrite00.c
index 3406bba..877c63c 100644
--- a/tests/fs-tests/stress/atoms/fwrite00.c
+++ b/tests/fs-tests/stress/atoms/fwrite00.c
@@ -138,7 +138,9 @@ static void filestress00(void)
 			deleted = 1;
 		}
 	}
-	CHECK(close(fd) != -1);
+	if (fd > 0) {
+		CHECK(close(fd) != -1);
+	}
 	/* Sleep */
 	if (tests_sleep_parameter > 0) {
 		unsigned us = tests_sleep_parameter * 1000;
-- 
2.24.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 3/8] mtd-utils: Fix various TOCTOU issues

[David Oberhollenzer]

This patch restructures various code parts that follow the pattern
of "stat(x, &sb) ... makes_sense(&sb) ... open(x)".

Signed-off-by: David Oberhollenzer <david.oberhollenzer@sigma-star.at>
---
 jffsX-utils/mkfs.jffs2.c |  4 +---
 lib/libmtd_legacy.c      | 31 ++++++++++++++++++++-----------
 misc-utils/ftl_check.c   | 14 ++++++++------
 misc-utils/ftl_format.c  | 14 ++++++++------
 4 files changed, 37 insertions(+), 26 deletions(-)

diff --git a/jffsX-utils/mkfs.jffs2.c b/jffsX-utils/mkfs.jffs2.c
index f46cc22..9cc5eaf 100644
--- a/jffsX-utils/mkfs.jffs2.c
+++ b/jffsX-utils/mkfs.jffs2.c
@@ -1772,9 +1772,7 @@ int main(int argc, char **argv)
 		}
 		out_fd = 1;
 	}
-	if (lstat(rootdir, &sb)) {
-		sys_errmsg_die("%s", rootdir);
-	}
+
 	if (chdir(rootdir))
 		sys_errmsg_die("%s", rootdir);
 
diff --git a/lib/libmtd_legacy.c b/lib/libmtd_legacy.c
index 2b7f65f..4eb4a70 100644
--- a/lib/libmtd_legacy.c
+++ b/lib/libmtd_legacy.c
@@ -221,18 +221,21 @@ int legacy_get_mtd_oobavail(const char *node)
 	struct nand_ecclayout_user usrlay;
 	int fd, ret;
 
-	if (stat(node, &st))
+	fd = open(node, O_RDONLY);
+	if (fd == -1)
 		return sys_errmsg("cannot open \"%s\"", node);
 
+	if (fstat(fd, &st)) {
+		ret = sys_errmsg("cannot open \"%s\"", node);
+		goto out_close;
+	}
+
 	if (!S_ISCHR(st.st_mode)) {
 		errno = EINVAL;
-		return errmsg("\"%s\" is not a character device", node);
+		ret = errmsg("\"%s\" is not a character device", node);
+		goto out_close;
 	}
 
-	fd = open(node, O_RDONLY);
-	if (fd == -1)
-		return sys_errmsg("cannot open \"%s\"", node);
-
 	ret = ioctl(fd, ECCGETLAYOUT, &usrlay);
 	if (ret < 0) {
 		if (errno == EOPNOTSUPP)
@@ -273,15 +276,24 @@ int legacy_get_dev_info(const char *node, struct mtd_dev_info *mtd)
 	loff_t offs = 0;
 	struct proc_parse_info pi;
 
-	if (stat(node, &st)) {
+	fd = open(node, O_RDONLY);
+	if (fd == -1) {
 		sys_errmsg("cannot open \"%s\"", node);
 		if (errno == ENOENT)
 			normsg("MTD subsystem is old and does not support "
 			       "sysfs, so MTD character device nodes have "
 			       "to exist");
+		return -1;
+	}
+
+	if (fstat(fd, &st)) {
+		sys_errmsg("cannot stat \"%s\"", node);
+		close(fd);
+		return -1;
 	}
 
 	if (!S_ISCHR(st.st_mode)) {
+		close(fd);
 		errno = EINVAL;
 		return errmsg("\"%s\" is not a character device", node);
 	}
@@ -291,6 +303,7 @@ int legacy_get_dev_info(const char *node, struct mtd_dev_info *mtd)
 	mtd->minor = minor(st.st_rdev);
 
 	if (mtd->major != MTD_DEV_MAJOR) {
+		close(fd);
 		errno = EINVAL;
 		return errmsg("\"%s\" has major number %d, MTD devices have "
 			      "major %d", node, mtd->major, MTD_DEV_MAJOR);
@@ -298,10 +311,6 @@ int legacy_get_dev_info(const char *node, struct mtd_dev_info *mtd)
 
 	mtd->mtd_num = mtd->minor / 2;
 
-	fd = open(node, O_RDONLY);
-	if (fd == -1)
-		return sys_errmsg("cannot open \"%s\"", node);
-
 	if (ioctl(fd, MEMGETINFO, &ui)) {
 		sys_errmsg("MEMGETINFO ioctl request failed");
 		goto out_close;
diff --git a/misc-utils/ftl_check.c b/misc-utils/ftl_check.c
index 5a04155..5b2dae5 100644
--- a/misc-utils/ftl_check.c
+++ b/misc-utils/ftl_check.c
@@ -206,18 +206,20 @@ int main(int argc, char *argv[])
 		exit(errflg > 0 ? 0 : EXIT_FAILURE);
 	}
 
-	if (stat(argv[optind], &buf) != 0) {
+	fd = open(argv[optind], O_RDONLY);
+	if (fd == -1) {
+		perror("open failed");
+		exit(EXIT_FAILURE);
+	}
+	if (fstat(fd, &buf) != 0) {
 		perror("status check failed");
+		close(fd);
 		exit(EXIT_FAILURE);
 	}
 	if (!(buf.st_mode & S_IFCHR)) {
 		fprintf(stderr, "%s is not a character special device\n",
 				argv[optind]);
-		exit(EXIT_FAILURE);
-	}
-	fd = open(argv[optind], O_RDONLY);
-	if (fd == -1) {
-		perror("open failed");
+		close(fd);
 		exit(EXIT_FAILURE);
 	}
 
diff --git a/misc-utils/ftl_format.c b/misc-utils/ftl_format.c
index bf3c8f2..34d436c 100644
--- a/misc-utils/ftl_format.c
+++ b/misc-utils/ftl_format.c
@@ -312,18 +312,20 @@ int main(int argc, char *argv[])
 		exit(errflg > 0 ? EXIT_SUCCESS : EXIT_FAILURE);
 	}
 
-	if (stat(argv[optind], &buf) != 0) {
+	fd = open(argv[optind], O_RDWR);
+	if (fd == -1) {
+		perror("open failed");
+		exit(EXIT_FAILURE);
+	}
+	if (fstat(fd, &buf) != 0) {
 		perror("status check failed");
+		close(fd);
 		exit(EXIT_FAILURE);
 	}
 	if (!(buf.st_mode & S_IFCHR)) {
 		fprintf(stderr, "%s is not a character special device\n",
 				argv[optind]);
-		exit(EXIT_FAILURE);
-	}
-	fd = open(argv[optind], O_RDWR);
-	if (fd == -1) {
-		perror("open failed");
+		close(fd);
 		exit(EXIT_FAILURE);
 	}
 
-- 
2.24.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 4/8] mtd-utils: Fix some simple cases of uninitialized value reads

[David Oberhollenzer]

This patch modifies the internal helpers to read and parse integers
from sysfs files by initializing them first and removes turns an
obscure "a = open(...) if (a >= 0) {...} if (a == -1) {...}" inside
recv_image into a more straight forward if/else branch.

Signed-off-by: David Oberhollenzer <david.oberhollenzer@sigma-star.at>
---
 lib/libmtd.c            | 2 ++
 lib/libubi.c            | 1 +
 misc-utils/recv_image.c | 3 +--
 3 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/libmtd.c b/lib/libmtd.c
index 564e5c0..9d8d0e8 100644
--- a/lib/libmtd.c
+++ b/lib/libmtd.c
@@ -217,6 +217,7 @@ static int read_hex_ll(const char *file, long long *value)
 	}
 	buf[rd] = '\0';
 
+	*value = 0;
 	if (sscanf(buf, "%llx\n", value) != 1) {
 		errmsg("cannot read integer from \"%s\"\n", file);
 		errno = EINVAL;
@@ -269,6 +270,7 @@ static int read_pos_ll(const char *file, long long *value)
 		goto out_error;
 	}
 
+	*value = 0;
 	if (sscanf(buf, "%lld\n", value) != 1) {
 		errmsg("cannot read integer from \"%s\"\n", file);
 		errno = EINVAL;
diff --git a/lib/libubi.c b/lib/libubi.c
index 4322a19..aaeeb38 100644
--- a/lib/libubi.c
+++ b/lib/libubi.c
@@ -94,6 +94,7 @@ static int read_positive_ll(const char *file, long long *value)
 	}
 	buf[rd] = '\0';
 
+	*value = 0;
 	if (sscanf(buf, "%lld\n", value) != 1) {
 		errmsg("cannot read integer from \"%s\"\n", file);
 		errno = EINVAL;
diff --git a/misc-utils/recv_image.c b/misc-utils/recv_image.c
index 7f6662b..eeaa2e2 100644
--- a/misc-utils/recv_image.c
+++ b/misc-utils/recv_image.c
@@ -81,8 +81,7 @@ int main(int argc, char **argv)
 			printf("Receive to MTD device %s with erasesize %d\n",
 			       argv[3], meminfo.erasesize);
 		}
-	}
-	if (flfd == -1) {
+	} else {
 		/* Try again, as if it's a file */
 		flfd = open(argv[3], O_CREAT|O_TRUNC|O_RDWR, 0644);
 		if (flfd < 0) {
-- 
2.24.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 5/8] mtd-utils: Fix wrong argument to sizeof in nanddump

[David Oberhollenzer]

Some temporary buffers are allocated with "sizeof(pointer) * count" as
size argument, which cannot possibly be correct.

Assuming what was meant was "sizeof(pointer[0]) * count" makes sense
in the context of how the buffers are used, but is actually pretty
pointless, since the buffers are unsigend char.

Signed-off-by: David Oberhollenzer <david.oberhollenzer@sigma-star.at>
---
 nand-utils/nanddump.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/nand-utils/nanddump.c b/nand-utils/nanddump.c
index 62699e0..d7fc320 100644
--- a/nand-utils/nanddump.c
+++ b/nand-utils/nanddump.c
@@ -362,8 +362,8 @@ int main(int argc, char * const argv[])
 		return errmsg("mtd_get_dev_info failed");
 
 	/* Allocate buffers */
-	oobbuf = xmalloc(sizeof(oobbuf) * mtd.oob_size);
-	readbuf = xmalloc(sizeof(readbuf) * mtd.min_io_size);
+	oobbuf = xmalloc(mtd.oob_size);
+	readbuf = xmalloc(mtd.min_io_size);
 
 	if (noecc)  {
 		if (ioctl(fd, MTDFILEMODE, MTD_FILE_MODE_RAW) != 0) {
-- 
2.24.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 6/8] mtd-utils: Fix "are we really at EOF" test logic in libubi read_data

[David Oberhollenzer]

The function reads file data into a buffer and then checks if we
actually are at the end-of-file by trying to read one more byte.
For whatever reason, the code uses an int instead of a char. It's
not pretty but works. But again, this is something that every
static analysis tool barks at.

Further more, the error messages are inverted. "We aren't at EOF yet"
is printed on failure and something like "read error %m" is printed
on success.

This patch fixes all of the above.

Signed-off-by: David Oberhollenzer <david.oberhollenzer@sigma-star.at>
---
 lib/libubi.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/lib/libubi.c b/lib/libubi.c
index aaeeb38..afe3648 100644
--- a/lib/libubi.c
+++ b/lib/libubi.c
@@ -156,7 +156,8 @@ static int read_positive_int(const char *file, int *value)
  */
 static int read_data(const char *file, void *buf, int buf_len)
 {
-	int fd, rd, tmp, tmp1;
+	int fd, rd, tmp1;
+	char tmp;
 
 	fd = open(file, O_RDONLY);
 	if (fd == -1)
@@ -178,11 +179,11 @@ static int read_data(const char *file, void *buf, int buf_len)
 
 	/* Make sure all data is read */
 	tmp1 = read(fd, &tmp, 1);
-	if (tmp1 == 1) {
+	if (tmp1 < 0) {
 		sys_errmsg("cannot read \"%s\"", file);
 		goto out_error;
 	}
-	if (tmp1) {
+	if (tmp1 > 0) {
 		errmsg("file \"%s\" contains too much data (> %d bytes)",
 		       file, buf_len);
 		errno = EINVAL;
-- 
2.24.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 7/8] mtd-utils: Fix potentially unterminated strings

[David Oberhollenzer]

This commit fixes some uses of strncpy that could leave the destination
buffer unterminated.

Signed-off-by: David Oberhollenzer <david.oberhollenzer@sigma-star.at>
---
 lib/libubi.c                  | 3 ++-
 misc-utils/mtdpart.c          | 4 +++-
 tests/checkfs/checkfs.c       | 3 ++-
 tests/jittertest/JitterTest.c | 3 ++-
 4 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/lib/libubi.c b/lib/libubi.c
index afe3648..baaca2f 100644
--- a/lib/libubi.c
+++ b/lib/libubi.c
@@ -1008,7 +1008,8 @@ int ubi_mkvol(libubi_t desc, const char *node, struct ubi_mkvol_request *req)
 	if (n > UBI_MAX_VOLUME_NAME)
 		return -1;
 
-	strncpy(r.name, req->name, UBI_MAX_VOLUME_NAME + 1);
+	strncpy(r.name, req->name, UBI_MAX_VOLUME_NAME);
+	r.name[UBI_MAX_VOLUME_NAME] = '\0';
 	r.name_len = n;
 
 	fd = open(node, O_RDONLY);
diff --git a/misc-utils/mtdpart.c b/misc-utils/mtdpart.c
index e480e1b..ba35d87 100644
--- a/misc-utils/mtdpart.c
+++ b/misc-utils/mtdpart.c
@@ -174,7 +174,9 @@ int main(int argc, char * const argv[])
 		case COMMAND_ADD:
 			part.start = start_addr;
 			part.length = length;
-			strncpy(part.devname, part_name, sizeof(part.devname));
+			strncpy(part.devname, part_name,
+				sizeof(part.devname) - 1);
+			part.devname[sizeof(part.devname) - 1] = '\0';
 			arg.op = BLKPG_ADD_PARTITION;
 			break;
 		case COMMAND_DEL:
diff --git a/tests/checkfs/checkfs.c b/tests/checkfs/checkfs.c
index 3e34cc4..203ad5c 100644
--- a/tests/checkfs/checkfs.c
+++ b/tests/checkfs/checkfs.c
@@ -512,7 +512,8 @@ static void processCmdLine(int argc, char **argv)
     {
         if(strcmp(argv[cnt], CMDLINE_PORT) == 0)
         {
-            strncpy(SerialDevice, argv[++cnt], sizeof(SerialDevice));
+            strncpy(SerialDevice, argv[++cnt], sizeof(SerialDevice) - 1);
+	    SerialDevice[sizeof(SerialDevice) - 1] = '\0';
             continue;
         }else
             if(strcmp(argv[cnt], CMDLINE_MAXFILEBYTES) == 0)
diff --git a/tests/jittertest/JitterTest.c b/tests/jittertest/JitterTest.c
index 797035b..2bee0b0 100644
--- a/tests/jittertest/JitterTest.c
+++ b/tests/jittertest/JitterTest.c
@@ -859,7 +859,8 @@ void HandleCmdLineArgs(
 	      /* Set the file to log console log on. */
 	      ++argNum;
 
-	      strncpy(LogFile, argv[argNum], sizeof(LogFile));
+	      strncpy(LogFile, argv[argNum], sizeof(LogFile) - 1);
+	      LogFile[sizeof(LogFile) - 1] = '\0';
             }
 
             else if ((strcmp(argv[argNum],"--grab_kprofile") ==
-- 
2.24.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

[PATCH 8/8] mtd-utils: Add checks to code that copies strings into fixed sized buffers

[David Oberhollenzer]

Signed-off-by: David Oberhollenzer <david.oberhollenzer@sigma-star.at>
---
 jffsX-utils/jffs2dump.c | 3 ++-
 ubi-utils/ubirename.c   | 7 +++++++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/jffsX-utils/jffs2dump.c b/jffsX-utils/jffs2dump.c
index ad7a9e3..d30b59f 100644
--- a/jffsX-utils/jffs2dump.c
+++ b/jffsX-utils/jffs2dump.c
@@ -149,7 +149,8 @@ static void process_options (int argc, char *argv[])
 				break;
 			case 'e':
 				convertendian = 1;
-				strcpy (cnvfile, optarg);
+				strncpy (cnvfile, optarg, sizeof(cnvfile) - 1);
+				cnvfile[sizeof(cnvfile) - 1] = '\0';
 				break;
 			case 'r':
 				recalccrc = 1;
diff --git a/ubi-utils/ubirename.c b/ubi-utils/ubirename.c
index f88ef82..97bf030 100644
--- a/ubi-utils/ubirename.c
+++ b/ubi-utils/ubirename.c
@@ -126,6 +126,13 @@ int main(int argc, char * const argv[])
 
 		rnvol.ents[count].vol_id = err;
 		rnvol.ents[count].name_len = strlen(argv[i + 1]);
+
+		if (rnvol.ents[count].name_len >=
+		    sizeof(rnvol.ents[count].name)) {
+			errmsg("\"%s\" volume name too long", argv[i + 1]);
+			goto out_libubi;
+		}
+
 		strcpy(rnvol.ents[count++].name, argv[i + 1]);
 	}
 
-- 
2.24.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/