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:   Mon, 16 Jan 2023 17:35:02 +0100
From:   Petr Mladek <pmladek@suse.com>
To:     Sergey Senozhatsky <senozhatsky@chromium.org>
Cc:     coverity-bot <keescook@chromium.org>,
        John Ogness <john.ogness@linutronix.de>,
        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: <Y8V8tqMJeB7t+rcJ@alley>
References: <202301131544.D9E804CCD@keescook>
 <Y8KAhaiZQOWTcfyF@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <Y8KAhaiZQOWTcfyF@google.com>
Precedence: bulk

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