Re: [RFC PATCH v1 2/2] printk: external log buffer (CONFIG_LOGBUFFER)

[Sean Hudson]

On 10/5/2016 2:48 PM, Petr Mladek wrote:
> 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:
>>>>>

<snip>

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

I happen to agree since this is a debug feature.  It would be nice to
avoid it, but every strategy that I've considered makes the code less
portable.

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

Checking for every possible problem isn't practical and would adversely
impact the performance of the logging code.  In the case of the function
you point out, the patch series doesn't change the logic, rather it
replaces the structure being used.

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

I do not agree with that last point.  If you cannot trust the bootloader
then the ability of a userspace program to read log contents seems to be
the least of your problems.

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

This is already defined in the code.  Using the wrong structure/version
will behave as expected, and be ignored.  That simply means that the
bootloader can't 'accidentally' use the wrong version and seems sane to
me.

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

It seems like the complexity comes from the problem itself and I'm
adding very little in the solution that I'm presenting.  How would they
suggest reducing that complexity?

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

I have not looked into either feature extensively.  However, I believe
that both work slightly differently.  For instance, this feature affords
more flexibility about the buffer location and size to the kernel in
that it doesn't require the kernel to reserve a chunk of RAM ahead of
time.  However, it might be possible to integrate these features
together in some manner in the future.

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

Additionally, this feature allows for multiple boot cycles to co-exist.
It also allows the _bootloader_ to understand/display _kernel_ log entries.

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

This feature existed before I picked it up in Wolfgang Denk's kernels.
It was broken by the changes to the internal log structures that came in
with 3.10.

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

yes, in order to match what the kernel uses, the bootloader will have to
use the same format.  FWIW, the kernel structure has been very stable
over time, with the last major change occurring in the 3.10 time frame.
Of course, any bootloader that wants to pass information to the kernel
in a common format will have to keep up to date.

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

The log structures do not provide protection for multiple CPUs.
Additional fields in the log structure are only there to provide
compatibility with the kernel.

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

That would not provide any benefit if the kernel doesn't successfully
start up user space.

--
Sean