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

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

[Steve French]

> 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.

-- 
Thanks,

Steve

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

[Randy Dunlap]

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

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

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!

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

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

[Aurélien Aptel]

I am working on a rewrite of that CIFS ioctl like we discussed, and I'm
not using the get_user() helper so it should be ok.

Cheers,
-- 
Aurélien Aptel / SUSE Labs Samba Team
GPG: 1839 CB5F 9F5B FB9B AA97  8C99 03C8 A49B 521B D5D3
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg, DE
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah HRB 247165 (AG München)