Re: [tip:x86/mm 5/16] sound/core/hwdep.c:243:24: sparse: sparse: incorrect type in assignment (different address spaces)

[Kirill A. Shutemov]

On Mon, Nov 14, 2022 at 02:55:46PM -0800, Dave Hansen wrote:
> On 11/14/22 13:41, kernel test robot wrote:
> >    sound/core/hwdep.c:243:24: sparse:     expected int [noderef] __user *__ptr_clean
> >    sound/core/hwdep.c:243:24: sparse:     got int *
> >    sound/core/hwdep.c:273:29: sparse: sparse: incorrect type in assignment (different address spaces) @@     expected int [noderef] __user *__ptr_clean @@     got int * @@
> >    sound/core/hwdep.c:273:29: sparse:     expected int [noderef] __user *__ptr_clean
> >    sound/core/hwdep.c:273:29: sparse:     got int *
> >    sound/core/hwdep.c:292:29: sparse: sparse: incorrect type in assignment (different address spaces) @@     expected int [noderef] __user *__ptr_clean @@     got int * @@
> >    sound/core/hwdep.c:292:29: sparse:     expected int [noderef] __user *__ptr_clean
> >    sound/core/hwdep.c:292:29: sparse:     got int *
> 
> I think the sparse ends up throwing away all of its annotations once it
> dereferences a pointer.  So, '*(int __user *)' boils down to a plain
> 'int'.  Confusingly, a '*(int __user *) *' boils down to an 'int *'.
> 
> That's what happened here.  A __user-annotated point got dereferenced
> down to an 'int' and then turned into a pointer again.
> 
> I think the trick in this case is to avoid dereferencing the pointer too
> early by just moving the dereference outside of the casting, like the
> attached patch.  But, it also feels kinda wrong.  I'd love a second
> opinion on this one.
> diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
> index 0db6f5451854..b8947b623c72 100644
> --- a/arch/x86/include/asm/uaccess.h
> +++ b/arch/x86/include/asm/uaccess.h
> @@ -39,7 +39,7 @@ static inline bool pagefault_disabled(void);
>  #define untagged_ptr(mm, ptr)	({					\
>  	u64 __ptrval = (__force u64)(ptr);				\
>  	__ptrval = untagged_addr(mm, __ptrval);				\
> -	(__force __typeof__(*(ptr)) *)__ptrval;				\
> +	*(__force __typeof__((ptr)) *)__ptrval;				\

It casts __ptrval to pointer to pointer and then dereferences it, so it
gives a new value on the output. It breaks boot for me.

We can drop all '*' here and get:

	(__force __typeof__(ptr))__ptrval;				\

It helps with sparse and streamlines the code. But it uncovers error in
sg_scsi_ioctl():

	error: cast specifies array type

The line that triggers the error is:

	if (get_user(opcode, sic->data))

'sic->data' is an array. And it breaks get_user() contract:

 * @ptr must have pointer-to-simple-variable type, and the result of
 * dereferencing @ptr must be assignable to @x without a cast.

Array is not pointer-to-simple-variable type. Let's cast it explicitly to
(char __user *). It should match the current behaviour. But double check
would be nice.

With sg_scsi_ioctl() fixed, it builds and boots fine for me.

I also looked again at get_user() and put_user() and I think we can
simplify them. The variable just adds noise.

The change below helps with the sparse complains. I didn't checked all of
them, but what check looks good.

If it looks okay, I will prepare 3 patches: scsi fix, sparse fix,
get/put_user() cleanup.

diff --git a/arch/x86/include/asm/uaccess.h b/arch/x86/include/asm/uaccess.h
index 1d2c79246681..bd92e1ee1c1a 100644
--- a/arch/x86/include/asm/uaccess.h
+++ b/arch/x86/include/asm/uaccess.h
@@ -43,7 +43,7 @@ DECLARE_STATIC_KEY_FALSE(tagged_addr_key);
 #define untagged_ptr(mm, ptr)	({					\
 	u64 __ptrval = (__force u64)(ptr);				\
 	__ptrval = untagged_addr(mm, __ptrval);				\
-	(__force __typeof__(*(ptr)) *)__ptrval;				\
+	(__force __typeof__(ptr))__ptrval;				\
 })
 #else
 #define untagged_addr(mm, addr)	(addr)
@@ -158,10 +158,8 @@ extern int __get_user_bad(void);
  */
 #define get_user(x,ptr)							\
 ({									\
-	__typeof__(*(ptr)) __user *__ptr_clean;				\
-	__ptr_clean = untagged_ptr(current->mm, ptr);			\
 	might_fault();							\
-	do_get_user_call(get_user,x,__ptr_clean);			\
+	do_get_user_call(get_user,x,untagged_ptr(current->mm, ptr));	\
 })
 
 /**
@@ -263,10 +261,8 @@ extern void __put_user_nocheck_8(void);
  * Return: zero on success, or -EFAULT on error.
  */
 #define put_user(x, ptr) ({						\
-	__typeof__(*(ptr)) __user *__ptr_clean;				\
-	__ptr_clean = untagged_ptr(current->mm, ptr);			\
 	might_fault();							\
-	do_put_user_call(put_user,x,__ptr_clean);			\
+	do_put_user_call(put_user,x,untagged_ptr(current->mm, ptr));	\
 })
 
 /**
diff --git a/drivers/scsi/scsi_ioctl.c b/drivers/scsi/scsi_ioctl.c
index 2d20da55fb64..c285502f5993 100644
--- a/drivers/scsi/scsi_ioctl.c
+++ b/drivers/scsi/scsi_ioctl.c
@@ -519,7 +519,7 @@ static int sg_scsi_ioctl(struct request_queue *q, fmode_t mode,
 		return -EFAULT;
 	if (in_len > PAGE_SIZE || out_len > PAGE_SIZE)
 		return -EINVAL;
-	if (get_user(opcode, sic->data))
+	if (get_user(opcode, (char __user *)sic->data))
 		return -EFAULT;
 
 	bytes = max(in_len, out_len);
-- 
  Kiryl Shutsemau / Kirill A. Shutemov