Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 21 Jun 2022 14:19:14 -0700
From:   Kees Cook <keescook@chromium.org>
To:     Ard Biesheuvel <ardb@kernel.org>
Cc:     linux-efi@vger.kernel.org, linux-kernel@vger.kernel.org,
        Matthew Garrett <mjg59@srcf.ucam.org>,
        Peter Jones <pjones@redhat.com>,
        Tony Luck <tony.luck@intel.com>,
        Thomas Gleixner <tglx@linutronix.de>,
        Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        Dave Hansen <dave.hansen@linux.intel.com>
Subject: Re: [PATCH v2 1/9] pstore: Don't expose ECC metadata via pstore file
 system
Message-ID: <202206211402.1D0B5A4D7@keescook>
References: <20220621153623.3786960-1-ardb@kernel.org>
 <20220621153623.3786960-2-ardb@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220621153623.3786960-2-ardb@kernel.org>
Precedence: bulk

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