Re: ioctl.c:undefined reference to `__get_user_bad'

Re: ioctl.c:undefined reference to `__get_user_bad'

[Randy Dunlap]

On 5/10/21 5:53 PM, kernel test robot wrote:
> tree:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git master
> head:   142b507f911c5a502dbb8f603216cb0ea8a79a48
> commit: 7ba3d1cdb7988ccfbc6e4995dee04510c85fefbc smb3.1.1: allow dumping keys for multiuser mounts
> date:   7 days ago
> config: arm-randconfig-s031-20210510 (attached as .config)
> compiler: arm-linux-gnueabi-gcc (GCC) 9.3.0
> reproduce:
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # apt-get install sparse
>         # sparse version: v0.6.3-341-g8af24329-dirty
>         # https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=7ba3d1cdb7988ccfbc6e4995dee04510c85fefbc
>         git remote add linus https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
>         git fetch --no-tags linus master
>         git checkout 7ba3d1cdb7988ccfbc6e4995dee04510c85fefbc
>         # save the attached .config to linux build tree
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' W=1 ARCH=arm 
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>):
> 
>    arm-linux-gnueabi-ld: fs/cifs/ioctl.o: in function `cifs_dump_full_key':
>>> ioctl.c:(.text+0x44): undefined reference to `__get_user_bad'
> 
> ---

# CONFIG_MMU is not set

and arch/arm/include/asm/uaccess.h does not implement get_user(size 8 bytes)
for the non-MMU case:

#define __get_user_err(x, ptr, err)					\
do {									\
	unsigned long __gu_addr = (unsigned long)(ptr);			\
	unsigned long __gu_val;						\
	unsigned int __ua_flags;					\
	__chk_user_ptr(ptr);						\
	might_fault();							\
	__ua_flags = uaccess_save_and_enable();				\
	switch (sizeof(*(ptr))) {					\
	case 1:	__get_user_asm_byte(__gu_val, __gu_addr, err);	break;	\
	case 2:	__get_user_asm_half(__gu_val, __gu_addr, err);	break;	\
	case 4:	__get_user_asm_word(__gu_val, __gu_addr, err);	break;	\
	default: (__gu_val) = __get_user_bad();				\
	}								\
	uaccess_restore(__ua_flags);					\
	(x) = (__typeof__(*(ptr)))__gu_val;				\
} while (0)


-- 
~Randy


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: Fwd: [EXTERNAL] Re: ioctl.c:undefined reference to `__get_user_bad'

[Randy Dunlap]

[adding back linux-arm-kernel; what happened to it? ]


On 5/17/21 2:04 PM, Randy Dunlap wrote:
> On 5/17/21 10:13 AM, Steve French wrote:
>>> If you fix the issue, kindly add following tag as appropriate
>>> Reported-by: kernel test robot <lkp@intel.com>
>>>
>>> All errors (new ones prefixed by >>):
>>>
>>>    arm-linux-gnueabi-ld: fs/cifs/ioctl.o: in function `cifs_dump_full_key':
>>>>> ioctl.c:(.text+0x44): undefined reference to `__get_user_bad'
>>>
>>
>> <snip>
>>
>>> # CONFIG_MMU is not set
>>>
>>> and arch/arm/include/asm/uaccess.h does not implement get_user(size 8 bytes)
>>> for the non-MMU case:
>>
>> I see another place in fs/cifs/ioctl.c where we already had been doing
>> a get_user() into a u64 - any idea what you are supposed to do
>> instead?  Any example code where people have worked around this.
> 
> Hi Steve,
> 
> This change in cifs_dump_full_key() makes it build OK:
> 
> -       if (get_user(suid, (__u64 __user *)arg))
> +       if (get_user(suid, (unsigned int __user *)arg))
> 
> 
> That is what the other call uses:
> 
> 		case FS_IOC_SETFLAGS:
> 			cifs_dbg(VFS, "cifs ioctl FS_IOC_SETFLAGS:\n");
> 			if (pSMBFile == NULL)
> 				break;
> 			tcon = tlink_tcon(pSMBFile->tlink);
> 			caps = le64_to_cpu(tcon->fsUnixInfo.Capability);
> 
> 			if (get_user(ExtAttrBits, (int __user *)arg)) {
> 				rc = -EFAULT;
> 				break;
> 			}
> 
> However, my reading/understanding is that the one immediately above
> is incorrect, as is the -/+ patch above it, since get_user() gets its
> data size (1, 2, 4, 8) from the type of the pointer that is passed to it.
> For 8 bytes (64 bits), 'int' is not sufficient, so IMO the get_user()
> call that builds:
> 			if (get_user(ExtAttrBits, (int __user *)arg)) {
> is a bug. It should be:
> 			if (get_user(ExtAttrBits, (__u64 __user *)arg)) {
> and if I make that latter change in the source file, the build says:
> 
> arm-linux-gnueabi-ld: fs/cifs/ioctl.o: in function `cifs_dump_full_key':
> ioctl.c:(.text+0x14): undefined reference to `__get_user_bad'
> arm-linux-gnueabi-ld: fs/cifs/ioctl.o: in function `cifs_ioctl':
> ioctl.c:(.text+0x1f2): undefined reference to `__get_user_bad'
> 
> so now both of them fail on the get_user() of 8 bytes.
> 
> Hope that clarifies things.  It tells me that arm no-MMU still
> needs support for get_user() of size 8 bytes.
> 

-- 
~Randy


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: Fwd: [EXTERNAL] Re: ioctl.c:undefined reference to `__get_user_bad'

[Russell King (Oracle)]

On Mon, May 17, 2021 at 02:06:33PM -0700, Randy Dunlap wrote:
> [adding back linux-arm-kernel; what happened to it? ]

Nothing. I'm not interested in trying to do major disgusting
contortions to make get_user() work for 8-byte values. If someone
else wants to put the effort in and come up with an elegant solution
that doesn't add warnings over the rest of the kernel, that's fine.

As far as I remember, everything in __get_user_err() relies on
__gu_val _not_ being 64-bit. If we use the same trick that we do
in __get_user_check():

	__inttype(x) __gu_val = (x);

then if get_user() is called with a 64-bit integer value and a
pointer-to-32-bit location to fetch from, we'd end up passing a
64-bit integer into the __get_user_asm() which could access the
wrong 32-bit half of the value in BE mode. Similar issue with
64-bit vs pointer-to-16-bit.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: Fwd: [EXTERNAL] Re: ioctl.c:undefined reference to `__get_user_bad'

[Randy Dunlap]

On 5/18/21 3:03 AM, Russell King (Oracle) wrote:
> On Mon, May 17, 2021 at 02:06:33PM -0700, Randy Dunlap wrote:
>> [adding back linux-arm-kernel; what happened to it? ]

That was about what I thought was Steve F. dropping LAK on his response email.

> Nothing. I'm not interested in trying to do major disgusting
> contortions to make get_user() work for 8-byte values. If someone
> else wants to put the effort in and come up with an elegant solution
> that doesn't add warnings over the rest of the kernel, that's fine.
> 
> As far as I remember, everything in __get_user_err() relies on
> __gu_val _not_ being 64-bit. If we use the same trick that we do
> in __get_user_check():
> 
> 	__inttype(x) __gu_val = (x);
> 
> then if get_user() is called with a 64-bit integer value and a
> pointer-to-32-bit location to fetch from, we'd end up passing a
> 64-bit integer into the __get_user_asm() which could access the
> wrong 32-bit half of the value in BE mode. Similar issue with
> 64-bit vs pointer-to-16-bit.

Yes, trying to handle get_user() of size 8 bytes is quite messy.
I have a few versions and they are all ugly and cause build warnings.

So we are down to what bugzilla calls WONTFIX. I'm OK with that.

Thanks.

-- 
~Randy


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel