linux-kernel - Re: [PATCH] fix: possible memory leak in unxz()

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20241006211720.5d7199b0@kaneli>
Date: Sun, 6 Oct 2024 21:17:20 +0300
From: Lasse Collin <lasse.collin@...aani.org>
To: Vishnu Sanal T <t.v.s10123@...il.com>
Cc: linux-kernel@...r.kernel.org, akpm@...ux-foundation.org
Subject: Re: [PATCH] fix: possible memory leak in unxz()

On 2024-10-06 Vishnu Sanal T wrote:
> Fixes possible memory leak in the function unxz() in
> lib/decompress_unxz.c forgets to free the pointer 'in', when
> the statement if (fill == NULL && flush == NULL) is true.

unxz() looks confusing but a memory leak shouldn't be possible. If "in"
is NULL then "fill" must be non-NULL. Otherwise the caller isn't
following the API defined in include/linux/decompress/generic.h and
things might break in other ways too. (I find the generic.h API
somewhat hairy.)

Note that both "in" and "fill" are allowed to be non-NULL at the same
time. That's why the code checks for "in == NULL" instead of "fill !=
NULL" before allocating memory.

The current malloc+free usage in unxz() is such that freeing is done in
reverse order compared to mallocs. I guess it's not important. At least
include/linux/decompress/mm.h doesn't care.

I think your patch makes it more obvious that there is no memory leak.
Perhaps it might help static analyzers too. The change shouldn't break
anything. On the other hand, it makes it less obvious to human readers
that free(in) is only needed when fill != NULL.

I didn't test but I guess that it shouldn't increase code size when
fill == NULL && flush == NULL. That is, compilers should see that
must_free_in must be false in that case. Even if they don't, it's not
many bytes.

I'm not in favor of this patch because the unxz() function is quite
delicate due to the API it is implementing, and the patch doesn't fix a
memory leak (unless the API is misused and then those misuses should be
fixed). On the other hand, I'm not against this patch either *if* other
developers think this is an improvement. In that case the commit
message should be changed since it's not fixing an actual memory leak.

Thanks!

-- 
Lasse Collin