linux-kernel - Re: [PATCH v2 1/9] pstore: Don't expose ECC metadata via pstore file system

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: <202206211402.1D0B5A4D7@keescook>
Date:   Tue, 21 Jun 2022 14:19:14 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Ard Biesheuvel <ardb@...nel.org>
Cc:     linux-efi@...r.kernel.org, linux-kernel@...r.kernel.org,
        Matthew Garrett <mjg59@...f.ucam.org>,
        Peter Jones <pjones@...hat.com>,
        Tony Luck <tony.luck@...el.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>
Subject: Re: [PATCH v2 1/9] pstore: Don't expose ECC metadata via pstore file
 system

On Tue, Jun 21, 2022 at 05:36:15PM +0200, Ard Biesheuvel wrote:
> If a pstore record has its ecc_notice_size field set to >0, it means the
> record's buffer has that many additional bytes appended to the end that
> carry backend specific metadata, typically used for error correction.
> 
> Given that this is backend specific, and that user space cannot really
> make sense of this metadata anyway, let's not expose it via the pstore
> filesystem.
> 
> Signed-off-by: Ard Biesheuvel <ardb@...nel.org>

"ecc_notice_size" is actually describing the length of the string
generated and appended by persistent_ram_ecc_string().

I've been bothered by this string, though, as it confuses what was
actually stored with additional lines. "Why does every entry end with a
string about ECC?"

I think it's more sensible to show to userspace the record "as stored". We
already prepend some chunking details when a panic write may split the
dump across multiple records, so if anyone needs this IN the userspace
file contents again, it could move there.

I'd rather ECC status be reported at boot, really. Given that nothing I
can find[1] parses the ECC notice string, I think it'd be fine to just
remove it from the string buffer entirely.

-Kees

[1] https://codesearch.debian.net/search?q=Corrected+bytes

-- 
Kees Cook