lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Y8aEdDxQrQICQtem@alley>
Date:   Tue, 17 Jan 2023 12:20:20 +0100
From:   Petr Mladek <pmladek@...e.com>
To:     Sergey Senozhatsky <senozhatsky@...omium.org>
Cc:     John Ogness <john.ogness@...utronix.de>,
        coverity-bot <keescook@...omium.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        linux-kernel@...r.kernel.org,
        "Gustavo A. R. Silva" <gustavo@...eddedor.com>,
        linux-next@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: Coverity: console_prepend_dropped(): Memory - corruptions

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@...omium.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

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ