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

From: Lu.Jiang
Date: Wed Mar 09 2016 - 23:56:41 EST


On 2016å03æ10æ 11:34, Greg KH wrote:
On Thu, Mar 10, 2016 at 11:17:23AM +0800, Jiang Lu wrote:
compat_ioctl use 0xffffffff as a magic number to mark invalid pointer
for iomem_base in serial_struct when truncating a 64bit pointer into
32bit.

Serial driver need recognize this invalid pointer when parsing
serial_struct from userspace.

Signed-off-by: Jiang Lu <lu.jiang@xxxxxxxxxxxxx>
---
drivers/tty/serial/serial_core.c | 3 +++
1 file changed, 3 insertions(+)

diff --git a/drivers/tty/serial/serial_core.c b/drivers/tty/serial/serial_core.c
index a5d545e..d293536 100644
--- a/drivers/tty/serial/serial_core.c
+++ b/drivers/tty/serial/serial_core.c
@@ -745,6 +745,9 @@ static int uart_set_info(struct tty_struct *tty, struct tty_port *port,
* allocations, we should treat type changes the same as
* IO port changes.
*/
+ if ((unsigned long)new_info->iomem_base == 0xffffffff)
+ new_info->iomem_base = (void *)(unsigned long)uport->mapbase;
This looks really odd to me, why do we care about userspace issues here?
Shouldn't the compat ioctl code have handled this already all for us?
When getting serial struct, compat ioctl code just set it to 0xffffffff when 64bit iomem_base is beyond 32bit in kernel.

Then when setting serial_struct, compat ioctl code for TIOCSSERIAL can not restore original value, it need serial_core to take care of this.

And why set it to mapbase? Just to keep it from being changed?

Serial_core just restore the invalid value with original value to make following code in uart_set_info() happy.


this worries me...


Actually, This member introduce lots of trouble with 32bit/64bit conversion. For example,

When userspace get setting with TIOCGSERIAL,

1.On 64bit kernel + 32bit rootfs, compat ioctl code use 0xffffffff to mark invalid conversion.

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.

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.

Thanks
Jiang Lu

greg k-h