| 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
| ||
|
Message-ID: <CAGXu5jJjUxGSA08GkGguhTBTTmVjKgG90LLNxfNnhd7TV4crhg@mail.gmail.com>
Date: Tue, 25 Jun 2013 09:55:07 -0700
From: Kees Cook <keescook@...omium.org>
To: Aruna Balakrishnaiah <aruna@...ux.vnet.ibm.com>
Cc: jkenisto@...ux.vnet.ibm.com, Tony Luck <tony.luck@...el.com>,
Colin Cross <ccross@...roid.com>,
LKML <linux-kernel@...r.kernel.org>,
Anton Vorontsov <cbouatmailru@...il.com>,
linuxppc-dev@...abs.org, paulus@...ba.org,
Anton Blanchard <anton@...ba.org>, mahesh@...ux.vnet.ibm.com
Subject: Re: [PATCH 3/3] powerpc/pseries: Support compression of oops text via pstore
On Tue, Jun 25, 2013 at 12:04 AM, Aruna Balakrishnaiah
<aruna@...ux.vnet.ibm.com> wrote:
> Hi Kees,
>
>
> On Monday 24 June 2013 11:27 PM, Kees Cook wrote:
>>
>> On Sun, Jun 23, 2013 at 11:23 PM, Aruna Balakrishnaiah
>> <aruna@...ux.vnet.ibm.com> wrote:
>>>
>>> The patch set supports compression of oops messages while writing to
>>> NVRAM,
>>> this helps in capturing more of oops data to lnx,oops-log. The pstore
>>> file
>>> for oops messages will be in decompressed format making it readable.
>>>
>>> In case compression fails, the patch takes care of copying the header
>>> added
>>> by pstore and last oops_data_sz bytes of big_oops_buf to NVRAM so that we
>>> have recent oops messages in lnx,oops-log.
>>>
>>> In case decompression fails, it will result in absence of oops file but
>>> still
>>> have files (in /dev/pstore) for other partitions.
>>>
>>> Signed-off-by: Aruna Balakrishnaiah <aruna@...ux.vnet.ibm.com>
>>> ---
>>> arch/powerpc/platforms/pseries/nvram.c | 132
>>> +++++++++++++++++++++++++++++---
>>> 1 file changed, 118 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/arch/powerpc/platforms/pseries/nvram.c
>>> b/arch/powerpc/platforms/pseries/nvram.c
>>> index 0159d74..b5ba5e2 100644
>>> --- a/arch/powerpc/platforms/pseries/nvram.c
>>> +++ b/arch/powerpc/platforms/pseries/nvram.c
>>> @@ -539,6 +539,65 @@ static int zip_oops(size_t text_len)
>>> }
>>>
>>> #ifdef CONFIG_PSTORE
>>> +/* Derived from logfs_uncompress */
>>> +int nvram_decompress(void *in, void *out, size_t inlen, size_t outlen)
>>> +{
>>> + int err, ret;
>>> +
>>> + ret = -EIO;
>>> + err = zlib_inflateInit(&stream);
>>> + if (err != Z_OK)
>>> + goto error;
>>> +
>>> + stream.next_in = in;
>>> + stream.avail_in = inlen;
>>> + stream.total_in = 0;
>>> + stream.next_out = out;
>>> + stream.avail_out = outlen;
>>> + stream.total_out = 0;
>>> +
>>> + err = zlib_inflate(&stream, Z_FINISH);
>>> + if (err != Z_STREAM_END)
>>> + goto error;
>>> +
>>> + err = zlib_inflateEnd(&stream);
>>> + if (err != Z_OK)
>>> + goto error;
>>> +
>>> + ret = stream.total_out;
>>> +error:
>>> + return ret;
>>> +}
>>> +
>>> +static int unzip_oops(char *oops_buf, char *big_buf)
>>> +{
>>> + struct oops_log_info *oops_hdr = (struct oops_log_info
>>> *)oops_buf;
>>> + u64 timestamp = oops_hdr->timestamp;
>>> + char *big_oops_data = NULL;
>>> + char *oops_data_buf = NULL;
>>> + size_t big_oops_data_sz;
>>> + int unzipped_len;
>>> +
>>> + big_oops_data = big_buf + sizeof(struct oops_log_info);
>>> + big_oops_data_sz = big_oops_buf_sz - sizeof(struct
>>> oops_log_info);
>>> + oops_data_buf = oops_buf + sizeof(struct oops_log_info);
>>> +
>>> + unzipped_len = nvram_decompress(oops_data_buf, big_oops_data,
>>> + oops_hdr->report_length,
>>> + big_oops_data_sz);
>>> +
>>> + if (unzipped_len < 0) {
>>> + pr_err("nvram: decompression failed; returned %d\n",
>>> +
>>> unzipped_len);
>>> + return -1;
>>> + }
>>> + oops_hdr = (struct oops_log_info *)big_buf;
>>> + oops_hdr->version = OOPS_HDR_VERSION;
>>> + oops_hdr->report_length = (u16) unzipped_len;
>>> + oops_hdr->timestamp = timestamp;
>>> + return 0;
>>> +}
>>> +
>>> static int nvram_pstore_open(struct pstore_info *psi)
>>> {
>>> /* Reset the iterator to start reading partitions again */
>>> @@ -567,6 +626,7 @@ static int nvram_pstore_write(enum pstore_type_id
>>> type,
>>> size_t size, struct pstore_info *psi)
>>> {
>>> int rc;
>>> + unsigned int err_type = ERR_TYPE_KERNEL_PANIC;
>>> struct oops_log_info *oops_hdr = (struct oops_log_info *)
>>> oops_buf;
>>>
>>> /* part 1 has the recent messages from printk buffer */
>>> @@ -577,8 +637,31 @@ static int nvram_pstore_write(enum pstore_type_id
>>> type,
>>> oops_hdr->version = OOPS_HDR_VERSION;
>>> oops_hdr->report_length = (u16) size;
>>> oops_hdr->timestamp = get_seconds();
>>> +
>>> + if (big_oops_buf) {
>>> + rc = zip_oops(size);
>>> + /*
>>> + * If compression fails copy recent log messages from
>>> + * big_oops_buf to oops_data.
>>> + */
>>> + if (rc != 0) {
>>> + int hsize = pstore_get_header_size();
>>
>> I think I would rather see the API to pstore_write() changed to
>> include explicit details about header sizes. Mkaing hsize a global
>> seems unwise, since it's not strictly going to be a constant value. It
>> could change between calls to the writer, for example.
>
>
> Introducing headersize in pstore_write() API would need changes at
> multiple places whereits being called. The idea is to move the
> compression support to pstore infrastructure so that other platforms
> could also make use of it. Once the compression support gets in,
> header size argument in pstore_write() will have to be deprecated.
>
> Till the time compression support for pstore goes in, can't we call
> pstore_header_size before every write call to knowthe header size.
I think it would be cleaner for it to be passed as part of the
pstore_write function. It does mean touching all the other callers,
but using a global variable for this is just wrong, IMO. :)
It would be a progression, at some point in the future, the
pstore_write function could lose the header size argument when it was
the right time.
-Kees
>
>> Beyond that, this all seems sensible, though it would be kind of cool
>> to move this compression logic into the pstore core so it would get
>> used by default (or through a module parameter).
>> -Kees
>>
>>> + size_t diff = size - oops_data_sz + hsize;
>>> +
>>> + if (size > oops_data_sz) {
>>> + memcpy(oops_data, big_oops_buf, hsize);
>>> + memcpy(oops_data + hsize, big_oops_buf +
>>> diff,
>>> + oops_data_sz - hsize);
>>> +
>>> + oops_hdr->report_length = (u16)
>>> oops_data_sz;
>>> + } else
>>> + memcpy(oops_data, big_oops_buf, size);
>>> + } else
>>> + err_type = ERR_TYPE_KERNEL_PANIC_GZ;
>>> + }
>>> +
>>> rc = nvram_write_os_partition(&oops_log_partition, oops_buf,
>>> - (int) (sizeof(*oops_hdr) + size), ERR_TYPE_KERNEL_PANIC,
>>> + (int) (sizeof(*oops_hdr) + oops_hdr->report_length),
>>> err_type,
>>> count);
>>>
>>> if (rc != 0)
>>> @@ -600,10 +683,11 @@ static ssize_t nvram_pstore_read(u64 *id, enum
>>> pstore_type_id *type,
>>> struct oops_log_info *oops_hdr;
>>> unsigned int err_type, id_no, size = 0;
>>> struct nvram_os_partition *part = NULL;
>>> - char *buff = NULL;
>>> - int sig = 0;
>>> + char *buff = NULL, *big_buff = NULL;
>>> + int rc, sig = 0;
>>> loff_t p;
>>>
>>> +read_partition:
>>> read_type++;
>>>
>>> switch (nvram_type_ids[read_type]) {
>>> @@ -666,6 +750,25 @@ static ssize_t nvram_pstore_read(u64 *id, enum
>>> pstore_type_id *type,
>>> if (nvram_type_ids[read_type] == PSTORE_TYPE_DMESG) {
>>> oops_hdr = (struct oops_log_info *)buff;
>>> *buf = buff + sizeof(*oops_hdr);
>>> +
>>> + if (err_type == ERR_TYPE_KERNEL_PANIC_GZ) {
>>> + big_buff = kmalloc(big_oops_buf_sz, GFP_KERNEL);
>>> + if (!big_buff)
>>> + return -ENOMEM;
>>> +
>>> + rc = unzip_oops(buff, big_buff);
>>> +
>>> + if (rc != 0) {
>>> + kfree(buff);
>>> + kfree(big_buff);
>>> + goto read_partition;
>>> + }
>>> +
>>> + oops_hdr = (struct oops_log_info *)big_buff;
>>> + *buf = big_buff + sizeof(*oops_hdr);
>>> + kfree(buff);
>>> + }
>>> +
>>> time->tv_sec = oops_hdr->timestamp;
>>> time->tv_nsec = 0;
>>> return oops_hdr->report_length;
>>> @@ -687,17 +790,18 @@ static int nvram_pstore_init(void)
>>> {
>>> int rc = 0;
>>>
>>> - nvram_pstore_info.buf = oops_data;
>>> - nvram_pstore_info.bufsize = oops_data_sz;
>>> + if (big_oops_buf) {
>>> + nvram_pstore_info.buf = big_oops_buf;
>>> + nvram_pstore_info.bufsize = big_oops_buf_sz;
>>> + } else {
>>> + nvram_pstore_info.buf = oops_data;
>>> + nvram_pstore_info.bufsize = oops_data_sz;
>>> + }
>>>
>>> rc = pstore_register(&nvram_pstore_info);
>>> if (rc != 0)
>>> pr_err("nvram: pstore_register() failed, defaults to "
>>> "kmsg_dump; returned %d\n", rc);
>>> - else
>>> - /*TODO: Support compression when pstore is configured */
>>> - pr_info("nvram: Compression of oops text supported only
>>> when "
>>> - "pstore is not configured");
>>>
>>> return rc;
>>> }
>>> @@ -731,11 +835,6 @@ static void __init nvram_init_oops_partition(int
>>> rtas_partition_exists)
>>> oops_data = oops_buf + sizeof(struct oops_log_info);
>>> oops_data_sz = oops_log_partition.size - sizeof(struct
>>> oops_log_info);
>>>
>>> - rc = nvram_pstore_init();
>>> -
>>> - if (!rc)
>>> - return;
>>> -
>>> /*
>>> * Figure compression (preceded by elimination of each line's
>>> <n>
>>> * severity prefix) will reduce the oops/panic report to at most
>>> @@ -759,6 +858,11 @@ static void __init nvram_init_oops_partition(int
>>> rtas_partition_exists)
>>> stream.workspace = NULL;
>>> }
>>>
>>> + rc = nvram_pstore_init();
>>> +
>>> + if (!rc)
>>> + return;
>>> +
>>> rc = kmsg_dump_register(&nvram_kmsg_dumper);
>>> if (rc != 0) {
>>> pr_err("nvram: kmsg_dump_register() failed; returned
>>> %d\n", rc);
>>>
>>
>>
>> --
>> Kees Cook
>> Chrome OS Security
>> _______________________________________________
>> Linuxppc-dev mailing list
>> Linuxppc-dev@...ts.ozlabs.org
>> https://lists.ozlabs.org/listinfo/linuxppc-dev
>>
>
--
Kees Cook
Chrome OS Security
--
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/
Powered by blists - more mailing lists