re: pstore/ram: Add ftrace messages handling

re: pstore/ram: Add ftrace messages handling

[Dan Carpenter]

Hello Anton Vorontsov,

This is a semi-automatic email about new static checker warnings.

The patch a694d1b5916a: "pstore/ram: Add ftrace messages handling" 
from Jul 9, 2012, leads to the following Smatch complaint:

fs/pstore/ram.c:423 ramoops_probe()
	 error: we previously assumed 'cxt->cprz' could be null (see line 408)

fs/pstore/ram.c
   407	
   408		if (!cxt->przs && !cxt->cprz && !cxt->fprz) {
                                  ^^^^^^^^^^

Checked here.

   409			pr_err("memory size too small, minimum is %lu\n",
   410				cxt->console_size + cxt->record_size +
   411				cxt->ftrace_size);
   412			goto fail_cnt;
   413		}
   414	
   415		cxt->pstore.data = cxt;
   416		/*
   417		 * Console can handle any buffer size, so prefer dumps buffer
   418		 * size since usually it is smaller.
   419		 */
   420		if (cxt->przs)
   421			cxt->pstore.bufsize = cxt->przs[0]->buffer_size;
   422		else
   423			cxt->pstore.bufsize = cxt->cprz->buffer_size;
                                              ^^^^^^^^^
Dereferenced here.  What about if only "cxt->fprz" is non-NULL?

Also these are crap variable names, "przs" and "cprz" look so similar.
It makes my head hurt to keep them appart.

   424		cxt->pstore.buf = kmalloc(cxt->pstore.bufsize, GFP_KERNEL);
   425		spin_lock_init(&cxt->pstore.buf_lock);

regards,
dan carpenter

Re: pstore/ram: Add ftrace messages handling

[Anton Vorontsov]

Hi Dan,

On Thu, Jul 19, 2012 at 05:28:56PM +0300, Dan Carpenter wrote:
> The patch a694d1b5916a: "pstore/ram: Add ftrace messages handling" 
> from Jul 9, 2012, leads to the following Smatch complaint:

A nice tool. The homepage of Smatch doesn't explicitly say that, so
I have to ask: is it a complete superset of sparse (i.e. does it
produce all the warnings that the pure sparse can produce)?
If so, I'll probably switch to it from the vanilla sparse.

Thanks,

-- 
Anton Vorontsov
Email: cbouatmailru@gmail.com

[PATCH] pstore/ram: Fix possible NULL dereference

[Anton Vorontsov]

We can dereference 'cxt->cprz' if console and dump logging are disabled
(which is unlikely, but still possible to do). This patch fixes the issue
by changing the code so that we don't dereference przs at all, we can
just calculate bufsize from console_size and record_size values.

Plus, while at it, the patch improves the buffer size calculation.

After Kay's printk rework, we know the optimal buffer size for console
logging -- it is LOG_LINE_MAX (defined privately in printk.c). Previously,
if only console logging was enabled, we would allocate unnecessary large
buffer in pstore, while we only need LOG_LINE_MAX. (Pstore console logging
is still capable of handling buffers > LOG_LINE_MAX, it will just do
multiple calls to psinfo->write).

Note that I don't export the constant, since we will do even a better
thing soon: we will switch console logging to a new write_buf API, which
will eliminate the need for the additional buffer; and so we won't need
the constant.

Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org>
---
 fs/pstore/ram.c |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/fs/pstore/ram.c b/fs/pstore/ram.c
index b86b2b7..c34fccf 100644
--- a/fs/pstore/ram.c
+++ b/fs/pstore/ram.c
@@ -414,13 +414,14 @@ static int __devinit ramoops_probe(struct platform_device *pdev)
 
 	cxt->pstore.data = cxt;
 	/*
-	 * Console can handle any buffer size, so prefer dumps buffer
-	 * size since usually it is smaller.
+	 * Console can handle any buffer size, so prefer LOG_LINE_MAX. If we
+	 * have to handle dumps, we must have at least record_size buffer. And
+	 * for ftrace, bufsize is irrelevant (if bufsize is 0, buf will be
+	 * ZERO_SIZE_PTR).
 	 */
-	if (cxt->przs)
-		cxt->pstore.bufsize = cxt->przs[0]->buffer_size;
-	else
-		cxt->pstore.bufsize = cxt->cprz->buffer_size;
+	if (cxt->console_size)
+		cxt->pstore.bufsize = 1024; /* LOG_LINE_MAX */
+	cxt->pstore.bufsize = max(cxt->record_size, cxt->pstore.bufsize);
 	cxt->pstore.buf = kmalloc(cxt->pstore.bufsize, GFP_KERNEL);
 	spin_lock_init(&cxt->pstore.buf_lock);
 	if (!cxt->pstore.buf) {
-- 
1.7.10.4

Re: pstore/ram: Add ftrace messages handling

[Dan Carpenter]

On Thu, Jul 19, 2012 at 04:20:32PM -0700, Anton Vorontsov wrote:
> Hi Dan,
> 
> On Thu, Jul 19, 2012 at 05:28:56PM +0300, Dan Carpenter wrote:
> > The patch a694d1b5916a: "pstore/ram: Add ftrace messages handling" 
> > from Jul 9, 2012, leads to the following Smatch complaint:
> 
> A nice tool. The homepage of Smatch doesn't explicitly say that, so
> I have to ask: is it a complete superset of sparse (i.e. does it
> produce all the warnings that the pure sparse can produce)?
> If so, I'll probably switch to it from the vanilla sparse.
> 

No.  It just uses Sparse as a parser.

regards,
dan carpenter

Re: [PATCH] pstore/ram: Fix possible NULL dereference

[Kees Cook]

On Thu, Jul 19, 2012 at 4:43 PM, Anton Vorontsov
<anton.vorontsov@linaro.org> wrote:
> We can dereference 'cxt->cprz' if console and dump logging are disabled
> (which is unlikely, but still possible to do). This patch fixes the issue
> by changing the code so that we don't dereference przs at all, we can
> just calculate bufsize from console_size and record_size values.
>
> Plus, while at it, the patch improves the buffer size calculation.
>
> After Kay's printk rework, we know the optimal buffer size for console
> logging -- it is LOG_LINE_MAX (defined privately in printk.c). Previously,
> if only console logging was enabled, we would allocate unnecessary large
> buffer in pstore, while we only need LOG_LINE_MAX. (Pstore console logging
> is still capable of handling buffers > LOG_LINE_MAX, it will just do
> multiple calls to psinfo->write).
>
> Note that I don't export the constant, since we will do even a better
> thing soon: we will switch console logging to a new write_buf API, which
> will eliminate the need for the additional buffer; and so we won't need
> the constant.
>
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Anton Vorontsov <anton.vorontsov@linaro.org>

Acked-by: Kees Cook <keescook@chromium.org>

-Kees

-- 
Kees Cook
Chrome OS Security