Re: [PATCH AUTOSEL 4.19 104/123] pstore/ram: Correctly calculate usable PRZ bytes

[Kees Cook]

On Wed, Dec 5, 2018 at 1:41 AM Sasha Levin <sashal@xxxxxxxxxx> wrote:
>
> From: Kees Cook <keescook@xxxxxxxxxxxx>
>
> [ Upstream commit 89d328f637b9904b6d4c9af73c8a608b8dd4d6f8 ]
>
> 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.
>
> 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!
>
> Backporting this depends on commit 70ad35db3321 ("pstore: Convert console
> write to use ->write_buf")

Please note the above. If this gets backported, this one is needed too.

-Kees

>
> Reported-by: Joel Fernandes <joel@xxxxxxxxxxxxxxxxx>
> Fixes: b0aad7a99c1d ("pstore: Add compression support to pstore")
> Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
> Reviewed-by: Joel Fernandes (Google) <joel@xxxxxxxxxxxxxxxxx>
> Signed-off-by: Sasha Levin <sashal@xxxxxxxxxx>
> ---
>  fs/pstore/ram.c        | 15 ++++++---------
>  include/linux/pstore.h |  5 ++++-
>  2 files changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
> index f4fd2e72add4..03cd59375abe 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -806,17 +806,14 @@ static int ramoops_probe(struct platform_device *pdev)
>
>         cxt->pstore.data = cxt;
>         /*
> -        * Console can handle any buffer size, so prefer LOG_LINE_MAX. If we
> -        * have to handle dumps, we must have at least record_size buffer. And
> -        * for ftrace, bufsize is irrelevant (if bufsize is 0, buf will be
> -        * ZERO_SIZE_PTR).
> +        * Since bufsize is only used for dmesg crash dumps, it
> +        * must match the size of the dprz record (after PRZ header
> +        * and ECC bytes have been accounted for).
>          */
> -       if (cxt->console_size)
> -               cxt->pstore.bufsize = 1024; /* LOG_LINE_MAX */
> -       cxt->pstore.bufsize = max(cxt->record_size, cxt->pstore.bufsize);
> -       cxt->pstore.buf = kmalloc(cxt->pstore.bufsize, GFP_KERNEL);
> +       cxt->pstore.bufsize = cxt->dprzs[0]->buffer_size;
> +       cxt->pstore.buf = kzalloc(cxt->pstore.bufsize, GFP_KERNEL);
>         if (!cxt->pstore.buf) {
> -               pr_err("cannot allocate pstore buffer\n");
> +               pr_err("cannot allocate pstore crash dump buffer\n");
>                 err = -ENOMEM;
>                 goto fail_clear;
>         }
> diff --git a/include/linux/pstore.h b/include/linux/pstore.h
> index a15bc4d48752..30fcec375a3a 100644
> --- a/include/linux/pstore.h
> +++ b/include/linux/pstore.h
> @@ -90,7 +90,10 @@ struct pstore_record {
>   *
>   * @buf_lock:  spinlock to serialize access to @buf
>   * @buf:       preallocated crash dump buffer
> - * @bufsize:   size of @buf available for crash dump writes
> + * @bufsize:   size of @buf available for crash dump bytes (must match
> + *             smallest number of bytes available for writing to a
> + *             backend entry, since compressed bytes don't take kindly
> + *             to being truncated)
>   *
>   * @read_mutex:        serializes @open, @read, @close, and @erase callbacks
>   * @flags:     bitfield of frontends the backend can accept writes for
> --
> 2.17.1
>


-- 
Kees Cook