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

From: Kirill A. Shutemov
Date: Mon Nov 14 2022 - 22:40:25 EST


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