Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sun, 4 Nov 2018 20:42:17 -0800
From:   Joel Fernandes <joel@joelfernandes.org>
To:     Kees Cook <keescook@chromium.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
Message-ID: <20181105044217.GB56850@google.com>
References: <20181101235200.28584-1-keescook@chromium.org>
 <20181101235200.28584-9-keescook@chromium.org>
 <20181102180111.GA14942@google.com>
 <CAGXu5jLcawCKQL9i6poyJu3v3Ru6VcAXqwa5dxKWQL1Bp+Ai_w@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAGXu5jLcawCKQL9i6poyJu3v3Ru6VcAXqwa5dxKWQL1Bp+Ai_w@mail.gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Kees,

On Fri, Nov 02, 2018 at 01:00:08PM -0700, Kees Cook wrote:
[..] 
> >> 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.)


So I *think* the issue is that the pstore had old compressed dmesg dumps in
EFI on my laptop, after the crypto layer in the kernel probably changed
enough to make the data non-decompressable, if that makes any sense. So older
code did compression in certain way, and newer code is doing the decompress,
or something like that.

I did some sysrq crashes on my laptop and the deflate decompress is working
fine with pstore+EFI. Its interesting I see some .enc.z files which fail to
decompress (which are older ones), and others which are decompressed fine
(the newer ones) ;-)

Dumping the magic bytes of the non decompressable .enc.z files, I get this
which shows a valid zlib compressed header:

Something like:
48 89 85 54 4d 6f 1a 31

The 0b1000 in the first byte means it is "deflate". The file tool indeed
successfully shows "zlib compressed data" and I did the math for the header
and it is indeed valid. So I don't think the data is insane. The buffer has
enough room because even the very small dumps are not decompressable.

At this point we can park this issue I guess, but a scenario that is still
broken is:
Say someone crashes the system on compress algo X and then recompiles with
compress algo Y, then the decompress would fail no?

One way to fix that is to store the comrpession method in buffer as well,
then initialize all algorithms at boot and choose the right one in the
buffer ideally. Otherwise atleast we should print a message saying "buffer is
encoded with algo X but compression selected is Y" or something. But I agree
its a very low priority "doctor it hurts if I do this" kind of issue :)

Anyway, let me know what you think :)

thanks,

- Joel