Re: [PATCH 8/8] pstore/ram: Correctly calculate usable PRZ bytes

From: Kees Cook <keescook@chromium.org>
To: Joel Fernandes <joel@joelfernandes.org>
Cc: LKML <linux-kernel@vger.kernel.org>,
	Anton Vorontsov <anton@enomsg.org>,
	Colin Cross <ccross@android.com>, Tony Luck <tony.luck@intel.com>
Subject: Re: [PATCH 8/8] pstore/ram: Correctly calculate usable PRZ bytes
Date: Fri, 2 Nov 2018 13:00:08 -0700	[thread overview]
Message-ID: <CAGXu5jLcawCKQL9i6poyJu3v3Ru6VcAXqwa5dxKWQL1Bp+Ai_w@mail.gmail.com> (raw)
In-Reply-To: <20181102180111.GA14942@google.com>

On Fri, Nov 2, 2018 at 11:01 AM, Joel Fernandes <joel@joelfernandes.org> wrote:
> On Thu, Nov 01, 2018 at 04:52:00PM -0700, Kees Cook wrote:
>> The actual number of bytes stored in a PRZ is smaller than the
>> bytes requested by platform data, since there is a header on each
>> PRZ. Additionally, if ECC is enabled, there are trailing bytes used
>> as well. Normally this mismatch doesn't matter since PRZs are circular
>> buffers and the leading "overflow" bytes are just thrown away. However, in
>> the case of a compressed record, this rather badly corrupts the results.
>
> Actually this would also mean some data loss for non-compressed records were
> also there before, but is now fixed?

No, it's what I mentioned in the commit log: only the "tail" of any
data was getting stored, which is consistent with the configuration
given. The main problem is that ECC bytes weren't part of the
calculation the ram backend provided to pstore for pstore to pick the
correct amount of bytes to compress.

>> This corruption was visible with "ramoops.mem_size=204800 ramoops.ecc=1".
>> Any stored crashes would not be uncompressable (producing a pstorefs
>> "dmesg-*.enc.z" file), and triggering errors at boot:
>>
>>   [    2.790759] pstore: crypto_comp_decompress failed, ret = -22!
>>
>> Reported-by: Joel Fernandes <joel@joelfernandes.org>
>> Fixes: b0aad7a99c1d ("pstore: Add compression support to pstore")
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>
> Thanks!
> Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>

Thanks!

> Also should this be fixed for other backends or are those good? AFAIR, I saw
> this for EFI too.

It seemed like the other backends were doing it correctly (e.g. erst
removes the header from calculation, etc). I did see that EFI
allocates more memory than needed?

        efi_pstore_info.buf = kmalloc(4096, GFP_KERNEL);
        if (!efi_pstore_info.buf)
                return -ENOMEM;

        efi_pstore_info.bufsize = 1024;

efi_pstore_write() does:

        ret = efivar_entry_set_safe(efi_name, vendor, PSTORE_EFI_ATTRIBUTES,
                              !pstore_cannot_block_path(record->reason),
                              record->size, record->psi->buf);

and efivar_entry_set_safe() says:

 * Returns 0 on success, -ENOSPC if the firmware does not have enough
 * space for set_variable() to succeed, or a converted EFI status code
 * if set_variable() fails.

So I don't see how this could get truncated. (I'm not saying it
didn't... just that I can't see it in an obvious place.)

-Kees

-- 
Kees Cook