Messages in this thread Patch in this message | | | Date | Tue, 15 Nov 2022 06:40:07 +0300 | From | "Kirill A. Shutemov" <> | Subject | Re: [tip:x86/mm 5/16] sound/core/hwdep.c:243:24: sparse: sparse: incorrect type in assignment (different address spaces) |
| |
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
| |