Re: Detecting user data on base types

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