Bug 215205 - uart_set_info not returning -EPERM when incapable user tries to set ASYNC_LOW_LATENCY
Summary: uart_set_info not returning -EPERM when incapable user tries to set ASYNC_LOW...
Status: NEW
Alias: None
Product: Drivers
Classification: Unclassified
Component: Serial (show other bugs)
Hardware: All Linux
: P1 normal
Assignee: Russell King
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-12-02 20:52 UTC by nlowell
Modified: 2021-12-08 14:05 UTC (History)
1 user (show)

See Also:
Kernel Version: HEAD
Subsystem:
Regression: No
Bisected commit-id:


Attachments

Description nlowell 2021-12-02 20:52:10 UTC
If a user does not have CAP_SYS_ADMIN and runs this code:

    if (ioctl(fd, TIOCGSERIAL, &serialUart) == -1)
    {
        printf("ioctl(TIOCGSERIAL) failed: %s\n", strerror(errno));
        close(fd);
        fd = -1;
        return fd;
    }
 
    // Now set the serial configuration with low latency set.
    serialUart.flags |= ASYNC_LOW_LATENCY;
    if (ioctl(fd, TIOCSSERIAL, &serialUart) == -1)
    {
        printf("ioctl(TIOCSSERIAL) failed: %s\n", strerror(errno));
        close(fd);
        fd = -1;
        return fd;
    }

Code inside uart_set_info will temporarily set retval to -EPERM and execute to "goto check_and_exit"

        if (!capable(CAP_SYS_ADMIN)) {
		retval = -EPERM;
		if (change_irq || change_port ||
		    (new_info->baud_base != uport->uartclk / 16) ||
		    (close_delay != port->close_delay) ||
		    (closing_wait != port->closing_wait) ||
		    (new_info->xmit_fifo_size &&
		     new_info->xmit_fifo_size != uport->fifosize) ||
		    (((new_flags ^ old_flags) & ~UPF_USR_MASK) != 0))
			goto exit;
		uport->flags = ((uport->flags & ~UPF_USR_MASK) |
			       (new_flags & UPF_USR_MASK));
		uport->custom_divisor = new_info->custom_divisor;
		goto check_and_exit;
	}

1. Should uport->flags and uport->custom_divisor still get set?

Additionally, first line of code for check_and_exit clears ret_val = -EPERM by setting it back to 0:

    check_and_exit:
	retval = 0;
	if (uport->type == PORT_UNKNOWN)
		goto exit;
	if (tty_port_initialized(port)) {
		if (((old_flags ^ uport->flags) & UPF_SPD_MASK) ||
		    old_custom_divisor != uport->custom_divisor) {
			/*
			 * If they're setting up a custom divisor or speed,
			 * instead of clearing it, then bitch about it.
			 */
			if (uport->flags & UPF_SPD_MASK) {
				dev_notice_ratelimited(uport->dev,
				       "%s sets custom speed on %s. This is deprecated.\n",
				      current->comm,
				      tty_name(port->tty));
			}
			uart_change_speed(tty, state, NULL);
		}
	} else {
		retval = uart_startup(tty, state, 1);
		if (retval == 0)
			tty_port_set_initialized(port, true);
		if (retval > 0)
			retval = 0;
	}
 exit:
	return retval;

Therefore the -EPERM ret_val is lost and the user is unaware of the capabilities rejection.
Comment 1 Jiri Slaby 2021-12-08 05:44:38 UTC
(In reply to nlowell from comment #0)
>         if (!capable(CAP_SYS_ADMIN)) {
>               retval = -EPERM;
>               if (change_irq || change_port ||
>                   (new_info->baud_base != uport->uartclk / 16) ||
>                   (close_delay != port->close_delay) ||
>                   (closing_wait != port->closing_wait) ||
>                   (new_info->xmit_fifo_size &&
>                    new_info->xmit_fifo_size != uport->fifosize) ||
>                   (((new_flags ^ old_flags) & ~UPF_USR_MASK) != 0))
>                       goto exit;

If a user set something inappropriate, we go to "exit" -- return -EPERM.

>               uport->flags = ((uport->flags & ~UPF_USR_MASK) |
>                              (new_flags & UPF_USR_MASK));
>               uport->custom_divisor = new_info->custom_divisor;
>               goto check_and_exit;

If they did an allowed thing, we go to check_and_exit -- return 0.

>       }
> 
> 1. Should uport->flags and uport->custom_divisor still get set?

Yes, a user can set UPF_USR_MASK flags to uport->flags.

> Additionally, first line of code for check_and_exit clears ret_val = -EPERM
> by setting it back to 0:
> 
>     check_and_exit:
>       retval = 0;

Which is exactly what is intended.

> Therefore the -EPERM ret_val is lost and the user is unaware of the
> capabilities rejection.

Rejection of what? Users are supposed to check via TIOCGSERIAL again.
Comment 2 nlowell 2021-12-08 14:05:57 UTC
Thank you for the explanation.  

I see now that 

port->low_latency = (uport->flags & UPF_LOW_LATENCY) ? 1 : 0;

which used to be just before check_and_exit label has been removed from newer kernel versions (I have been working with v5.4).
So the issue was uport->flags was getting set but port->low_latency was not, which I believe is a bug, but perhaps that was the only item getting skipped that mattered.  So perhaps this is a false alarm for the latest kernel.  My apologies.

Side note: It's confusing that ret gets set to -EPERM before the additional checking which could end up negating that change.  Perhaps it would be better to only set ret = -EPERM before "goto exit".

Note You need to log in before you can comment on or make changes to this bug.