Re: [PATCH] i386: Add a temporary to make put_user more type safe.

[Andrew Morton]

ebiederm@xxxxxxxxxxxx (Eric W. Biederman) wrote:
>
> > Sounds sane.  We could make it warn if typeof(x)!=typeof(*ptr) by adding
>  > another temporary for the pointer, give it type typeof(x)*, but I haven't
>  > tried it.
> 
>  I guess we could do that.  However if we don't use the value we will probably
>  get an unused variable warning.  

No, that's OK, we can use the temporary.

Something like this:

--- devel/include/asm-i386/uaccess.h~x86-tighten-uaccess-type-checking	2006-01-28 23:45:16.000000000 -0800
+++ devel-akpm/include/asm-i386/uaccess.h	2006-01-28 23:48:31.000000000 -0800
@@ -149,14 +149,16 @@ extern void __get_user_4(void);
 #define get_user(x,ptr)							\
 ({	int __ret_gu;							\
 	unsigned long __val_gu;						\
+	__typeof__(x)*_p_;						\
+	_p_ = ptr;							\
 	__chk_user_ptr(ptr);						\
-	switch(sizeof (*(ptr))) {					\
-	case 1:  __get_user_x(1,__ret_gu,__val_gu,ptr); break;		\
-	case 2:  __get_user_x(2,__ret_gu,__val_gu,ptr); break;		\
-	case 4:  __get_user_x(4,__ret_gu,__val_gu,ptr); break;		\
-	default: __get_user_x(X,__ret_gu,__val_gu,ptr); break;		\
+	switch(sizeof (*(_p_))) {					\
+	case 1:  __get_user_x(1, __ret_gu, __val_gu, _p_); break;	\
+	case 2:  __get_user_x(2, __ret_gu, __val_gu, _p_); break;	\
+	case 4:  __get_user_x(4, __ret_gu, __val_gu, _p_); break;	\
+	default: __get_user_x(X, __ret_gu, __val_gu, _p_); break;	\
 	}								\
-	(x) = (__typeof__(*(ptr)))__val_gu;				\
+	(x) = __val_gu;							\
 	__ret_gu;							\
 })
 
@@ -198,13 +200,17 @@ extern void __put_user_8(void);
 #define put_user(x,ptr)						\
 ({	int __ret_pu;						\
 	__chk_user_ptr(ptr);					\
-	__typeof__(*(ptr)) __pu_val = x;			\
+	__typeof__(x)*_p_;					\
+	__typeof__(x)__pu_val;					\
+								\
+	_p_ = ptr;						\
+	__pu_val = x;						\
 	switch(sizeof(*(ptr))) {				\
-	case 1: __put_user_1(__pu_val, ptr); break;		\
-	case 2: __put_user_2(__pu_val, ptr); break;		\
-	case 4: __put_user_4(__pu_val, ptr); break;		\
-	case 8: __put_user_8(__pu_val, ptr); break;		\
-	default:__put_user_X(__pu_val, ptr); break;		\
+	case 1: __put_user_1(__pu_val, _p_); break;		\
+	case 2: __put_user_2(__pu_val, _p_); break;		\
+	case 4: __put_user_4(__pu_val, _p_); break;		\
+	case 8: __put_user_8(__pu_val, _p_); break;		\
+	default:__put_user_X(__pu_val, _p_); break;		\
 	}							\
 	__ret_pu;						\
 })
_


It gives ~100 warnings on my usual test config.  It's a bit awkward because
get_user/put_user/etc aren't allowed to evaluate their args multiple times
- code likes to do

	get_user(v, p++);

I guess fixing those 100-odd warnings might find some warts -
compiler-caused truncation or sign-extension during uaccess copies is a bit
of a worry.

But I have enough things to be going on with :(
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/