linux-kernel - Re: [PATCH printk 1/3] printk: move printk

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]

Date:   Fri, 18 Sep 2020 12:06:02 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     John Ogness <john.ogness@...utronix.de>
Cc:     Sergey Senozhatsky <sergey.senozhatsky.work@...il.com>,
        Sergey Senozhatsky <sergey.senozhatsky@...il.com>,
        Steven Rostedt <rostedt@...dmis.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        kexec@...ts.infradead.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH printk 1/3] printk: move printk_info into separate array

On Thu 2020-09-17 15:22:42, John Ogness wrote:
> The majority of the size of a descriptor is taken up by meta data,
> which is often not of interest to the ringbuffer (for example,
> when performing state checks). Since descriptors are often
> temporarily stored on the stack, keeping their size minimal will
> help reduce stack pressure.
> 
> Rather than embedding the printk_info into the descriptor, create
> a separate printk_info array. The index of a descriptor in the
> descriptor array corresponds to the printk_info with the same
> index in the printk_info array. The rules for validity of a
> printk_info match the existing rules for the data blocks: the
> descriptor must be in a consistent state.

I like this approach.

IMHO, it better separates dict_ring- and printk-specific metadata.

The three printk-specific values (seq, caller_id, len) are still
accessed by the ring buffer code. It is not ideal. But it helps
to hide tricky operations in the ring buffer API, keep the code
more safe and sane. These exceptions are actually better
visible now.

> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -1097,6 +1097,7 @@ static char setup_dict_buf[CONSOLE_EXT_LOG_MAX] __initdata;
>  
>  void __init setup_log_buf(int early)
>  {
> +	struct printk_info *new_infos;
>  	unsigned int new_descs_count;
>  	struct prb_desc *new_descs;
>  	struct printk_info info;
> @@ -1156,6 +1157,17 @@ void __init setup_log_buf(int early)
>  		return;
>  	}
>  
> +	new_descs_size = new_descs_count * sizeof(struct printk_info);

Must be stored into new variable, e.g.  new_infos_size.=

> +	new_infos = memblock_alloc(new_descs_size, LOG_ALIGN);
> +	if (unlikely(!new_infos)) {
> +		pr_err("log_buf_len: %zu info bytes not available\n",
> +		       new_descs_size);
> +		memblock_free(__pa(new_descs), new_log_buf_len);
> +		memblock_free(__pa(new_dict_buf), new_log_buf_len);

The above two calls have wrong size.

The same problem is there also in the error path when new_descs
allocation fail. It might be better to handle this using some
goto err_* tagrets.

Please, fix the old problem in a separate patch.

> +		memblock_free(__pa(new_log_buf), new_log_buf_len);
> +		return;
> +	}
> +
>  	prb_rec_init_rd(&r, &info,
>  			&setup_text_buf[0], sizeof(setup_text_buf),
>  			&setup_dict_buf[0], sizeof(setup_dict_buf));


> --- a/kernel/printk/printk_ringbuffer.c
> +++ b/kernel/printk/printk_ringbuffer.c
> @@ -1726,12 +1762,12 @@ static bool copy_data(struct prb_data_ring *data_ring,
>  	/*
>  	 * Actual cannot be less than expected. It can be more than expected
>  	 * because of the trailing alignment padding.
> +	 *
> +	 * Note that invalid @len values can occur because the caller loads
> +	 * the value during an allowed data race.

I hope that this will not bite us in the future. The fact is that
copying the entire struct printk_info in get_desc() is ugly and
copy_data() has to be careful anyway.

>  	 */
> -	if (WARN_ON_ONCE(data_size < (unsigned int)len)) {
> -		pr_warn_once("wrong data size (%u, expecting >=%hu) for data: %.*s\n",
> -			     data_size, len, data_size, data);
> +	if (data_size < (unsigned int)len)


>  		return false;
> -	}
>  
>  	/* Caller interested in the line count? */
>  	if (line_count)

Otherwise, I like this patch.

Best Regards,
Petr