ksummit.lists.linux.dev archive mirror
 help / color / mirror / Atom feed
* Potential static analysis ideas
@ 2021-07-23 19:10 Dan Carpenter
  2021-07-24 13:33 ` Geert Uytterhoeven
  2021-07-26 15:50 ` Paul E. McKenney
  0 siblings, 2 replies; 30+ messages in thread
From: Dan Carpenter @ 2021-07-23 19:10 UTC (permalink / raw)
  To: ksummit; +Cc: Julia Lawall

Rust has many good static analysis features but if we wanted we could
implement a number of stricter rules in C.  With Smatch I have tried to
focus on exclusively on finding bugs because everyone can agree that
bugs are bad.  But if some subsystems wanted to implement stricter rules
just as a hardenning measure then that's a doable thing.

For example, I've tried a bunch of approaches to warning about when the
user can trigger an integer overflow.  The challenge is that most
integer overflows are harmless and do not cause a real life bug.

I've also written some checks for locking bugs where the warning is
"expected lock 'foo' held when accessing struct member 'bar'".  The
problem is that I'm not a locking expert.  My process is 1) Infer the
pairing between this lock protect this pointer.  2)  Make list of places
where the pointer holds a function pointer and we free it under lock
(I still need to add the locking part).  3) Make a list of places where
we access the data without holding the lock and the attacker can control
the timing with userfaultd or whatever.

This sort of locking check which finds real life exploitable bugs is
much more complicated to write.  If there were subsystems which had
simpler, hardenned rules then that would be easier.

Another idea is that if I were a subsystem maintainer, I think I would
enforce my prefered error handling rules.  1) Every allocation function
must have exactly one free function.  2) Every function should clean up
after itself on failure. 3) Never free things that aren't allocated.
4) The frees must happen in reverse order from how they were allocated.

These days we have disabled GCC's uninitialized warnings so it falls
to static analysis devs to review all the new warnings.  I sometimes
ignore warnings if they look like probably they aren't a bug.  Does this
function allow zero size writes?  Does this switch statement really need
a default case?  Maybe sometimes I miss bugs.

Here is another example of something which I have a check for but I
haven't pushed.

	ret = frob();
	if (!ret)
		return ret;

This code is normally correct but sometimes it means the if statement is
reversed and it should be "if (ret) return ret;".  To me returning a
literal is always more clear but but clearly the original author felt
"ret" was more clear...  It's only a few bugs per year so it's not a big
deal either way.

A sort of related example that I have pushed is this:

	int ret = 0;

	/* 20 lines of code */

	if (condition)
		goto cleanup;

These trigger a published Smatch warning for missing error codes but I
only email people when I can't understand the code.  Please, put the
"ret = 0;" within 4 lines of the goto.

Those are just some ideas of things we could do if we wanted to use
static analysis for hardenning instead of for fixing bugs.

regards,
dan carpenter

^ permalink raw reply	[flat|nested] 30+ messages in thread

end of thread, other threads:[~2021-07-27 16:06 UTC | newest]

Thread overview: 30+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-23 19:10 Potential static analysis ideas Dan Carpenter
2021-07-24 13:33 ` Geert Uytterhoeven
2021-07-24 13:40   ` Julia Lawall
2021-07-24 14:08   ` Arnd Bergmann
2021-07-24 23:18   ` Laurent Pinchart
2021-07-24 23:45     ` NeilBrown
2021-07-26  7:25       ` Arnd Bergmann
2021-07-26  7:53         ` Geert Uytterhoeven
2021-07-26  8:20           ` Arnd Bergmann
2021-07-26  8:39             ` Geert Uytterhoeven
2021-07-26  8:52               ` Arnd Bergmann
2021-07-26  9:11                 ` Geert Uytterhoeven
2021-07-26  8:55             ` Julia Lawall
2021-07-26  9:08               ` Hannes Reinecke
2021-07-26  9:16                 ` Geert Uytterhoeven
2021-07-26  9:28                   ` Julia Lawall
2021-07-26  9:35                     ` Hannes Reinecke
2021-07-26 10:03                       ` Julia Lawall
2021-07-26 17:54                   ` James Bottomley
2021-07-26 18:16                     ` Linus Torvalds
2021-07-26 21:53                       ` NeilBrown
2021-07-26 18:31                     ` Laurent Pinchart
2021-07-26  9:17                 ` Dan Carpenter
2021-07-26  9:13             ` Dan Carpenter
2021-07-26 21:43         ` NeilBrown
2021-07-26  7:05   ` Dan Carpenter
2021-07-26 15:50 ` Paul E. McKenney
2021-07-27  9:38   ` Dan Carpenter
2021-07-27  9:50     ` Geert Uytterhoeven
2021-07-27 16:06     ` Paul E. McKenney

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).