Messages in this thread | | | Date | Thu, 21 Sep 2023 16:22:35 +0200 | From | Petr Mladek <> | 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@windriver.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@suse.com>
| |