linux-hardening - Re: [PATCH 1/2] pstore: Remove worst-case compression size logic

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: <20230704180711.GC1851@sol.localdomain>
Date:   Tue, 4 Jul 2023 11:07:11 -0700
From:   Eric Biggers <ebiggers@...nel.org>
To:     Ard Biesheuvel <ardb@...nel.org>
Cc:     linux-hardening@...r.kernel.org, Kees Cook <keescook@...omium.org>,
        "Guilherme G. Piccoli" <gpiccoli@...lia.com>
Subject: Re: [PATCH 1/2] pstore: Remove worst-case compression size logic

On Tue, Jul 04, 2023 at 03:52:10PM +0200, Ard Biesheuvel wrote:
> From: Kees Cook <keescook@...omium.org>
> 
> The worst case compression size gives an upper bound for how much the
> data might inadvertently *grow* due to encapsulation overhead if the
> input is not compressible at all.
> 
> The kernel log is ASCII text so it should generally compress rather
> well. This means that the probability that the kernel log grows beyond
> the uncompressed size after compression is astronomically low, and in
> such cases (i.e., dmesg filled with perfect entropy) we won't be able to
> make sense of it anyway.
> 
> So let's just drop this logic, and use the uncompressed size as the
> worst case instead.
> 
> Co-developed-by: Kees Cook <keescook@...omium.org>
> Signed-off-by: Kees Cook <keescook@...omium.org>
> Signed-off-by: Ard Biesheuvel <ardb@...nel.org>

I think the reasoning about the kernel log being low-entropy, which is a bit
hand-wavy, is entirely unnecessary.  pstore records include a flag that
indicates whether they are compressed or not.  Therefore, any record that would
compress to more than its original size can and should be stored uncompressed.
There's nothing more to it than that.

>  static int pstore_compress(const void *in, void *out,
>  			   unsigned int inlen, unsigned int outlen)
>  {
> @@ -291,36 +178,31 @@ static void allocate_buf_for_compression(void)
>  	char *buf;
>  
>  	/* Skip if not built-in or compression backend not selected yet. */
> -	if (!IS_ENABLED(CONFIG_PSTORE_COMPRESS) || !zbackend)
> +	if (!IS_ENABLED(CONFIG_PSTORE_COMPRESS) || !compress)
>  		return;
>  
>  	/* Skip if no pstore backend yet or compression init already done. */
>  	if (!psinfo || tfm)
>  		return;
>  
> -	if (!crypto_has_comp(zbackend->name, 0, 0)) {
> -		pr_err("Unknown compression: %s\n", zbackend->name);
> -		return;
> -	}
> -
> -	size = zbackend->zbufsize(psinfo->bufsize);
> -	if (size <= 0) {
> -		pr_err("Invalid compression size for %s: %d\n",
> -		       zbackend->name, size);
> +	if (!crypto_has_comp(compress, 0, 0)) {
> +		pr_err("Unknown compression: %s\n", compress);
>  		return;
>  	}
>  
> +	/* Worst-case compression should never be more than uncompressed. */
> +	size = psinfo->bufsize;
>  	buf = kmalloc(size, GFP_KERNEL);
>  	if (!buf) {
>  		pr_err("Failed %d byte compression buffer allocation for: %s\n",
> -		       size, zbackend->name);
> +		       size, compress);
>  		return;
>  	}

The local variable 'size' should be removed, and psinfo->bufsize used directly.

> @@ -330,7 +212,7 @@ static void allocate_buf_for_compression(void)
>  	big_oops_buf_sz = size;

The static variable 'big_oops_buf_sz' should be removed, as it is redundant with
psinfo->bufsize.

>		if (big_oops_buf) {
>			dst = big_oops_buf;
>			dst_size = big_oops_buf_sz;
>		} else {
>			dst = psinfo->buf;
>			dst_size = psinfo->bufsize;
>		}

This can be simplified to:

		if (big_oops_buf)
			dst = big_oops_buf;
		else
			dst = psinfo->buf;
		dst_size = psinfo->bufsize;

>	unzipped_len = big_oops_buf_sz;
>	workspace = kmalloc(unzipped_len + record->ecc_notice_size,
>			    GFP_KERNEL);
>	if (!workspace)
>		return;

This can be simplified to:

	workspace = kmalloc(psinfo->bufsize + record->ecc_notice_size,
			    GFP_KERNEL);
	if (!workspace)
		return;

> /*
>  * Called when compression fails, since the printk buffer
>  * would be fetched for compression calling it again when
>  * compression fails would have moved the iterator of
>  * printk buffer which results in fetching old contents.
>  * Copy the recent messages from big_oops_buf to psinfo->buf
>  */
> static size_t copy_kmsg_to_buffer(int hsize, size_t len)
> {
> 	size_t total_len;
> 	size_t diff;
> 
> 	total_len = hsize + len;
> 
> 	if (total_len > psinfo->bufsize) {
> 		diff = total_len - psinfo->bufsize + hsize;
> 		memcpy(psinfo->buf, big_oops_buf, hsize);
> 		memcpy(psinfo->buf + hsize, big_oops_buf + diff,
> 					psinfo->bufsize - hsize);
> 		total_len = psinfo->bufsize;
> 	} else
> 		memcpy(psinfo->buf, big_oops_buf, total_len);
> 
> 	return total_len;
> }

This patch makes the 'total_len > psinfo->bufsize' case in the above function
unreachable.  That function should be removed, and its caller should just do:

			zipped_len = pstore_compress(dst, psinfo->buf,
						header_size + dump_size,
						psinfo->bufsize);

			if (zipped_len > 0) {
				record.compressed = true;
				record.size = zipped_len;
			} else {
				memcpy(psinfo->buf, dst,
				       header_size + dump_size);
				record.size = header_size + dump_size;
			}


- Eric