| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20221115034007.go64rirsoqazpks7@box.shutemov.name>
Date: Tue, 15 Nov 2022 06:40:07 +0300
From: "Kirill A. Shutemov" <kirill@...temov.name>
To: Dave Hansen <dave.hansen@...el.com>
Cc: kernel test robot <lkp@...el.com>,
"Kirill A. Shutemov" <kirill.shutemov@...ux.intel.com>,
oe-kbuild-all@...ts.linux.dev, linux-kernel@...r.kernel.org,
x86@...nel.org, Dave Hansen <dave.hansen@...ux.intel.com>,
"James E.J. Bottomley" <jejb@...ux.ibm.com>,
"Martin K. Petersen" <martin.petersen@...cle.com>
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
Powered by blists - more mailing lists