linux-kernel - Re: [v2] serial_core:recognize invalid pointer from userspace

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <56E22DE1.80101@windriver.com>
Date:	Fri, 11 Mar 2016 10:30:57 +0800
From:	"Lu.Jiang" <lu.jiang@...driver.com>
To:	One Thousand Gnomes <gnomes@...rguk.ukuu.org.uk>
CC:	Greg KH <gregkh@...uxfoundation.org>,
	<linux-kernel@...r.kernel.org>, <linux-serial@...r.kernel.org>
Subject: Re: [v2] serial_core:recognize invalid pointer from userspace

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
>