linux-kernel - Re: [PATCH v2 2/5] pstore/platform: pass max

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: <CA+CK2bA1_5e8HCwfMJSsCuN7qjRKYTNY2Q-UbJGGVPAEfZVgaw@mail.gmail.com>
Date:   Wed, 6 May 2020 10:42:50 -0400
From:   Pavel Tatashin <pasha.tatashin@...een.com>
To:     Kees Cook <keescook@...omium.org>
Cc:     James Morris <jmorris@...ei.org>, Sasha Levin <sashal@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        Petr Mladek <pmladek@...e.com>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Steven Rostedt <rostedt@...dmis.org>, anton@...msg.org,
        ccross@...roid.com, Tony Luck <tony.luck@...el.com>,
        robh+dt@...nel.org, devicetree@...r.kernel.org
Subject: Re: [PATCH v2 2/5] pstore/platform: pass max_reason to kmesg dump

On Tue, May 5, 2020 at 5:59 PM Kees Cook <keescook@...omium.org> wrote:
>
> On Tue, May 05, 2020 at 11:45:07AM -0400, Pavel Tatashin wrote:
> > Add a new field to pstore_info that passes information about kmesg dump
> > maximum reason.
> >
> > This allows a finer control of what kmesg dumps are stored on pstore
> > device.
> >
> > Those clients that do not explicitly set this field (keep it equal to 0),
> > get the default behavior: dump only Oops and Panics, and dump everything
> > if printk.always_kmsg_dump is provided.
> >
> > Signed-off-by: Pavel Tatashin <pasha.tatashin@...een.com>
> > ---
> >  fs/pstore/platform.c   | 4 +++-
> >  include/linux/pstore.h | 3 +++
> >  2 files changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> > index 408277ee3cdb..75bf8a43f92a 100644
> > --- a/fs/pstore/platform.c
> > +++ b/fs/pstore/platform.c
> > @@ -602,8 +602,10 @@ int pstore_register(struct pstore_info *psi)
> >       if (pstore_is_mounted())
> >               pstore_get_records(0);
> >
> > -     if (psi->flags & PSTORE_FLAGS_DMESG)
> > +     if (psi->flags & PSTORE_FLAGS_DMESG) {
> > +             pstore_dumper.max_reason = psinfo->max_reason;
> >               pstore_register_kmsg();
> > +     }
>
> I haven't finished reading the whole series carefully, but I think
> something we can do here to make things a bit more user-friendly is to
> do the KMSG_DUMP_UNDEF value here to get us the "all" instead of INT_MAX:
>
>         if (psi->flags & PSTORE_FLAGS_DMESG) {
>                 pstore_dumper.max_reason = psinfo->max_reason;
>                 if (pstore_dumper.max_reason == KMSG_DUMP_UNDEF)
>                         pstore_dumper.max_reason = KMSG_DUMP_MAX;
>                 pstore_register_kmsg();
>         }
>
> That way setting max_reason to 0 without setting dump_oops at all will
> get "all". I think it'll need some tweaks to the next patch.

Hm, but if we change it this way, it will change the behavior for
other backends. With the current version of this patchset,
when psinfo->max_reason is left undefined (0, KMSG_DUMP_UNDEF) -> the
existing behaviour is honored, which means: printk chooses the kmesg
dump level, and users can set dump for all reasons via printk
always_kmsg_dump. This is what efi-pstore, erst, and nvram_64 are
currently doing, and I am not sure we should change that.
However, with the proposed change if the backend specifically sets
max_reason: printk will use it and ignore always_kmsg_dump.

Thank you,
Pasha