linux-kernel - Re: [PATCH v2] pstore/ram: no timekeeping calls when unavailable

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: <CAGXu5jKq0=+XfH6yDSaJMcd-S3=o7xB67kspPkCRcXjRSGgciw@mail.gmail.com>
Date:	Fri, 9 Nov 2012 17:26:53 -0800
From:	Kees Cook <keescook@...omium.org>
To:	John Stultz <johnstul@...ibm.com>
Cc:	linux-kernel@...r.kernel.org,
	Anton Vorontsov <cbouatmailru@...il.com>,
	Colin Cross <ccross@...roid.com>,
	Tony Luck <tony.luck@...el.com>,
	Thomas Gleixner <tglx@...utronix.de>
Subject: Re: [PATCH v2] pstore/ram: no timekeeping calls when unavailable

On Fri, Nov 9, 2012 at 4:56 PM, John Stultz <johnstul@...ibm.com> wrote:
> On 11/05/2012 02:00 PM, Kees Cook wrote:
>>
>> We must not call timekeeping functions unless they are available. If we
>> dump
>> before they have resumed, avoid a WARN_ON by setting the timestamp to 0.
>>
>> Since the "ram" pstore driver can be a module, we must have
>> timekeeping_suspended exported.
>>
>> Reported-by: Doug Anderson <dianders@...omium.org>
>> Cc: Anton Vorontsov <cbouatmailru@...il.com>
>> Cc: John Stultz <johnstul@...ibm.com>
>> Signed-off-by: Kees Cook <keescook@...omium.org>
>> ---
>> v2:
>>   - export needed for timekeeping_suspended (thanks to Fengguang Wu).
>> ---
>>   fs/pstore/ram.c           |    8 +++++++-
>>   kernel/time/timekeeping.c |    1 +
>>   2 files changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
>> index 1a4f6da..6d014e0 100644
>> --- a/fs/pstore/ram.c
>> +++ b/fs/pstore/ram.c
>> @@ -171,7 +171,13 @@ static size_t ramoops_write_kmsg_hdr(struct
>> persistent_ram_zone *prz)
>>         struct timeval timestamp;
>>         size_t len;
>>
>> -       do_gettimeofday(&timestamp);
>> +       /* Handle dumping before timekeeping has resumed. */
>> +       if (unlikely(timekeeping_suspended)) {
>> +               timestamp.tv_sec = 0;
>> +               timestamp.tv_usec = 0;
>> +       } else
>> +               do_gettimeofday(&timestamp);
>> +
>
> Would nulling out the timestamp be better done in do_gettimeofday()?  That
> way we don't have to export timekeeping internals and users would get
> something more sane for this corner case.

Well... I'm not sure. If we don't want to expose the
timekeeping_suspended variable, maybe we need a function to check
this? I think it's probably better to find the users of timekeeping
that could call it when suspended. That's why I figured the BUG was
there. Very very few things should be attempting to call gettimeofday
in a place where it might be suspended. As such, it seems like those
things should be able to determine how to handle it. Maybe not
everything would be sensible to get back 0s.

In this particular case, I'm fine with removing the BUG and returning
0 instead, since that's fine for ramoops. :)

-Kees

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