linux-kernel - Re: [PATCH 2/8] pstore: Do not use crash buffer for decompression

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20181102182454.GB14942@google.com>
Date:   Fri, 2 Nov 2018 11:24:54 -0700
From:   Joel Fernandes <joel@...lfernandes.org>
To:     Kees Cook <keescook@...omium.org>
Cc:     linux-kernel@...r.kernel.org, Anton Vorontsov <anton@...msg.org>,
        Colin Cross <ccross@...roid.com>,
        Tony Luck <tony.luck@...el.com>
Subject: Re: [PATCH 2/8] pstore: Do not use crash buffer for decompression

On Thu, Nov 01, 2018 at 04:51:54PM -0700, Kees Cook wrote:
> The pre-allocated compression buffer used for crash dumping was also
> being used for decompression. This isn't technically safe, since it's
> possible the kernel may attempt a crashdump while pstore is populating the
> pstore filesystem (and performing decompression). Instead, just allocate

Yeah, that would be bad if it happened ;)

> a separate buffer for decompression. Correctness is preferred over
> performance here.
> 
> Signed-off-by: Kees Cook <keescook@...omium.org>
> ---
>  fs/pstore/platform.c | 56 ++++++++++++++++++++------------------------
>  1 file changed, 25 insertions(+), 31 deletions(-)
> 
> diff --git a/fs/pstore/platform.c b/fs/pstore/platform.c
> index b821054ca3ed..8b6028948cf3 100644
> --- a/fs/pstore/platform.c
> +++ b/fs/pstore/platform.c
> @@ -258,20 +258,6 @@ static int pstore_compress(const void *in, void *out,
>  	return outlen;
>  }
>  
> -static int pstore_decompress(void *in, void *out,
> -			     unsigned int inlen, unsigned int outlen)
> -{
> -	int ret;
> -
> -	ret = crypto_comp_decompress(tfm, in, inlen, out, &outlen);
> -	if (ret) {
> -		pr_err("crypto_comp_decompress failed, ret = %d!\n", ret);
> -		return ret;
> -	}
> -
> -	return outlen;
> -}
> -
>  static void allocate_buf_for_compression(void)
>  {
>  	struct crypto_comp *ctx;
> @@ -656,8 +642,9 @@ EXPORT_SYMBOL_GPL(pstore_unregister);
>  
>  static void decompress_record(struct pstore_record *record)
>  {
> +	int ret;
>  	int unzipped_len;

nit: We could get rid of the unzipped_len variable now I think.

> -	char *decompressed;
> +	char *unzipped, *workspace;
>  
>  	if (!record->compressed)
>  		return;
> @@ -668,35 +655,42 @@ static void decompress_record(struct pstore_record *record)
>  		return;
>  	}
>  
> -	/* No compression method has created the common buffer. */
> +	/* Missing compression buffer means compression was not initialized. */
>  	if (!big_oops_buf) {
> -		pr_warn("no decompression buffer allocated\n");
> +		pr_warn("no decompression method initialized!\n");
>  		return;
>  	}
>  
> -	unzipped_len = pstore_decompress(record->buf, big_oops_buf,
> -					 record->size, big_oops_buf_sz);
> -	if (unzipped_len <= 0) {
> -		pr_err("decompression failed: %d\n", unzipped_len);
> +	/* Allocate enough space to hold max decompression and ECC. */
> +	unzipped_len = big_oops_buf_sz;
> +	workspace = kmalloc(unzipped_len + record->ecc_notice_size,

Should tihs be unzipped_len + record->ecc_notice_size + 1. The extra byte
being for the NULL character of the ecc notice?

This occurred to me when I saw the + 1 in ram.c. It could be better to just
abstract the size as a macro.

> +			    GFP_KERNEL);
> +	if (!workspace)
>  		return;
> -	}
>  
> -	/* Build new buffer for decompressed contents. */
> -	decompressed = kmalloc(unzipped_len + record->ecc_notice_size,
> -			       GFP_KERNEL);
> -	if (!decompressed) {
> -		pr_err("decompression ran out of memory\n");
> +	/* After decompression "unzipped_len" is almost certainly smaller. */
> +	ret = crypto_comp_decompress(tfm, record->buf, record->size,
> +					  workspace, &unzipped_len);
> +	if (ret) {
> +		pr_err("crypto_comp_decompress failed, ret = %d!\n", ret);
> +		kfree(workspace);
>  		return;
>  	}
> -	memcpy(decompressed, big_oops_buf, unzipped_len);
>  
>  	/* Append ECC notice to decompressed buffer. */
> -	memcpy(decompressed + unzipped_len, record->buf + record->size,
> +	memcpy(workspace + unzipped_len, record->buf + record->size,
>  	       record->ecc_notice_size);
>  
> -	/* Swap out compresed contents with decompressed contents. */
> +	/* Copy decompressed contents into an minimum-sized allocation. */
> +	unzipped = kmemdup(workspace, unzipped_len + record->ecc_notice_size,
> +			   GFP_KERNEL);
> +	kfree(workspace);
> +	if (!unzipped)
> +		return;
> +
> +	/* Swap out compressed contents with decompressed contents. */
>  	kfree(record->buf);
> -	record->buf = decompressed;
> +	record->buf = unzipped;

Rest of it LGTM, thanks!

 - Joel