[PATCH v2] lib: gui: png_pico: fix use-after-free and double-free in png_open

Johannes Schneider <[email protected]>
Newsgroups org.infradead.lists.barebox
Message-ID <[email protected]>
png_open() set img->data from png_info->image->data and then called
png_alloc_free_all(), which freed every buffer the picopng allocator
tracks -- including the decoded pixel buffer.  Callers held a
dangling img->data, and the later png_close() free()'d it again.

Add png_alloc_detach() to drop a tracked address from the allocator
without freeing it, transferring ownership to the caller, and use it
in png_open() before png_alloc_free_all() runs.

Suggested-by: Ahmad Fatoum <[email protected]>
Signed-off-by: Johannes Schneider <[email protected]>
---
 lib/gui/picopng.c  | 13 ++++++++++++-
 lib/gui/picopng.h  |  7 +++++++
 lib/gui/png_pico.c |  8 +++++++-
 3 files changed, 26 insertions(+), 2 deletions(-)

diff --git a/lib/gui/picopng.c b/lib/gui/picopng.c
index 80f03beb68..ae733fde0f 100644
--- a/lib/gui/picopng.c
+++ b/lib/gui/picopng.c
@@ -103,7 +103,7 @@ static void png_alloc_free(void *addr)
 	free(addr);
 }
 
-void png_alloc_free_all()
+void png_alloc_free_all(void)
 {
 	while (png_alloc_tail) {
 		void *addr = png_alloc_tail->addr;
@@ -112,6 +112,17 @@ void png_alloc_free_all()
 	}
 }
 
+void *png_alloc_detach(void *addr)
+{
+	png_alloc_node_t *node = png_alloc_find_node(addr);
+
+	if (!node)
+		return NULL;
+
+	png_alloc_remove_node(node);
+	return addr;
+}
+
 /*************************************************************************************************/
 
 __maybe_unused static void vector32_cleanup(vector32_t *p)
diff --git a/lib/gui/picopng.h b/lib/gui/picopng.h
index a17dd14b0c..bad5f4c6c4 100644
--- a/lib/gui/picopng.h
+++ b/lib/gui/picopng.h
@@ -28,6 +28,13 @@ typedef struct {
 PNG_info_t *PNG_decode(const uint8_t *in, uint32_t size);
 void png_alloc_free_all(void);
 
+/*
+ * Remove @addr from the picopng allocator's bookkeeping without freeing it,
+ * transferring ownership of the buffer to the caller (who must free() it).
+ * Returns @addr on success or NULL if @addr was not tracked.
+ */
+void *png_alloc_detach(void *addr);
+
 unsigned picopng_zlib_decompress(unsigned char* out, size_t outsize,
 				 const unsigned char* in, size_t insize);
 
diff --git a/lib/gui/png_pico.c b/lib/gui/png_pico.c
index 029fee2a40..bf6eddb74b 100644
--- a/lib/gui/png_pico.c
+++ b/lib/gui/png_pico.c
@@ -67,7 +67,13 @@ struct image *png_open(char *inbuf, int insize)
 	img->width = png_info->width;
 	img->height = png_info->height;
 	img->bits_per_pixel = 4 << 3;
-	img->data = png_info->image->data;
+
+	/* detach so png_alloc_free_all() below leaves the pixel buffer alive */
+	img->data = png_alloc_detach(png_info->image->data);
+	if (!img->data) {
+		ret = -EINVAL;
+		goto err;
+	}
 
 	pr_debug("png: %d x %d data@0x%p\n", img->width, img->height, img->data);
 

base-commit: 651343da8af78d134d7ead4d2b36095d7ddc2d8f
-- 
2.43.0
lmpx.com only provides a reader for public news (NNTP) servers. It is not affiliated with the servers or forums shown here and is not responsible for the content of articles, which is written by their respective authors.