Bug class

Posted May 22, 2013 2:18 UTC (Wed) by cesarb (subscriber, #6266)
Parent article: An unexpected perf feature

Would it be possible to add a check to something like sparse to detect this class of bug (value being truncated by being assigned from a larger type to a smaller type)? And the related sign confusion class of bug (assigning from unsigned to signed or vice versa without checking the range first)?

Bug class

Posted May 22, 2013 9:28 UTC (Wed) by mcfrisk (guest, #40131) [Link]

Wouldn't Coverity find this already?

Bug class

Posted May 22, 2013 12:28 UTC (Wed) by PaXTeam (guest, #24616) [Link] (1 responses)

> [...]to detect this class of bug (value being truncated by being assigned from a larger type to a smaller type)

it's not true that casting a value from a wider type to a narrower one is a bug in general (think how often long->int happens when error codes are returned in the kernel) so detecting that construct would produce immense amounts of false positives (based on our own experiments last year we're talking about many thousands of instances for allyesconfig/amd64). and even if the value is not preserved during the narrowing cast it may not be a bug but intended behaviour and it's very hard to tell for a compiler.

with that said, Emese Revfy wrote a gcc plugin for us that tries to detect this specific instance of unchecked signed array index usage. the results are somewhat more managable, there're about 200 instances on allmod/amd64, most of which are false positives (interprocedural analysis, etc would help eliminate most of them, but that's not a half an hour project as this one was), however some of them look genuine bugs, albeit nothing security related so far (of those that i checked that is).

> And the related sign confusion class of bug (assigning from unsigned to
> signed or vice versa without checking the range first)?

the size overflow plugin from Emese (http://forums.grsecurity.net/viewtopic.php?f=7&t=3043) does this but we had to scale it back due to the amount of false positives (even gcc creates these itself during canonicalization), as it's just not feasible to eliminate such constructs for now.

Bug class

Posted May 22, 2013 13:35 UTC (Wed) by cesarb (subscriber, #6266) [Link]

> and even if the value is not preserved during the narrowing cast it may not be a bug but intended behaviour and it's very hard to tell for a compiler.

If the narrowing losing the higher bits is intended behavior, the programmer should either use an explicit cast instead of an implicit one (x = (int)y instead of int x = y), or explicitly mask out the upper bits (x = y & 0xffffffffu). This would be similar to having to add an extra pair of parenthesis when using bitwise operators within an if/while/for, and would also make it more visible to readers of the code that the narrowing is taking place. So this class of false positives (intentionally masking by narrowing) is not a problem.

However, I do agree that the rest of the false positives would be a problem. What would be needed would be a way to somehow track the range of the variables, so the checker would know that for instance the variable being narrowed is in the range -1..-4095 (the error code case).

> however some of them look genuine bugs, albeit nothing security related so far (of those that i checked that is).

Will they be reported upstream? A bug is a bug, so even if they are not security related, they should be fixed.

Bug class

Posted May 23, 2013 10:41 UTC (Thu) by error27 (subscriber, #8346) [Link]

Smatch theoretically can find buffer underflows.

In this case it missed for several reasons. 1) This wasn't getting marked as untrusted user data. 2) The cross function tracking wasn't working. 3) The underflow check was ignoring if we capped the upper value. *eyeroll*

I haven't pushed all the Smatch changes yet, but it's sort of working now to the point where it would have found this bug. It did find a couple problems in ATM network drivers as well.