From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from userp2120.oracle.com ([156.151.31.85]:59656 "EHLO userp2120.oracle.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726038AbfE2Ttq (ORCPT ); Wed, 29 May 2019 15:49:46 -0400 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=oracle.com; h=date : from : to : cc : subject : message-id : references : mime-version : content-type : in-reply-to; s=corp-2018-07-02; bh=4G1UtlEhWolsusYIRQm1NK6v3Ud6+zG1P4z1s53lIdc=; b=bGXjty7FLRwpT2bAcNAGgJlvala9rgW2xixzJ9zH3CNp7MDDNQWp3eCP3VZdg/PjnBjg fKhAvw8uZdkP9S1Ntu03uYr2bS8ZU+AaAsJOflWvVJelf9scPgL0qs+jUfkKeGzFNqTk j0HSFPaowwpjajv8xMF+Pb2URX5k8lzC0RVyvipJiadzOHDfMYn+zQBkRvMQYzDiuP84 mVMj6DL/+wQE3JFyhA9unWPj705q9p/vsPyGxwWwLJRKiYCgClz590b5HBton4NtLzGC wvO5/Vp1z7jEqWAmrT2U+XLRV2xrhEHJMCRjlrtMxnfi8+pSwil704oKHTD7OXNqMJMQ bw== Date: Wed, 29 May 2019 22:49:33 +0300 From: Dan Carpenter Subject: Re: Detecting user data on base types Message-ID: <20190529194933.GN24680@kadam> References: <20190529184722.GG709@e119886-lin.cambridge.arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190529184722.GG709@e119886-lin.cambridge.arm.com> Sender: smatch-owner@vger.kernel.org List-ID: To: Andrew Murray Cc: smatch@vger.kernel.org On Wed, May 29, 2019 at 07:47:23PM +0100, Andrew Murray wrote: > void func1(int x) > { > x++; > func2(x); > } > > void start(void *b) > { > void *a; > copy_from_user(a, b, 10); > func1(a[0]); > } Unfortunately, Smatch is pretty crap at tracking array elements so it doesn't track that a[0] is user controlled. Otherwise it generally does track that an int is controlled by the user and the user can pick a number between 0-10 or whatever. Like maybe the a=0-100 but only for in-kernel API and 0-10 can be set from the ioctl, the two ranges are tracked separately. > > In the above, smatch seems to be able to identify the callsite for func1 as > having user data, however not for x in the x++ expressions. I guess smatch would > need to track each use of 'a' and propogate a 'userspace' state for each assignment, > e.g. to x. (Is smatch currently limited to tracking user data for pointers rather > than base types?). x++ is problematic for Smatch, also. If we're outside of a loop then it says 0-10 becomes 1-11, but if we're inside a loop then it becomes 1-s32max. Another problem is that Smatch doesn't understand how the sign_extend64() function works so it doesn't understand the untagged_addr() macro. :/ I see one bug here and a missing feature... Right now Smatch thinks that the return value is totally unknown and not user controlled. This is a fixable issue by implementing SPECIAL_LEFTSHIFT in rl_binop(). Anyway, let me send you some updated test code without the array. Copy it to the smatch/ directory and do: ./smatch --info -p=kernel test.c | tee warns.txt ./smatch_data/db/create_db.sh -p=kernel warns.txt ./smatch -p=kernel test.c That maybe gives you a better idea of where we're at. I guess my other question would be where would you want to print the warning? regards, dan carpenter -------------- #include "check_debug.h" int copy_from_user(void *dest, void *src, long size); #define __s64 signed long long #define __u64 unsigned long long #define u64 unsigned long long #define __u8 unsigned char /** * sign_extend64 - sign extend a 64-bit value using specified bit as sign-bit * @value: value to sign extend * @index: 0 based bit index (0<=index<64) to sign bit */ static inline __s64 sign_extend64(__u64 value, int index) { __u8 shift = 63 - index; return (__s64)(value << shift) >> shift; } #define untagged_addr(addr) ((__typeof__(addr))sign_extend64((u64)(addr), 55)) void func1(unsigned long x) { __smatch_user_rl(x); } void *p; int main(void) { unsigned long a; copy_from_user(&a, p, sizeof(a)); __smatch_user_rl(a); __smatch_user_rl(untagged_addr(a)); func1(a); func1(untagged_addr(a)); return 0; }