serial_core: verify_port() in wrong spot?

serial_core: verify_port() in wrong spot?

[Stuart MacDonald]

Posted yesterday to linux-serial with no response.

The OX16PCI954 UART contains a 9bit mode. I'm developing support for
it. I thought it would be easy to shoehorn into the UPF_* flags:

diff -Naurp linux-2.6.11-5-titan485/include/linux/serial_core.h linux-2.6.11-6-9bit/include/linux/serial_core.h
--- linux-2.6.11-5-titan485/include/linux/serial_core.h	2006-06-02 13:59:07.000000000 -0400
+++ linux-2.6.11-6-9bit/include/linux/serial_core.h	2006-06-07 18:11:51.000000000 -0400
@@ -219,6 +219,7 @@ struct uart_port {
 #define UPF_SKIP_TEST		(1 << 6)
 #define UPF_AUTO_IRQ		(1 << 7)
 #define UPF_HARDPPS_CD		(1 << 11)
+#define UPF_9BIT		(1 << 12)
 #define UPF_LOW_LATENCY		(1 << 13)
 #define UPF_BUGGY_UART		(1 << 14)
 #define UPF_AUTOPROBE		(1 << 15)

However, in serial_core.c:set_uart_info(), there is a problem. The
flag should be within the purview of UPF_USR_MASK so that
non-privileged users can turn it on or off, and yet, I don't want the
mode to be enabled on UARTs that don't have it which requires
verification from the low-level driver. There is only one call to
ops->verify_port(), and it's not in the correct place for this to
happen.

So, I initially thought this patch would be best:

diff -Naurp linux-2.6.11-5-titan485/drivers/serial/serial_core.c linux-2.6.11-6-9bit/drivers/serial/serial_core.c
--- linux-2.6.11-5-titan485/drivers/serial/serial_core.c	2006-06-07 16:01:44.000000000 -0400
+++ linux-2.6.11-6-9bit/drivers/serial/serial_core.c	2006-06-08 11:08:00.000000000 -0400
@@ -647,6 +647,12 @@ static int uart_set_info(struct uart_sta
 	old_flags = port->flags;
 	old_custom_divisor = port->custom_divisor;
 
+	/*
+	 * Ask the low level driver to verify the settings.
+	 */
+	if (port->ops->verify_port)
+		retval = port->ops->verify_port(port, &new_serial);
+
 	if (!capable(CAP_SYS_ADMIN)) {
 		retval = -EPERM;
 	if (change_irq || change_port ||
@@ -662,12 +668,6 @@ static int uart_set_info(struct uart_sta
 		goto check_and_exit;
 	}

-	/*
-	 * Ask the low level driver to verify the settings.
-	 */
-	if (port->ops->verify_port)
-		retval = port->ops->verify_port(port, &new_serial);
-
 	if ((new_serial.irq >= NR_IRQS) || (new_serial.irq < 0) ||
 	    (new_serial.baud_base < 9600))
 		retval = -EINVAL;

but I'm not sure that's not a security hole of some sort; revealing
that the setting is valid or invalid before revealing whether the user
is allowed to set it. So perhaps this is better:

diff -Naurp linux-2.6.11-5-titan485/drivers/serial/serial_core.c linux-2.6.11-6-9bit/drivers/serial/serial_core.c
--- linux-2.6.11-5-titan485/drivers/serial/serial_core.c	2006-06-07 16:01:44.000000000 -0400
+++ linux-2.6.11-6-9bit/drivers/serial/serial_core.c	2006-06-08 11:45:16.000000000 -0400
@@ -656,6 +656,14 @@ static int uart_set_info(struct uart_sta
 		    (new_serial.xmit_fifo_size != port->fifosize) ||
 		    (((new_serial.flags ^ old_flags) & ~UPF_USR_MASK) != 0))
 			goto exit;
+		/*
+		 * Ask the low level driver to verify the settings.
+		 */
+		if (port->ops->verify_port) {
+			retval = port->ops->verify_port(port, &new_serial);
+			if (retval)
+				goto exit;
+		}
 		port->flags = ((port->flags & ~UPF_USR_MASK) |
 			       (new_serial.flags & UPF_USR_MASK));
 		port->custom_divisor = new_serial.custom_divisor;

but I don't like the duplication of code.

Any thoughts?

..Stu

Re: serial_core: verify_port() in wrong spot?

[Russell King]

On Fri, Jun 09, 2006 at 10:52:31AM -0400, Stuart MacDonald wrote:
> However, in serial_core.c:set_uart_info(), there is a problem. The
> flag should be within the purview of UPF_USR_MASK so that
> non-privileged users can turn it on or off, and yet, I don't want the
> mode to be enabled on UARTs that don't have it which requires
> verification from the low-level driver. There is only one call to
> ops->verify_port(), and it's not in the correct place for this to
> happen.

I'd rather verify_port didn't get used for that - it's purpose is to
validate changes the admin makes to the port.

I don't know why you think that setting 9bit mode should be done this
way rather than through the usual termios methods - the termios methods
already have a way to control the length of each character, so it would
seem logical to put the control in there.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

RE: serial_core: verify_port() in wrong spot?

[Stuart MacDonald]

From: Russell King [rmk@arm.linux.org.uk]
> I'd rather verify_port didn't get used for that - it's purpose is to
> validate changes the admin makes to the port.

I did figure out that's what it's currently used as, but I didn't want
to introduce a whole new call just to verify that the UART has 9bit
capability.

Why aren't user changes validated?

> I don't know why you think that setting 9bit mode should be done this
> way rather than through the usual termios methods - the 
> termios methods
> already have a way to control the length of each character, 
> so it would
> seem logical to put the control in there.

9bit mode is much more than just words of 9 bit length. Parity is
gone, replaced by the 9th bit; reads and writes have to treat the
buffers driver-side buffers as 16 bit-wide instead of 8-bit; reads and
writes to the hardware are correspondingly different; there are new
interrupts; software flow control is gone; there's special address
matching and a new ioctl to set that up.

It seemed easier to create a new mode of operation based on the
UPF_9BIT flag; using the CS9 flag doesn't imply any of the above
except for 9 bit length.

However, I'm open to having my mind changed.

..Stu

Re: serial_core: verify_port() in wrong spot?

[Russell King]

On Fri, Jun 09, 2006 at 01:59:13PM -0400, Stuart MacDonald wrote:
> From: Russell King [rmk@arm.linux.org.uk]
> > I'd rather verify_port didn't get used for that - it's purpose is to
> > validate changes the admin makes to the port.
> 
> I did figure out that's what it's currently used as, but I didn't want
> to introduce a whole new call just to verify that the UART has 9bit
> capability.
> 
> Why aren't user changes validated?

The only things which users can change is low latency, the alternative
(deprecated) "38400-baud" baud rates, and the custom divisor.  None of
these depend on the low level driver, so there's no point asking the
low level driver to validate them.

> 9bit mode is much more than just words of 9 bit length. Parity is
> gone, replaced by the 9th bit; reads and writes have to treat the
> buffers driver-side buffers as 16 bit-wide instead of 8-bit; reads and
> writes to the hardware are correspondingly different; there are new
> interrupts; software flow control is gone; there's special address
> matching and a new ioctl to set that up.

Well, I'll have to read up on this before I can comment any further.
At the moment I don't feel qualified to answer your questions.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core