Merge pull request #161 from igoropaniuk/memleaks_sec_issues

Address memory leaks/mitigate buffer overflows

Author: andersson

firehose.c

@@ -370,7 +370,8 @@ static int firehose_try_configure(struct qdl_device *qdl, bool skip_storage_init
 
 	if (storage != QDL_STORAGE_NAND) {
 		max_sector_size = sector_sizes[ARRAY_SIZE(sector_sizes) - 1];
-		buf = malloc(max_sector_size);
+		buf = alloca(max_sector_size);
+
 		memset(&op, 0, sizeof(op));
 		op.partition = 0;
 		op.start_sector = "1";

gpt.c

@@ -11,6 +11,7 @@
 #include <assert.h>
 #include <dirent.h>
 #include <fcntl.h>
+#include <inttypes.h>
 #include <stdbool.h>
 #include <stdint.h>
 #include <unistd.h>
@@ -140,8 +141,8 @@ static int gpt_load_table_from_partition(struct qdl_device *qdl, unsigned int ph
 	uint8_t buf[4096];
 	struct read_op op;
 	unsigned int offset;
-	unsigned int lba;
-	char lba_buf[10];
+	uint64_t lba;
+	char lba_buf[21];
 	uint16_t name_utf16le[36];
 	char name[36 * 4];
 	int ret;
@@ -178,7 +179,7 @@ static int gpt_load_table_from_partition(struct qdl_device *qdl, unsigned int ph
 
 		if (offset == 0) {
 			lba = gpt.part_entry_lba + i * gpt.part_entry_size / qdl->sector_size;
-			sprintf(lba_buf, "%u", lba);
+			snprintf(lba_buf, sizeof(lba_buf), "%" PRIu64, lba);
 			op.start_sector = lba_buf;
 
 			memset(buf, 0, sizeof(buf));

qdl.c

@@ -150,13 +150,14 @@ static uint32_t parse_ascii_hex32(const char *s)
 /**
  * decode_programmer_archive() - Attempt to decode a programmer CPIO archive
  * @blob: Loaded image to be decoded as archive
- * @images: List of Sahara images, with @images[0] populated
+ * @images: List of Sahara images to populate
  *
- * The single blob provided in @images[0] might be a CPIO archive containing
- * Sahara images, in files with names in the format "<id>:<filename>". Load
- * each such Sahara image into the relevant spot in the @images array.
+ * The blob might be a CPIO archive containing Sahara images, in files with
+ * names in the format "<id>:<filename>". Load each such Sahara image into the
+ * relevant spot in the @images array.
  *
- * The original blob (in @images[0]) is freed once it has been consumed.
+ * The blob is always consumed (freed) on both success and error paths.
+ * On error, any partially-populated @images entries are also freed.
  *
  * Returns: 0 if no archive was found, 1 if archive was decoded, -1 on error
  */
@@ -178,13 +179,13 @@ static int decode_programmer_archive(struct sahara_image *blob, struct sahara_im
 	for (;;) {
 		if (ptr + sizeof(*hdr) > end) {
 			ux_err("programmer archive is truncated\n");
-			return -1;
+			goto err;
 		}
 		hdr = ptr;
 
 		if (memcmp(hdr->c_magic, "070701", 6)) {
 			ux_err("expected cpio header in programmer archive\n");
-			return -1;
+			goto err;
 		}
 
 		filesize = parse_ascii_hex32(hdr->c_filesize);
@@ -193,12 +194,12 @@ static int decode_programmer_archive(struct sahara_image *blob, struct sahara_im
 		ptr += sizeof(*hdr);
 		if (ptr + namesize > end || ptr + filesize + namesize > end) {
 			ux_err("programmer archive is truncated\n");
-			return -1;
+			goto err;
 		}
 
 		if (namesize > sizeof(name)) {
 			ux_err("unexpected filename length in progammer archive\n");
-			return -1;
+			goto err;
 		}
 		memcpy(name, ptr, namesize);
 
@@ -209,7 +210,7 @@ static int decode_programmer_archive(struct sahara_image *blob, struct sahara_im
 		id = strtoul(tok, NULL, 0);
 		if (id == 0 || id >= MAPPING_SZ) {
 			ux_err("invalid image id \"%s\" in programmer archive\n", tok);
-			return -1;
+			goto err;
 		}
 
 		ptr += namesize;
@@ -231,6 +232,13 @@ static int decode_programmer_archive(struct sahara_image *blob, struct sahara_im
 	blob->len = 0;
 
 	return 1;
+
+err:
+	sahara_images_free(images, MAPPING_SZ);
+	free(blob->ptr);
+	blob->ptr = NULL;
+	blob->len = 0;
+	return -1;
 }
 
 /**
@@ -335,6 +343,10 @@ static int decode_sahara_config(struct sahara_image *blob, struct sahara_image *
 	return 1;
 
 err_free_doc:
+	sahara_images_free(images, MAPPING_SZ);
+	free(blob->ptr);
+	blob->ptr = NULL;
+	blob->len = 0;
 	xmlFreeDoc(doc);
 	free(blob_name_buf);
 	return -1;
@@ -747,6 +759,9 @@ static int qdl_flash(int argc, char **argv)
 		vip_gen_finalize(qdl);
 
 	qdl_close(qdl);
+
+	sahara_images_free(sahara_images, MAPPING_SZ);
+
 	free_programs();
 	free_patches();
 

qdl.h

@@ -2,6 +2,13 @@
 #ifndef __QDL_H__
 #define __QDL_H__
 
+#ifdef _WIN32
+#include <malloc.h>
+#define alloca _alloca
+#else
+#include <alloca.h>
+#endif
+
 #include <stdbool.h>
 
 #include "patch.h"
@@ -72,7 +79,7 @@ struct qdl_device {
 };
 
 struct sahara_image {
-	const char *name;
+	char *name;
 	void *ptr;
 	size_t len;
 };
@@ -106,6 +113,7 @@ int sahara_run(struct qdl_device *qdl, const struct sahara_image *images,
 	       const char *ramdump_path,
 	       const char *ramdump_filter);
 int load_sahara_image(const char *filename, struct sahara_image *image);
+void sahara_images_free(struct sahara_image *images, size_t count);
 void print_hex_dump(const char *prefix, const void *buf, size_t len);
 unsigned int attr_as_unsigned(xmlNode *node, const char *attr, int *errors);
 const char *attr_as_string(xmlNode *node, const char *attr, int *errors);

usb.c

@@ -328,7 +328,7 @@ static int usb_read(struct qdl_device *qdl, void *buf, size_t len, unsigned int
 		return -ETIMEDOUT;
 
 	/* If what we read equals the endpoint's Max Packet Size, consume the ZLP explicitly */
-	if (len == actual && !(actual % qdl_usb->in_maxpktsize)) {
+	if (len == (size_t)actual && !(actual % qdl_usb->in_maxpktsize)) {
 		ret = libusb_bulk_transfer(qdl_usb->usb_handle, qdl_usb->in_ep,
 					   NULL, 0, NULL, timeout);
 		if (ret)

util.c

@@ -242,6 +242,10 @@ int load_sahara_image(const char *filename, struct sahara_image *image)
 	lseek(fd, 0, SEEK_SET);
 
 	ptr = malloc(len);
+	if (!ptr) {
+		ux_err("failed to init buffer for content of \"%s\"\n", filename);
+		goto err_close;
+	}
 
 	n = read(fd, ptr, len);
 	if (n != len) {
@@ -262,3 +266,12 @@ int load_sahara_image(const char *filename, struct sahara_image *image)
 	close(fd);
 	return -1;
 }
+
+void sahara_images_free(struct sahara_image *images, size_t count)
+{
+	for (size_t i = 0; i < count; i++) {
+		free(images[i].name);
+		free(images[i].ptr);
+		images[i] = (struct sahara_image){};
+	}
+}