linux-kernel - Re: [PATCH] printk: fix the check on modifying 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] [day] [month] [year] [list]

Message-ID: <ZQxRqxLFWZ1w45qE@alley>
Date:   Thu, 21 Sep 2023 16:22:35 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     Yun Zhou <yun.zhou@...driver.com>
Cc:     senozhatsky@...omium.org, rostedt@...dmis.org,
        john.ogness@...utronix.de, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] printk: fix the check on modifying printk_devkmsg

First, I am sorry for the late review. So many things have accumulated
during my vacation.

On Thu 2023-08-17 07:47:45, Yun Zhou wrote:
> When we use 'echo' to write a string to printk_devkmsg, it writes '\0' at
> the end. It means lenp is larger then the length of string 1. However,
> sometimes we write data with string length by other way, e.g
>     write(fd, string, strlen(string));
> In this case, the writing will fail.
> 
> Comparing err with string length is able to handle all scenarios.
> 
> Signed-off-by: Yun Zhou <yun.zhou@...driver.com>
> ---
>  kernel/printk/printk.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> index 357a4d18f6387..992872f7b7747 100644
> --- a/kernel/printk/printk.c
> +++ b/kernel/printk/printk.c
> @@ -229,7 +229,7 @@ int devkmsg_sysctl_set_loglvl(struct ctl_table *table, int write,
>  		 * Do not accept an unknown string OR a known string with
>  		 * trailing crap...
>  		 */
> -		if (err < 0 || (err + 1 != *lenp)) {
> +		if (err != strlen(devkmsg_log_str)) {
>  
>  			/* ... and restore old setting. */
>  			devkmsg_log = old;

I see. _proc_do_string() does the following:

_proc_do_string(char *data, int maxlen, int write,
		char *buffer, size_t *lenp, loff_t *ppos)
{
[...]
	if (write) {
[...]
		*ppos += *lenp;
		p = buffer;
		while ((p - buffer) < *lenp && len < maxlen - 1) {
			c = *(p++);
			if (c == 0 || c == '\n')
				break;
			data[len++] = c;
		}
		data[len] = 0;

It means that it proceeds "*lenp" characters but the trailing
'\0' need not match. It might be inserted earlier when
(c == 0 || c == '\n') was true earlier. Or one bite behind when
the trailing '\0' was missing within the *lenp characters.

As a result, the *lenp check is not reliable.

Reviewed-by: Petr Mladek <pmladek@...e.com>