linux-kernel - Re: [PATCH] pstore: Remove duplicate invoking of persistent_ram

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAGXu5jJtwLgrMVvXR+C8oSKPus+iVdYsYn4Q2+8FjRbgpL+jNA@mail.gmail.com>
Date:   Fri, 26 Oct 2018 10:44:48 +0100
From:   Kees Cook <keescook@...omium.org>
To:     Peng15 Wang 王鹏 <wangpeng15@...omi.com>
Cc:     "anton@...msg.org" <anton@...msg.org>,
        "ccross@...roid.com" <ccross@...roid.com>,
        "tony.luck@...el.com" <tony.luck@...el.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] pstore: Remove duplicate invoking of persistent_ram_zap()

On Fri, Oct 26, 2018 at 4:41 AM, Peng15 Wang 王鹏 <wangpeng15@...omi.com> wrote:
> From: Peng Wang <wangpeng15@...omi.com>
>
> When initialing przs with invalid data in buffer(no PERSISTENT_RAM_SIG),
> function call path is like this:
>
> ramoops_init_prz ->
> |
> |-> persistent_ram_new -> persistent_ram_post_init -> persistent_ram_zap
> |
> |--> persistent_ram_zap

There does appear to be a duplicate call to persistent_ram_zap() in this case.

> As we can see, persistent_ram_zap() is called twice.
> We can avoid this by removing it in ramoops_init_prz(),
> and only call it in persistent_ram_post_init().

However, I think the proposed fix doesn't work the way it should.

There are two prz init paths: ramoops_init_prz() (a single prz) and
ramoops_init_przs (multiple przs). The "dump" and "ftrace" cases use
the latter. In those, there is no call to persistent_ram_zap() if the
buffer is valid.

In other words:

ramoops_init_prz() unconditionally calls persistent_ram_zap(). (And
may call it twice if there is a mismatch of the magic header.)

ramoops_init_przs() only calls persistent_ram_zap() when the magic
header is wrong.

The proposed patch unconditionally zaps all regions, which means we'd
lose "dump" and "ftrace" across the next reboot.

Perhaps we could make it an option to persistent_ram_new()?

-- 
Kees Cook