Coverity: console_prepend_dropped(): Memory - corruptions
Hello!
This is an experimental semi-automated report about issues detected by Coverity from a scan of next-20230113 as part of the linux-next scan project:
https://scan.coverity.com/projects/linux-next-weekly-scan
You're getting this email because you were associated with the identified lines of code (noted below) that were touched by commits:
Wed Jan 11 15:35:11 2023 +0100
c4fcc617e148 ("printk: introduce console_prepend_dropped() for dropped messages")
Coverity reported the following:
*** CID 1530570: Memory - corruptions (OVERRUN) kernel/printk/printk.c:2738 in console_prepend_dropped()
2732 /* Truncate the message, but keep it terminated. */ 2733 pmsg->outbuf_len = outbuf_sz - (len + 1); 2734 outbuf[pmsg->outbuf_len] = 0; 2735 }
2736
2737 memmove(outbuf + len, outbuf, pmsg->outbuf_len + 1);
vvv CID 1530570: Memory - corruptions (OVERRUN)
vvv Overrunning buffer pointed to by "scratchbuf" of 1024 bytes by passing it to a function which accesses it at byte offset 1998 using argument "len" (which evaluates to 1999). [Note: The source code implementation of the function has been overridden by a builtin model.]
2738 memcpy(outbuf, scratchbuf, len); 2739 pmsg->outbuf_len += len; 2740 } 2741 #else 2742 #define console_prepend_dropped(pmsg, dropped) 2743 #endif /* CONFIG_PRINTK */
If this is a false positive, please let us know so we can mark it as such, or teach the Coverity rules to be smarter. If not, please make sure fixes get into linux-next. :) For patches fixing this, please include these lines (but double-check the "Fixes" first):
Addresses-Coverity-ID: 1530570 ("Memory - corruptions")
Thanks for your attention!
Human notes from Kees:
I'm not sure how it got 1998, but I do see that snprintf() should probably be scnprintf(), otherwise "len" might be a lie (i.e. it'll hold what it WANTED to write, rather than what it actually wrote).
Signature · 1 line
Coverity-bot
Quoted 16 lines · coverity-bot
*** CID 1530570: Memory - corruptions (OVERRUN)
kernel/printk/printk.c:2738inconsole_prepend_dropped()2732 /* Truncate the message, but keep it terminated. */ 2733 pmsg->outbuf_len = outbuf_sz - (len + 1); 2734 outbuf[pmsg->outbuf_len] = 0; 2735 }2736
2737memmove(outbuf + len, outbuf, pmsg->outbuf_len + 1);
vvv CID 1530570: Memory - corruptions (OVERRUN)
vvv Overrunning buffer pointed to by "scratchbuf" of 1024 bytes by passing it to a function which accesses it at byte offset 1998 using argument "len" (which evaluates to 1999). [Note: The source code implementation of the function has been overridden by a builtin model.]2738 memcpy(outbuf, scratchbuf, len); 2739 pmsg->outbuf_len += len; 2740 } 2741 #else 2742 #define console_prepend_dropped(pmsg, dropped) 2743 #endif /* CONFIG_PRINTK */
[..]
Quoted 5 lines
Human notes from Kees:
I'm not sure how it got 1998, but I do see that
snprintf()should probably bescnprintf(), otherwise "len" might be a lie (i.e. it'll hold what it WANTED to write, rather than what it actually wrote).
Cannot imagine how "** %lu printk messages dropped **\n" can expand into 1998 bytes. Does coverity have a "verbose" mode?
Quoted 18 lines · Sergey Senozhatsky
Quoted 16 lines · coverity-bot
*** CID 1530570: Memory - corruptions (OVERRUN)
kernel/printk/printk.c:2738inconsole_prepend_dropped()2732 /* Truncate the message, but keep it terminated. */ 2733 pmsg->outbuf_len = outbuf_sz - (len + 1); 2734 outbuf[pmsg->outbuf_len] = 0; 2735 }2736
2737memmove(outbuf + len, outbuf, pmsg->outbuf_len + 1);
vvv CID 1530570: Memory - corruptions (OVERRUN)
vvv Overrunning buffer pointed to by "scratchbuf" of 1024 bytes by passing it to a function which accesses it at byte offset 1998 using argument "len" (which evaluates to 1999). [Note: The source code implementation of the function has been overridden by a builtin model.]2738 memcpy(outbuf, scratchbuf, len); 2739 pmsg->outbuf_len += len; 2740 } 2741 #else 2742 #define console_prepend_dropped(pmsg, dropped) 2743 #endif /* CONFIG_PRINTK */[..]
Quoted 8 lines
Quoted 5 lines
Human notes from Kees:
I'm not sure how it got 1998, but I do see that
snprintf()should probably bescnprintf(), otherwise "len" might be a lie (i.e. it'll hold what it WANTED to write, rather than what it actually wrote).Cannot imagine how "** %lu printk messages dropped **\n" can expand into 1998 bytes. Does coverity have a "verbose" mode?
I guess that coverity tries to pass some random string that is longer than the provided buffer.
The code might be safe with the current size of the buffer and the string. But it is true that the following is wrong:
len = snprintf(scratchbuf, scratchbuf_sz, "** %lu printk messages dropped **\n", dropped);
As Kees pointed out in the human comment, we should use scnprintf() that will return the really written length of the string that fits into the buffer.
I am going to send a patch.
Best Regards,
Petr
Quoted 2 lines · Petr Mladek
I am going to send a patch.
Sure, sounds good.
Quoted 5 lines
The code might be safe with the current size of the buffer and the string. But it is true that the following is wrong:
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?
Quoted 10 lines · Sergey Senozhatsky
Quoted 2 lines · Petr Mladek
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?
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.
John Ogness
Quoted 10 lines · John Ogness
Quoted 9 lines · Sergey Senozhatsky
Quoted 2 lines · Petr Mladek
len = snprintf(scratchbuf, scratchbuf_sz, "** %lu printk messages dropped **\n", dropped);Wouldn't
if (WARN_ON_ONCE(len + PRINTK_PREFIX_MAX >= outbuf_sz)) return;
Quoted 6 lines
Quoted 1 line
prevent us from doing something harmful?
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.
Quoted 6 lines · Sergey Senozhatsky
Quoted 5 lines · John Ogness
Quoted 4 lines · Sergey Senozhatsky
Quoted 2 lines · Petr Mladek
len = snprintf(scratchbuf, scratchbuf_sz, "** %lu printk messages dropped **\n", dropped);
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
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()".
Quoted 10 lines
Quoted 4 lines
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 inkernel/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
Quoted 9 lines · Petr Mladek
Quoted 5 lines
Oh, yes, I agree that
snprintf()should be replaced. Maybe we can go even a bit furhter and replace all snprintf()-s inkernel/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()invprintk_store()is correct.
Oh, right, that's an excellebt point.