Re: [PATCH] printk: fix the check on modifying printk_devkmsg

[Petr Mladek]

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@xxxxxxxxxxxxx>
> ---
>  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@xxxxxxxx>