Coverity: console_prepend_dropped(): Memory - corruptions

Coverity: console_prepend_dropped(): Memory - corruptions

[coverity-bot]

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):

Reported-by: coverity-bot <keescook+coverity-bot@chromium.org>
Addresses-Coverity-ID: 1530570 ("Memory - corruptions")
Fixes: c4fcc617e148 ("printk: introduce console_prepend_dropped() for dropped messages")

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).

-- 
Coverity-bot

Re: Coverity: console_prepend_dropped(): Memory - corruptions

[Sergey Senozhatsky]

On (23/01/13 15:46), coverity-bot wrote:
> *** 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 */
[..]
> 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).

Cannot imagine how "** %lu printk messages dropped **\n" can expand into
1998 bytes. Does coverity have a "verbose" mode?

Re: Coverity: console_prepend_dropped(): Memory - corruptions

[Petr Mladek]

On Sat 2023-01-14 19:14:29, Sergey Senozhatsky wrote:
> On (23/01/13 15:46), coverity-bot wrote:
> > *** 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 */
> [..]
> > 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).
> 
> 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

Re: Coverity: console_prepend_dropped(): Memory - corruptions

[Sergey Senozhatsky]

On (23/01/16 17:35), Petr Mladek wrote:
> 
> I am going to send a patch.

Sure, sounds good.

> 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?

Re: Coverity: console_prepend_dropped(): Memory - corruptions

[John Ogness]

On 2023-01-17, Sergey Senozhatsky <senozhatsky@chromium.org> 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?

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

Re: Coverity: console_prepend_dropped(): Memory - corruptions

[Sergey Senozhatsky]

On (23/01/17 08:16), John Ogness wrote:
> On 2023-01-17, Sergey Senozhatsky <senozhatsky@chromium.org> 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?
> 
> 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.

Re: Coverity: console_prepend_dropped(): Memory - corruptions

[Petr Mladek]

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@chromium.org> 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

Re: Coverity: console_prepend_dropped(): Memory - corruptions

[Sergey Senozhatsky]

On (23/01/17 12:20), Petr Mladek wrote:
> > 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.

Oh, right, that's an excellebt point.