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

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

[Johannes Schneider]

From: Thomas Haemmerle <thomas.haemmerle@leica-geosystems.com>

png_alloc_free_all() frees all picopng-internal allocations, including
the image->data buffer.  The previous code stored a pointer to this
buffer in img->data and called png_alloc_free_all() — leaving img->data
as a dangling pointer.  The subsequent png_close()'s free(img->data)
then performed a double-free on already-freed memory, causing a crash or
heap corruption when displaying the boot logo.

Fix by copying the decoded pixel data into a fresh malloc buffer before
calling png_alloc_free_all().  png_close() correctly frees this copy.

Assisted-by: Claude:claude-sonnet-4-6
Signed-of-by: Thomas Haemmerle <thomas.haemmerle@leica-geosystems.com>
---
 lib/gui/png_pico.c | 21 +++++++++++++++++++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/lib/gui/png_pico.c b/lib/gui/png_pico.c
index 029fee2a40..8d70521b46 100644
--- a/lib/gui/png_pico.c
+++ b/lib/gui/png_pico.c
@@ -46,6 +46,8 @@ struct image *png_open(char *inbuf, int insize)
 {
 	PNG_info_t *png_info;
 	int ret;
+	size_t imgsize;
+	void *imgcopy;
 	struct image *img = calloc(1, sizeof(struct image));
 
 	if (!img)
@@ -67,12 +69,27 @@ 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;
 
-	pr_debug("png: %d x %d data@0x%p\n", img->width, img->height, img->data);
+	/*
+	 * Copy decoded pixels to a stable buffer before png_alloc_free_all()
+	 * frees the picopng internal allocations (including image->data).
+	 * Without this copy, img->data would be a dangling pointer and
+	 * png_close()'s free(img->data) would be a double-free.
+	 */
+	imgsize = png_info->width * png_info->height * 4;
+	imgcopy = malloc(imgsize);
+	if (!imgcopy) {
+		ret = -ENOMEM;
+		goto err;
+	}
+	memcpy(imgcopy, png_info->image->data, imgsize);
 
 	png_alloc_free_all();
 
+	img->data = imgcopy;
+
+	pr_debug("png: %d x %d data@0x%p\n", img->width, img->height, img->data);
+
 	return img;
 err:
 	png_alloc_free_all();
-- 
2.43.0

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

[Ahmad Fatoum]

Hello,

Thanks for the fix.

On 6/2/26 4:24 AM, Johannes Schneider wrote:
> From: Thomas Haemmerle <thomas.haemmerle@leica-geosystems.com>
> 
> png_alloc_free_all() frees all picopng-internal allocations, including
> the image->data buffer.  The previous code stored a pointer to this
> buffer in img->data and called png_alloc_free_all() — leaving img->data
> as a dangling pointer.  The subsequent png_close()'s free(img->data)
> then performed a double-free on already-freed memory, causing a crash or
> heap corruption when displaying the boot logo.
> 
> Fix by copying the decoded pixel data into a fresh malloc buffer before
> calling png_alloc_free_all().  png_close() correctly frees this copy.

Never ceases to amaze how long memory corruption can go unnoticed..

> -	pr_debug("png: %d x %d data@0x%p\n", img->width, img->height, img->data);
> +	/*
> +	 * Copy decoded pixels to a stable buffer before png_alloc_free_all()
> +	 * frees the picopng internal allocations (including image->data).
> +	 * Without this copy, img->data would be a dangling pointer and
> +	 * png_close()'s free(img->data) would be a double-free.
> +	 */
> +	imgsize = png_info->width * png_info->height * 4;
> +	imgcopy = malloc(imgsize);
> +	if (!imgcopy) {
> +		ret = -ENOMEM;
> +		goto err;
> +	}
> +	memcpy(imgcopy, png_info->image->data, imgsize);
>  
>  	png_alloc_free_all();
>  
> +	img->data = imgcopy;
> +
> +	pr_debug("png: %d x %d data@0x%p\n", img->width, img->height, img->data);
> +

I would prefer avoiding the memory copy here. My suggestion would be adding
(untested):

void *png_alloc_detach(void *addr)
{
        for (png_alloc_node_t *node = png_alloc_tail; node; node = node->prev) {
                if (node->addr == addr) {
                        png_alloc_remove_node(node);
                        return addr;
                }
        }

        return NULL;
}

and then a single line change in png_open:

-	img->data = png_info->image->data
+	img->data = png_alloc_detach(png_info->image->data);

What do you think?

Cheers,
Ahmad

>  	return img;
>  err:
>  	png_alloc_free_all();

-- 
Pengutronix e.K.                  |                             |
Steuerwalder Str. 21              | http://www.pengutronix.de/  |
31137 Hildesheim, Germany         | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686  | Fax:   +49-5121-206917-5555 |

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

[SCHNEIDER Johannes]

Hoi Ahmad,

>
>
> Hello,
>
> Thanks for the fix.
>

:-D

> On 6/2/26 4:24 AM, Johannes Schneider wrote:
> > From: Thomas Haemmerle <thomas.haemmerle@leica-geosystems.com>
> >
> > png_alloc_free_all() frees all picopng-internal allocations, including
> > the image->data buffer.  The previous code stored a pointer to this
> > buffer in img->data and called png_alloc_free_all() — leaving img->data
> > as a dangling pointer.  The subsequent png_close()'s free(img->data)
> > then performed a double-free on already-freed memory, causing a crash or
> > heap corruption when displaying the boot logo.
> >
> > Fix by copying the decoded pixel data into a fresh malloc buffer before
> > calling png_alloc_free_all().  png_close() correctly frees this copy.
>
> Never ceases to amaze how long memory corruption can go unnoticed..
>
> > -     pr_debug("png: %d x %d data@0x%p\n", img->width, img->height, img->data);
> > +     /*
> > +      * Copy decoded pixels to a stable buffer before png_alloc_free_all()
> > +      * frees the picopng internal allocations (including image->data).
> > +      * Without this copy, img->data would be a dangling pointer and
> > +      * png_close()'s free(img->data) would be a double-free.
> > +      */
> > +     imgsize = png_info->width * png_info->height * 4;
> > +     imgcopy = malloc(imgsize);
> > +     if (!imgcopy) {
> > +             ret = -ENOMEM;
> > +             goto err;
> > +     }
> > +     memcpy(imgcopy, png_info->image->data, imgsize);
> >
> >       png_alloc_free_all();
> >
> > +     img->data = imgcopy;
> > +
> > +     pr_debug("png: %d x %d data@0x%p\n", img->width, img->height, img->data);
> > +
>
> I would prefer avoiding the memory copy here. My suggestion would be adding
> (untested):
>
> void *png_alloc_detach(void *addr)
> {
>         for (png_alloc_node_t *node = png_alloc_tail; node; node = node->prev) {
>                 if (node->addr == addr) {
>                         png_alloc_remove_node(node);
>                         return addr;
>                 }
>         }
>
>         return NULL;
> }
>
> and then a single line change in png_open:
>
> -       img->data = png_info->image->data
> +       img->data = png_alloc_detach(png_info->image->data);
>
> What do you think?
>

good idea, sending out a v2
(with your 'suggested-by' :-)

>
> Cheers,
> Ahmad
>
> >       return img;
> >  err:
> >       png_alloc_free_all();
>

gruß
Johannes