imx: Race when disabling RX in IMX.6 UART in half duplex

[Piotr Figiel]

Hi,


On 22.02.2017 11:44, Sascha Hauer wrote:
> On Tue, Feb 21, 2017 at 09:48:54AM +0100, Piotr Figiel wrote:
>> Hi Baruch,
>>
>>
>> On 21.02.2017 09:18, Baruch Siach wrote:
>>> Hi Piotr,
>>>
>>> On Tue, Feb 21, 2017 at 09:00:39AM +0100, Piotr Figiel wrote:
>>>> On 20.02.2017 20:20, Baruch Siach wrote:
>>>>> On Mon, Feb 20, 2017 at 02:54:02PM +0100, Piotr Figiel wrote:
>>>>>>     I'm looking to find a correct way to disable RX in the I.MX6 UART during
>>>>>> TX-ing. I stumbled on the issue described below when working on
>>>>>> ~SER_RS485_RX_DURING_TX on custom 3.10 tree (branched from Freescale's/NXP's
>>>>>> BSP release) independently to what is now in the main line imx.c, but I see
>>>>>> now that the implementation in mainline also seem to have the same problem
>>>>>> I'm trying to solve now. I would like to request for comments to
>>>>>> confirm/deny whether the issue I raise here is valid and how to best
>>>>>> approach this.
>>>>>>
>>>>>>     I'm mostly concerned about the fact that the URXD register must not be
>>>>>> accessed (as documented in IMX.6 RM) when RX is disabled (RXEN=0). The issue
>>>>>> is that in the following sequence happening during imx_start_tx with
>>>>>> ~SER_RS485_RX_DURING_TX set:
>>>>>>
>>>>>> 1. Acquire spinlock, disable local interrupts (from serial_core.c),
>>>>>> 2. Disable RX receiver (RXEN=0 in UCR2),
>>>>>> 3. Release spinlock, reenable interrupts.
>>>>>>
>>>>>> The RX fifo may become not empty between #1 and #2. This will raise
>>>>>> interrupt which will be handled after re-enabling interrupts (after #3). ISR
>>>>>> in this case will check the status bit of the interrupt and fetch RX FIFO
>>>>>> contents, which I understand is forbidden by the documentation and may raise
>>>>>> error on the bus [1]. In addition disabling RX IRQ in this procedure, e.g.
>>>>>> after #1 doesn't seem to be enough, as the IRQ still may trigger after #1
>>>>>> and will need to be serviced.
>>>>> As I understand it, SER_RS485_RX_DURING_TX is for half-duplex RS-485 where
>>>>> only one device is allowed to transmit at any given time. Higher level code
>>>>> determines when any given device on the bus is allowed to transmit. If Rx FIFO
>>>>> becomes non-empty during Tx enable, you have a contention problem to solve.
>>>> That's correct, although I don't want bugs from the user-space or
>>>> misbehaving devices on RS-485 to cause errors on the AXI/AHB bus resulting
>>>> with possible crash/exception on the CPU core. The reason I write about this
>>>> is that the RM explicitly forbids accessing those registers in such case.
>>> The trivial solution might be to return early from imx_rxint() when RXEN=0.
>>> Would that be sufficient?
>> I'm not sure if it's best idea, I don't know if the UART will keep
>> triggering the interrupt if the source won't be cleared (FIFO drained), but
>> if it does we'll be irq flooded,
> And indeed that's what happens. The RX irq is acked by reading all
> characters from the FIFO. I had the same problem as you describe, see
> below for the patch I came up with (with the intention to mainline i
> t when I find the time). My "usecase" was to send a character while
> being flooded with received characters from the other side. That of
> course was only for testing purposes, but I could reliably crash
> the kernel with this setup.

Thanks for confirming this, this probably requires CVE, since user with 
tty access can crash mainline kernel by correctly timing a write 
(personally though I wasn't able to trigger this without putting delay 
in the code to trigger the race). Some comments regarding the 
implementation (thanks for sharing this btw):

> +static void imx_receiver_safe_disable(struct imx_port *sport)
> +{
> +	u32 ucr2, ucr1;
> +	int t = 50;
> +
> +	ucr1 = readl(sport->port.membase + UCR1);
> +	ucr1 &= ~UCR1_RRDYEN;
> +	writel(ucr1, sport->port.membase + UCR1);
> +
> +	ucr2 = readl(sport->port.membase + UCR2);
> +
> +	/*
> +	 * Disable the receiver and make sure that no characters are left
> +	 * in the FIFO. Otherwise an interrupt will be triggered even when
> +	 * UCR1_RRDYEN is cleared. We can't ack this interrupt because with
> +	 * disabled receiver we are not allowed to read from the FIFO.
> +	 */
> +	do {
> +		writel(ucr2 | UCR2_RXEN, sport->port.membase + UCR2);
> +
> +		while (readl(sport->port.membase + USR2) & USR2_RDR)
> +			readl(sport->port.membase + URXD0);
> +
> +		writel(ucr2 & ~UCR2_RXEN, sport->port.membase + UCR2);
> +
> +		if (!t--) {
> +			dev_err(sport->port.dev, "Failed to disable the receiver\n");
> +			return;
In this unlikely event perhaps an error should be returned so that the 
RXEN is not disabled in imx_start_tx() (i.e. I think it's better to 
ignore ~SER_RS485_RX_DURING_TX for a while than to crash).

> +		}
> +	} while (readl(sport->port.membase + USR2) & USR2_RDR);
> +}
> +
>   /*
>    * interrupts disabled on entry
>    */
> @@ -633,8 +669,10 @@ static void imx_start_tx(struct uart_port *port)
>   				imx_port_rts_active(sport, &temp);
>   			else
>   				imx_port_rts_inactive(sport, &temp);
> -			if (!(port->rs485.flags & SER_RS485_RX_DURING_TX))
> +			if (!(port->rs485.flags & SER_RS485_RX_DURING_TX)) {
> +				imx_receiver_safe_disable(sport);
>   				temp &= ~UCR2_RXEN;
> +			}
>   			writel(temp, port->membase + UCR2);
>   			sport->tx_state = WAIT_AFTER_RTS;
>   			sport->tx_state_next_change =

Other than that I think it's OK (it's also consistent with the procedure 
from my first post [1]).

Best regards, Piotr.

[1] 
http://lists.infradead.org/pipermail/linux-arm-kernel/2017-February/488957.html