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]

Message-ID: <5201A9BD.4090503@linux.vnet.ibm.com>
Date:	Wed, 07 Aug 2013 07:28:21 +0530
From:	Aruna Balakrishnaiah <aruna@...ux.vnet.ibm.com>
To:	Tony Luck <tony.luck@...il.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 Wednesday 07 August 2013 05:06 AM, Tony Luck wrote:
> 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 "||"?

Yes right, it should be &&.

>                                  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.

The patch looks right. I will clean it up. Does the issue still persist after this?

> -Tony

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/