Message-ID: <a4810c02-dc3e-a7f9-f613-aa961a930c44@darknighte.com>
Date:   Tue, 4 Oct 2016 12:55:35 -0500
From:   Sean Hudson <darknighte@...knighte.com>
To:     Petr Mladek <pmladek@...e.com>
Cc:     linux-kernel@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Yang Shi <yang.shi@...aro.org>, Ingo Molnar <mingo@...nel.org>,
        Prarit Bhargava <prarit@...hat.com>,
        Rasmus Villemoes <linux@...musvillemoes.dk>,
        "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
        Yaowei Bai <baiyaowei@...s.chinamobile.com>,
        Kees Cook <keescook@...omium.org>,
        Andrey Ryabinin <aryabinin@...tuozzo.com>,
        Johannes Weiner <hannes@...xchg.org>,
        Tejun Heo <tj@...nel.org>, Arnd Bergmann <arnd@...db.de>,
        Ard Biesheuvel <ard.biesheuvel@...aro.org>,
        Thomas Garnier <thgarnie@...gle.com>,
        Nicolas Pitre <nicolas.pitre@...aro.org>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Borislav Petkov <bp@...e.de>,
        Tim Bird <tim.bird@...ymobile.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Grant Likely <grant.likely@...retlab.ca>
Subject: Re: [RFC PATCH v1 2/2] printk: external log buffer (CONFIG_LOGBUFFER)

On 10/4/2016 6:27 AM, Petr Mladek wrote:
> On Fri 2016-09-30 23:06:49, Sean Hudson wrote:
>> On 9/30/2016 7:39 AM, Petr Mladek wrote:
>>>
>>> Is the bootloader allowed to write to the shared buffer from this
>>> point on? How will be synchronized the writes from the kernel and
>>> the bootloader?
>>
>> I am a bit confused by this question.  I assume that the kernel and
>> bootloader will not be accessing the same log space simultaneously.
>> While each one is running, they will utilize the same data structures to
>> keep track of current entries.
> 
> They both will write new messages at the end of the buffer. Therefore
> they both might want to write to exactly the same location. Also if
> the buffer is full, the oldest messages are overwritten. Therefore
> even reading must be synchronized against writes.
> 
> These operations are synchronized using logbuf_lock on the kernel side.
> I do not know much about bootloaders but I guess that the bootloader
> will not be able to use the logbuf_lock. So the question is how
> the bootloader and kernel will synchronize the access to the shared
> buffer.
> 
> Or did I miss anything?

The state of the log is preserved in the log control block, or lcb,
which is passed when the boot loader passes control to the kernel.  In
order to make the transition as seamless as possible, the boot loader
appends entries in the same way that the kernel does and overwrites the
oldest entries once the ring buffer is full.  It also updates tracking
indices in the lcb.  When the boot loader passes control to the kernel,
the kernel adopts the lcb and appends entries using that structure.  In
this way, the lcb provides continuity between the boot loader and the
kernel for the state of the buffer.

Note, there is a small window of time during early boot in which the
kernel puts entries into the default location, a staticlly declared
char[].  These entries are appended to the external location once
setup_ext_logbuff() is called from init/main.c.  (I'd love to close that
gap entirely, but all solutions I've tried involve arch specific code,
which I'm trying to avoid.)

>>> How protected will the address of the shared buffer? Would not
>>> it open a security hole? We need to make sure that non-privileged
>>> user or non-trusted application must not read sensitive data
>>> from the log or break the structure of the log by writing
>>> invalid data.
>>
>> There is no notion of protection of the address.  However, additional
>> checking could be added. Currently, there is potential for corrupt log
>> entries from the bootloader causing the kernel to crash.
>> I can't think of a way to prevent that from happening.
> 
> IMHO, the only way is to revisit all locations when the log buffer
> is accessed and add all the needed consistency checks. We must make
> sure that we do not access outside of the buffer. Also the log_*_seq
> numbers are supposed to grow linearly...

Adding an explicit walk of the external log entries should be trivial.
That will only validate that the passed in log entries are consistent
internally.  I will be submitting a v2 patch shortly that fixes an error
that kernel-ci caught. (if CONFIG_PRINTK is disabled, compilation fails)
I'll see about adding that check at the same time.

>>  Also, as you point out, a bootloader could read log contents from
>> the shared region. Of course, in order for either to happen,
>> the bootloader would already be compromised and I'm not sure that
>>  reading log entries or crashing the kernel is such a big consideration.
> 
> The log might contain addresses of some kernel structures, for example
> in some error messages. For this reason, only a privileged users are
> able to read it. We must make sure that they will not get accessible
> for a non-privileged user, for example via an address to the shared
> buffer in the bootloader config.

Ahhh, if I am understanding correctly, your concern is that an
unprivileged program in user space would be able to gain access directly
to the log entries via the external buffer location.  I haven't tested
that scenario, so I'm not sure if the memory reservation via the DT
protects that memory from user space.  I'll have to check on that.  In
the meantime, if you know of standard ways to protect memory from user
space access, please let me know.

> BTW: You did not answered the question about how the bootloader would
> know the right version of the log format. I am afraid that we do not
> want to maintain backward compatibility on the kernel side. The printk
> code already is too complex.

This is a matter of magic numbers.  Uboot has two preexisting log
structure versions. Since the kernel did not have a version, I used
LOGBUFF_LOG_VERSION = 3, if CONFIG_LOGBUFFER is enabled.  So, if the
boot loader passes the command line parameter 'lcb_phys_addr' and the
contents of that address pass the validation checks found in
is_valid_external_cb(), then the external buffer is used.  Those checks
include matching the version number, structure sizes match, etc.
Otherwise, the external buffer is ignored.  This should allow for the
kernel to change in the future, e.g. v4, and continue to run correctly
even if an older boot loader passes a v3 version of the log entries.

> Also you did not answered the question about your plans. I wonder
> which bootloader would be able to use such a feature.
> 

Sorry about that, I missed the question in your previous reply.  I am
preparing to submit a patch set to mainline uBoot that adds this capability.




Download attachment "signature.asc" of type "application/pgp-signature" (820 bytes)

Powered by blists - more mailing lists