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;
}
