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

[Lu.Jiang]

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