LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Josh Poimboeuf <jpoimboe@redhat.com>
To: Dan Carpenter <dan.carpenter@oracle.com>
Cc: linux-kernel@vger.kernel.org,
	Peter Zijlstra <peterz@infradead.org>,
	"Gustavo A. R. Silva" <gustavo@embeddedor.com>
Subject: Re: Smatch check for Spectre stuff
Date: Fri, 8 Jun 2018 11:12:19 -0500	[thread overview]
Message-ID: <20180608161219.q3lwvlydvs4l2gxa@treble> (raw)
In-Reply-To: <20180419051510.GA21898@mwanda>

On Thu, Apr 19, 2018 at 08:15:10AM +0300, Dan Carpenter wrote:
> Several people have asked me to write this and I think one person was
> maybe working on writing it themselves...
> 
> The point of this check is to find place which might be vulnerable to
> the Spectre vulnerability.  In the kernel we have the array_index_nospec()
> macro which turns off speculation.  There are fewer than 10 places where
> it's used.  Meanwhile this check complains about 800 places where maybe
> it could be used.  Probably the 100x difference means there is something
> that I haven't understood...
> 
> What the test does is it looks at array accesses where the user controls
> the offset.  It asks "is this a read?" and have we used the
> array_index_nospec() macro?  If the answers are yes, and no respectively
> then print a warning.
> 
> http://repo.or.cz/smatch.git/blob/HEAD:/check_spectre.c
> 
> The other thing is that speculation probably goes to 200 instructions
> ahead at most.  But the Smatch doesn't have any concept of CPU
> instructions.  I've marked the offsets which were recently compared to
> something as "local cap" because they were probably compared to the
> array limit.  Those are maybe more likely to be under the 200 CPU
> instruction count.
> 
> This obviously a first draft.
> 
> What would help me, is maybe people could tell me why there are so many
> false positives.  Saying "the lower level checks for that" is not
> helpful but if you could tell me the exact function name where the check
> is that helps a lot...
> 
> I have included the warnings from yesterday's linux-next.

Hi Dan,

Smatch is amazing.  I've been going through a lot of the results.  The
false positive rate is much lower than I expected.

I have a few questions/comments.

1) I've noticed a common pattern for many of the false positives.
   Smatch doesn't seem to detect when the code masks off the array index
   to ensure that it's safe.

   For example:

   > ./include/linux/mmzone.h:1161 __nr_to_section() warn: potential spectre issue 'mem_section[(nr / (((1) << 12) / 32))]'

   1153 static inline struct mem_section *__nr_to_section(unsigned long nr)
   1154 {
   1155 #ifdef CONFIG_SPARSEMEM_EXTREME
   1156         if (!mem_section)
   1157                 return NULL;
   1158 #endif
   1159         if (!mem_section[SECTION_NR_TO_ROOT(nr)])
   1160                 return NULL;
   1161         return &mem_section[SECTION_NR_TO_ROOT(nr)][nr & SECTION_ROOT_MASK];
   1162 }

   In the 2-D array access, it seems to be complaining about the '[nr &
   SECTION_ROOT_MASK]' reference.  But that appears to be safe because
   all the unsafe bits are masked off.
 
   It would be great if Smatch could detect that situation if possible.

2) Looking at the above example, it seems that the value of 'nr' is
   untrusted.  If so, then I wonder why didn't it warn about the other
   array accesses in the function: line 1559 and the first dimension
   access in 1161?

3) One thing that I think would help with analyzing the results would be
   if there was a way to see the call chain for each warning, so that
   it's clear which value isn't trusted and why.

4) Is there a way to put some results in a whitelist to mark them as
   false positives so they won't show up in future scans?  Something
   like that would help with automatic detection and reporting of new
   issues by the 0-day kbuild test robot, for example.

-- 
Josh

  parent reply	other threads:[~2018-06-08 16:12 UTC|newest]

Thread overview: 21+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-19  5:15 Dan Carpenter
2018-04-19 21:39 ` Gustavo A. R. Silva
2018-04-20 12:00 ` Peter Zijlstra
2018-04-23 12:31   ` Gustavo A. R. Silva
2018-04-23 12:45     ` Peter Zijlstra
2018-04-23 13:08       ` Gustavo A. R. Silva
2018-04-23 13:48       ` Dan Williams
2018-04-20 12:25 ` Thomas Gleixner
2018-04-20 17:21   ` Oleg Nesterov
2018-04-20 12:47 ` Mark Rutland
2018-04-23 12:53   ` Dan Carpenter
2018-04-23 13:22     ` Mark Rutland
2018-04-23 13:26       ` Dan Carpenter
2018-04-23 17:11 ` Davidlohr Bueso
2018-04-25 13:19 ` Mark Rutland
2018-04-25 14:48   ` Alan Cox
2018-04-25 15:03     ` Mark Rutland
2018-06-08 16:12 ` Josh Poimboeuf [this message]
2018-06-11  9:28   ` Peter Zijlstra
2018-06-13 13:10   ` Dan Carpenter
2018-06-13 13:58     ` Dan Carpenter

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20180608161219.q3lwvlydvs4l2gxa@treble \
    --to=jpoimboe@redhat.com \
    --cc=dan.carpenter@oracle.com \
    --cc=gustavo@embeddedor.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=peterz@infradead.org \
    --subject='Re: Smatch check for Spectre stuff' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).