Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 17 Jan 2023 12:20:20 +0100
From:   Petr Mladek <pmladek@suse.com>
To:     Sergey Senozhatsky <senozhatsky@chromium.org>
Cc:     John Ogness <john.ogness@linutronix.de>,
        coverity-bot <keescook@chromium.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        linux-kernel@vger.kernel.org,
        "Gustavo A. R. Silva" <gustavo@embeddedor.com>,
        linux-next@vger.kernel.org, linux-hardening@vger.kernel.org
Subject: Re: Coverity: console_prepend_dropped(): Memory - corruptions
Message-ID: <Y8aEdDxQrQICQtem@alley>
References: <202301131544.D9E804CCD@keescook>
 <Y8KAhaiZQOWTcfyF@google.com>
 <Y8V8tqMJeB7t+rcJ@alley>
 <Y8YRBo7ZmtzWT4J1@google.com>
 <877cxl3abr.fsf@jogness.linutronix.de>
 <Y8ZTlVX3HQUVkU13@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y8ZTlVX3HQUVkU13@google.com>
Precedence: bulk

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