linux-kernel - Re: [PATCH 00/11] Add compression support to pstore

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]

Date:	Tue, 6 Aug 2013 16:36:13 -0700
From:	Tony Luck <tony.luck@...il.com>
To:	Aruna Balakrishnaiah <aruna@...ux.vnet.ibm.com>
Cc:	"linuxppc-dev@...abs.org" <linuxppc-dev@...abs.org>,
	"paulus@...ba.org" <paulus@...ba.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"benh@...nel.crashing.org" <benh@...nel.crashing.org>,
	"keescook@...omium.org" <keescook@...omium.org>
Subject: Re: [PATCH 00/11] Add compression support to pstore

On Mon, Aug 5, 2013 at 2:20 PM, Tony Luck <tony.luck@...il.com> wrote:
> Still have problems booting if there are any compressed images in ERST
> to be inflated.

So I took another look at this part of the code ... and saw a couple of issues:

        while ((size = psi->read(&id, &type, &count, &time, &buf, &compressed,
                                psi)) > 0) {
                if (compressed && (type == PSTORE_TYPE_DMESG)) {
                        big_buf_sz = (psinfo->bufsize * 100) / 45;
                        big_buf = allocate_buf_for_decompression(big_buf_sz);

                        if (big_buf || stream.workspace)
>>> Did you mean "&&" here rather that "||"?
                                unzipped_len = pstore_decompress(buf, big_buf,
                                                        size, big_buf_sz);
>>> Need an "else" here to set unzipped_len to -1 (or set it to -1 down
>>> at the bottom of the loop ready for next time around.

                        if (unzipped_len > 0) {
                                buf = big_buf;
>>> This sets us up for problems.  First, you just overwrote the address
>>> of the buffer that psi->read allocated - so we have a memory leak. But
>>> worse than that we now double free the same buffer below when we
>>> kfree(buf) and then kfree(big_buf)
                                size = unzipped_len;
                                compressed = false;
                        } else {
                                pr_err("pstore: decompression failed;"
                                        "returned %d\n", unzipped_len);
                                compressed = true;
                        }
                }
                rc = pstore_mkfile(type, psi->name, id, count, buf,
                                  compressed, (size_t)size, time, psi);
                kfree(buf);
                kfree(stream.workspace);
                kfree(big_buf);
                buf = NULL;
                stream.workspace = NULL;
                big_buf = NULL;
                if (rc && (rc != -EEXIST || !quiet))
                        failed++;
        }


See attached patch that fixes these - but the code still looks like it
could be cleaned up a bit more.

-Tony

Download attachment "pstore.patch" of type "application/octet-stream" (807 bytes)