On Tue, Nov 25, 2014 at 4:59 AM, Dan Carpenter <dan.carpenter@xxxxxxxxxx> wrote:
> I added the smatch@xxxxxxxxxxxxxxx mailing list to the CC.
I can't find a public archive of this list... :(
> On Mon, Nov 24, 2014 at 02:31:38PM -0800, Kees Cook wrote:
>> Hi Dan,
>>
>> I've got a weird behavior out of smatch that I can't reproduce at
>> will. It just happens sometimes under differing build environments. I
>> haven't been able to isolate it yet, but I thought I'd check with you
>> in case it was something obvious. The warning I've been getting is:
>>
>> net/ipv4/icmp.c:514 icmp_route_lookup() error: potential NULL dereference 'rt'.
>>
>> The code is:
>>
>> static struct rtable *icmp_route_lookup(struct net *net,
>> ...
>> {
>> ...
>> rt2 = rt;
>>
>> rt = (struct rtable *) xfrm_lookup(net, &rt->dst,
>> flowi4_to_flowi(fl4), NULL, 0);
>> if (!IS_ERR(rt)) {
>> if (rt != rt2)
>> return rt;
>> } else if (PTR_ERR(rt) == -EPERM) {
>> rt = NULL;
>> } else
>> return rt;
>> ...
>>
>> if (!IS_ERR(rt2)) {
>> dst_release(&rt->dst);
>>
>> With line 514 being the dst_release() above.
>>
>> There's a clear path "possible" between "rt = NULL" and "rt->", though
>> I think in the real world it's not possible. (Though I haven't studied
>> it closely -- it's a bit weird.)
>>
>> So, with my local copy of smatch, on the regular upstream tree, I do
>> NOT get the warning, but it seems like I should. Any idea what's going
>> on here?
>
> I've figured it out partly, sort of:
>
> smatch/check_deref.c
> 179 static void match_assign(struct expression *expr)
> 180 {
> 181 struct statement *stmt;
> 182
> 183 if (!is_zero(expr->right))
> 184 return;
> 185
> 186 FOR_EACH_PTR_REVERSE(big_statement_stack, stmt) {
> 187 if (stmt->type == STMT_DECLARATION)
> 188 return;
> 189 break;
> 190 } END_FOR_EACH_PTR_REVERSE(stmt);
> 191
> 192 set_state_expr(my_id, expr->left, &null);
> 193 }
>
> The big_statement_stack is getting corrupted somehow so it
> randomly/unpredictably returns instead of setting "rt" to &null.
>
> I was trying to ignore initializers when I wrote that code because
> everyone initializes variables to NULL to silence GCC warnings so
> initializers cause a lot of false positives.
>
> I guess I'll try figure out how big_statement_stack is getting
> corrupted but memory corruption is a pain to figure out. Valgrind
> hasn't helped me here.
Cool, thanks for looking into it!
-Kees
--
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe smatch" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
