linux-kernel - Re: [PATCH v4] ramoops: use pstore interface

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, 17 Jan 2012 15:48:25 -0800
From:	Kees Cook <keescook@...omium.org>
To:	Andrew Morton <akpm@...ux-foundation.org>
Cc:	Tony Luck <tony.luck@...el.com>,
	Marco Stornelli <marco.stornelli@...il.com>,
	Arnd Bergmann <arnd@...db.de>,
	Greg Kroah-Hartman <greg@...ah.com>,
	Randy Dunlap <rdunlap@...otime.net>, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v4] ramoops: use pstore interface

On Tue, Jan 17, 2012 at 1:59 PM, Andrew Morton
<akpm@...ux-foundation.org> wrote:
> On Sat, 7 Jan 2012 09:15:16 -0800
> Kees Cook <keescook@...omium.org> wrote:
>
>> Instead of using /dev/mem directly, use the common pstore infrastructure
>> to handle Oops gathering and extraction.
>
> um, why?  This changelog provides no reason for anyone to apply the
> patch!

Good point; this was lost as the patch cycled. I will expand this.

>> +     struct ramoops_context *cxt = (struct ramoops_context *)psi->data;
>
> Unneeded and undesirable cast of void*.  Multiple instances of this.

Ah yes; I will fix this.

>> +     /* TODO(kees): Bogus time for the moment. */
>
> Is this hard to fix now?

I felt it was out of scope for the moment. There was enough change
happening for bolting it to pstore that adding a header with magic
values, etc, seemed like a logically separate task. As such, I left
this as TODO.

> Note that pstore_get_records() will treat the -ve errno returns from
> ->read() in the same manner as EOF.  IOW, your error codes will be
> dropped on the floor.  This appears to be a bug in pstore_get_records().

Well, IIUC, it just means the file doesn't get populated at all; there
is no userspace interface to finding out why a file didn't appear in
the pstore fliesystem. But yes, the specifics of the error are ignored
by pstore_get_records(). It didn't seem wrong to produce meaningful
codes in ramoops, though.

>> +     /* Only store dmesg dumps. */
>> +     if (type != PSTORE_TYPE_DMESG)
>> +             return -EINVAL;
>> +
>> +     /* Only store crash dumps. */
>>       if (reason != KMSG_DUMP_OOPS &&
>> -         reason != KMSG_DUMP_PANIC &&
>> -         reason != KMSG_DUMP_KEXEC)
>> -             return;
>> +         reason != KMSG_DUMP_PANIC)
>> +             return -EINVAL;
>>
>>       /* Only dump oopses if dump_oops is set */
>>       if (reason == KMSG_DUMP_OOPS && !cxt->dump_oops)
>
> The above three comments describe what the code does, which was
> obvious.  They failed to describe why it does this, which was
> unobvious.  Sigh.

They were terse; I will attempt to expand on them.

Thanks for the review! I will send v5 shortly...

-Kees

-- 
Kees Cook
ChromeOS 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/