On Tue 2023-01-17 16:51:49, Sergey Senozhatsky wrote: > On (23/01/17 08:16), John Ogness wrote: > > On 2023-01-17, Sergey Senozhatsky <senozhatsky@xxxxxxxxxxxx> wrote: > > > On (23/01/16 17:35), Petr Mladek wrote: > > >> len = snprintf(scratchbuf, scratchbuf_sz, > > >> "** %lu printk messages dropped **\n", dropped); > > > > > > Wouldn't > > > > > > if (WARN_ON_ONCE(len + PRINTK_PREFIX_MAX >= outbuf_sz)) > > > return; > > > > > > prevent us from doing something harmful? The problem is that it compares outbuf_sz that is bigger than scratchbuf. The above check should prevent crash in: memmove(outbuf + len, outbuf, pmsg->outbuf_len + 1); But it would not prevent out-of-bound access to scratchbuf in: memcpy(outbuf, scratchbuf, len); That said, the coverity report is pretty confusing. It is below the memmove() so that it looks like the memmove() is dangerous. But it talks about "scratchbuf" so that it probably describes the real problem in "memcpy()". > > Sure. But @0len is supposed to contain the number of bytes in > > @scratchbuf, which theoretically it does not. snprintf() is the wrong > > function to use here, even if there is not real danger in this > > situation. > > Oh, yes, I agree that snprintf() should be replaced. Maybe we can go > even a bit furhter and replace all snprintf()-s in kernel/printk/* > (well, in a similar fashion, just in case). I'm just trying to understand > what type of assumptions does coverity make here and so far everything > looks rather peculiar. Note that we sometimes need snprintf() to compute the needed size of the buffer. For example, vsnprintf() in vprintk_store() is correct. It looks to me that snprintf() in console_prepend_dropped() is the only real problem. Well, it would be nice to replace the few sprintf() calls. They look safe because the size of the output is limited by the printf format but... Best Regards, Petr
