Re: [v2] serial_core:recognize invalid pointer from userspace

["Lu.Jiang" <lu.jiang@windriver.com>]

On 2016年03月10日 21:46, One Thousand Gnomes wrote:
>> When userspace get setting with TIOCGSERIAL,
>>
>> 1.On 64bit kernel + 32bit rootfs, compat ioctl code use 0xffffffff to
>> mark invalid conversion.
> Start at the beginning. What is being passed back and forth that causes
> the problem. What memory address does your uart have ?

In fs/compat_ioctl.c, serial_struct_ioctl() use following code for 
convert a 64bit pointer into 32bit userspace pointer

         if (cmd == TIOCGSERIAL && err >= 0) {
                 if (copy_in_user(ss32, ss, offsetof(SS32, iomem_base)) ||
                     get_user(iomem_base, &ss->iomem_base))
                         return -EFAULT;
                 base = (unsigned long)iomem_base  >> 32 ?
                         0xffffffff : (unsigned)(unsigned long)iomem_base;

If iomem_base exceed 4G, TIOCGSERIAL didn't return right value for 
iomem_base.

it  use following code to covert 32bit to 64bit.
         if (cmd == TIOCSSERIAL) {
                 if (copy_in_user(ss, ss32, offsetof(SS32, iomem_base)) ||
                     get_user(udata, &ss32->iomem_base))
                         return -EFAULT;
                 iomem_base = compat_ptr(udata);


static inline void __user *compat_ptr(compat_uptr_t uptr)
{
         return (void __user *)(unsigned long)uptr;
}

You can see userspace application didn't get valid iomem_base via 
TIOCGSERAIL.
When issuing TIOCSSERIAL, application can not provide a workable value 
for kernel. It means TIOCSSERIAL can not work.

Typical serial setting application setserial is depend on those 2 ioctls.

>
>> 2.On 32bit kernel with 64bit physical address, uart_get_info() in
>> serial_core will truncate a 64bit address into 32bit pointer with
>> following code:
>>       retinfo->iomem_base      = (void *)(unsigned long)uport->mapbase;
>>
>> Then when setting with TIOCSSERIAL ioctl, application can not provide
>> original value for iomem_base, it leads setserial can not work on such
>> boards.
> Yes - it's a legacy feature that isn't really needed on 64bit systems.
>
>> I don't know why kernel should expose this value to userspace, and seems
>> no need for userspace application to update an physical address for driver.
>> Can we consider drop this member in handle for TIOCSSERIAL ioctl. Please
>> see a rough patch in attachment.
> There are old PC and embedded cases from the time before devicetree and
> ACPI were the default. It's now an API people rely upon.
>
> It would make sense I think to return 0 for the base address if it
> exceeds 32bits, and also to ignore a TIOCSSERIAL that passes 0 as the
> address back. Would that fix your use case ?

If boards relay on this TIOCSSERIAL only working on 32bit, this solution 
is workable.


Thanks
Jiang Lu



>
> Alan
>