Message-ID: <20161005124803.GE23809@pathway.suse.cz>
Date:   Wed, 5 Oct 2016 14:48:03 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     Sean Hudson <darknighte@...knighte.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 Tue 2016-10-04 12:55:35, Sean Hudson wrote:
> 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.

I see. The bootloader and the kernel never runs in parallel. So they
do not need to be synchronized using a lock or so. Huh, I do not
know what I have imagined.


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

IMHO, a copying of few messages is fine. Well, we need to make sure
that they are not printed twice on the console.


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

The question is if a one time walk might find all potential problems.
We might want to add checks into the existing code. For example,
log_from_idx() is rather sensitive but it does not check if
all the values are sane.


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

I am not expert. I think that the main trick on the memory
management level is that the userspace and kernel have different
address spaces. IMHO, the pages that are used by the kernel log buffer
cannot be addressed from userspace. In fact, each user have only very
limited number of pages that are mapped to his address space.

I do not know how this works with the persistent memory.

In each case, we need to make sure that the address of the
shared buffer is not readable in the booloaded config file
for a normal user.


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

There might be a problem how to bootstrap this. Someone has to define
the initial log control block. If it is done by bootloader and it uses
a wrong version, the kernel will not know the size of the external
buffer and will ignore and never use it.

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


I have discussed this with colleagues. All this looked to complicated
to them.

This patch set actually implements two features:

1. Possibility to preserve the kernel log buffer between boots.

   But this is already supported by pstore/ramoops.


2. Possibility to store bootloader messages in the kernel
   log buffer.

   The question is who wants it and if it is worth the effort.

   The bootloader will need to implement log storing in the
   non-trivial format. It will need to maintain compatibility
   of this format in the future.

   I guess that the bootloader is running on a single CPU
   and is basically one process. Therefore it does not need
   to solve any races. Therefore it does not need such a complex
   format.

   It might be more effective to write just a simple
   text into the persistent memory and provide a userspace tool
   to dump it.


Best Regards,
Petr

Powered by blists - more mailing lists