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] [day] [month] [year] [list]
Message-ID: <20241008174013.7b957a99@kaneli>
Date: Tue, 8 Oct 2024 17:40:13 +0300
From: Lasse Collin <lasse.collin@...aani.org>
To: Andrew Morton <akpm@...ux-foundation.org>, Vishnu Sanal T
 <t.v.s10123@...il.com>
Cc: linux-kernel@...r.kernel.org
Subject: Re: [PATCH] fix: possible memory leak in unxz()

In short, I dont want this patch to be merged, especially with its
current misleading commit message.

On 2024-10-07 Andrew Morton wrote:
> must_free_in needn't exist - `in' is always non-NULL here.

No, must_free_in is needed. The API (defined in
include/linux/decompress/generic.h) packs three different functions
into a single function:

1. Buffer to buffer (from "in"/"inbuf" to "out"/"outbuf")
2. Buffer ("in"/"inbuf") to callback function (flush)
3. Callback function (fill) to callback function (flush)

The callback functions need input and output buffers. For "flush", the
buffer always has to be allocated.

But, for some reason, generic.h says that "fill" can use the input
buffer provided by the caller when it's non-NULL. Thus, unxz()
allocates "in" conditionally and must_free_in is needed to avoid
freeing "in" when unxz() didn't allocate it.

I don't know if anything actually uses this subvariant where fill !=
NULL && in != NULL. Several other lib/decompress_*.c files support it
too although unlzo and unlz4 don't. So perhaps the feature is an
unneeded complication. Getting rid of this feature could make the code
less confusing. It would also allow making the "in" pointer
const-correct for buffer-to-{buffer,callback} cases.

> And free(NULL) is OK anwyay.

No, it's not. The free() implementation in
include/linux/decompress/mm.h doesn't check for NULL; each free() just
decrements the allocation counter. Thus free(NULL) would desync it.

As I wrote two days ago[1], this patch doesn't fix a memory leak or any
other bugs, and thus the commit message is incorrect. Still, I wasn't
really against the patch two days ago but I am now. I see the code
being misunderstood, and I feel this patch doesn't make it more
understandable.

[1] https://lore.kernel.org/lkml/20241006211720.5d7199b0@kaneli/

-- 
Lasse Collin

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ