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:   Fri, 2 Nov 2018 11:01:11 -0700
From:   Joel Fernandes <joel@joelfernandes.org>
To:     Kees Cook <keescook@chromium.org>
Cc:     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: <20181102180111.GA14942@google.com>
References: <20181101235200.28584-1-keescook@chromium.org>
 <20181101235200.28584-9-keescook@chromium.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20181101235200.28584-9-keescook@chromium.org>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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?

> 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>

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

- Joel


> ---
>  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 25bede911809..10ac4d23c423 100644
> --- a/fs/pstore/ram.c
> +++ b/fs/pstore/ram.c
> @@ -814,17 +814,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 3549f2ba865c..f46e5df76b58 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
>