[v7,4/6] gen_init_cpio: fix short read file handling

Commit Message

David Disseldorp April 4, 2022, 9:34 a.m. UTC

When processing a "file" entry, gen_init_cpio attempts to allocate a
buffer large enough to stage the entire contents of the source file.
It then attempts to fill the buffer via a single read() call and
subsequently writes out the entire buffer length, without checking that
read() returned the full length, potentially writing uninitialized
buffer memory.

Fix this by breaking up file I/O into 64k chunks and only writing the
length returned by the prior read() call.

Signed-off-by: David Disseldorp <[e-mail]>
Reviewed-by: Martin Wilck <[e-mail]>
---
 usr/gen_init_cpio.c | 44 +++++++++++++++++++++++++-------------------
 1 file changed, 25 insertions(+), 19 deletions(-)

Comments

Andrew Morton April 26, 2022, 8:40 p.m. UTC | #1

On Mon,  4 Apr 2022 11:34:28 +0200 David Disseldorp <[e-mail]> wrote:

> When processing a "file" entry, gen_init_cpio attempts to allocate a
> buffer large enough to stage the entire contents of the source file.
> It then attempts to fill the buffer via a single read() call and
> subsequently writes out the entire buffer length, without checking that
> read() returned the full length, potentially writing uninitialized
> buffer memory.

That was rather rude of it.

> Fix this by breaking up file I/O into 64k chunks and only writing the
> length returned by the prior read() call.

Does this change fix any known or reported problems?

David Disseldorp April 27, 2022, 9:05 p.m. UTC | #2

On Tue, 26 Apr 2022 13:40:39 -0700, Andrew Morton wrote:

> On Mon,  4 Apr 2022 11:34:28 +0200 David Disseldorp <[e-mail]> wrote:
> 
> > When processing a "file" entry, gen_init_cpio attempts to allocate a
> > buffer large enough to stage the entire contents of the source file.
> > It then attempts to fill the buffer via a single read() call and
> > subsequently writes out the entire buffer length, without checking that
> > read() returned the full length, potentially writing uninitialized
> > buffer memory.  
> 
> That was rather rude of it.
> 
> > Fix this by breaking up file I/O into 64k chunks and only writing the
> > length returned by the prior read() call.  
> 
> Does this change fix any known or reported problems?

This was found via code inspection. I'm not aware of anyone hitting it
in the wild.

Thanks for the feedback, Andrew.

Cheers, David

Patch

diff --git a/usr/gen_init_cpio.c b/usr/gen_init_cpio.c
index 0e2c8a5838b1..9a0f8c37273a 100644
--- a/usr/gen_init_cpio.c
+++ b/usr/gen_init_cpio.c
@@ -20,6 +20,7 @@ 
 
 #define xstr(s) #s
 #define str(s) xstr(s)
+#define MIN(a, b) ((a) < (b) ? (a) : (b))
 
 static unsigned int offset;
 static unsigned int ino = 721;
@@ -297,9 +298,8 @@  static int cpio_mkfile(const char *name, const char *location,
 			unsigned int nlinks)
 {
 	char s[256];
-	char *filebuf = NULL;
 	struct stat buf;
-	long size;
+	unsigned long size;
 	int file = -1;
 	int retval;
 	int rc = -1;
@@ -326,22 +326,17 @@  static int cpio_mkfile(const char *name, const char *location,
 		buf.st_mtime = 0xffffffff;
 	}
 
-	filebuf = malloc(buf.st_size);
-	if (!filebuf) {
-		fprintf (stderr, "out of memory\n");
-		goto error;
-	}
-
-	retval = read (file, filebuf, buf.st_size);
-	if (retval < 0) {
-		fprintf (stderr, "Can not read %s file\n", location);
+	if (buf.st_size > 0xffffffff) {
+		fprintf(stderr, "%s: Size exceeds maximum cpio file size\n",
+			location);
 		goto error;
 	}
 
 	size = 0;
 	for (i = 1; i <= nlinks; i++) {
 		/* data goes on last link */
-		if (i == nlinks) size = buf.st_size;
+		if (i == nlinks)
+			size = buf.st_size;
 
 		if (name[0] == '/')
 			name++;
@@ -366,23 +361,34 @@  static int cpio_mkfile(const char *name, const char *location,
 		push_string(name);
 		push_pad();
 
-		if (size) {
-			if (fwrite(filebuf, size, 1, stdout) != 1) {
+		while (size) {
+			unsigned char filebuf[65536];
+			ssize_t this_read;
+			size_t this_size = MIN(size, sizeof(filebuf));
+
+			this_read = read(file, filebuf, this_size);
+			if (this_read <= 0 || this_read > this_size) {
+				fprintf(stderr, "Can not read %s file\n", location);
+				goto error;
+			}
+
+			if (fwrite(filebuf, this_read, 1, stdout) != 1) {
 				fprintf(stderr, "writing filebuf failed\n");
 				goto error;
 			}
-			offset += size;
-			push_pad();
+			offset += this_read;
+			size -= this_read;
 		}
+		push_pad();
 
 		name += namesize;
 	}
 	ino++;
 	rc = 0;
-	
+
 error:
-	if (filebuf) free(filebuf);
-	if (file >= 0) close(file);
+	if (file >= 0)
+		close(file);
 	return rc;
 }