linux-kernel - Re: [PATCH v2 1/2] tty: serial: 8250

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <yw1xwmaslv1d.fsf@mansr.com>
Date: Wed, 07 May 2025 08:49:50 +0100
From: Måns Rullgård <mans@...sr.com>
To: Jiri Slaby <jirislaby@...nel.org>
Cc: gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org,
 linux-serial@...r.kernel.org, linux-omap@...r.kernel.org,
 stable@...r.kernel.org
Subject: Re: [PATCH v2 1/2] tty: serial: 8250_omap: fix tx with dma

Jiri Slaby <jirislaby@...nel.org> writes:

> On 06. 05. 25, 17:07, Mans Rullgard wrote:
>> Commit 1788cf6a91d9 ("tty: serial: switch from circ_buf to kfifo")
>> introduced an error in the TX DMA handling for 8250_omap.
>> When the OMAP_DMA_TX_KICK flag is set, one byte is pulled from the
>> kfifo and emitted directly in order to start the DMA.  This is done
>> without updating DMA tx_size which leads to uart_xmit_advance() called
>> in the DMA complete callback advancing the kfifo by one too much.
>> In practice, transmitting N bytes has been seen to result in the last
>> N-1 bytes being sent repeatedly.
>> This change fixes the problem by moving all of the dma setup after
>> the OMAP_DMA_TX_KICK handling and using kfifo_len() instead of the
>> dma size for the 4-byte cutoff check. This slightly changes the
>> behaviour at buffer wraparound, but it still transmits the correct
>> bytes somehow. At the point kfifo_dma_out_prepare_mapped is called,
>> at least one byte is guaranteed to be in the fifo, so checking the
>> return value is not necessary.
>> Fixes: 1788cf6a91d9 ("tty: serial: switch from circ_buf to kfifo")
>> Cc: stable@...r.kernel.org
>> Signed-off-by: Mans Rullgard <mans@...sr.com>
>> ---
>> v2: split patch in two
>> ---
>>   drivers/tty/serial/8250/8250_omap.c | 24 +++++++++---------------
>>   1 file changed, 9 insertions(+), 15 deletions(-)
>> diff --git a/drivers/tty/serial/8250/8250_omap.c
>> b/drivers/tty/serial/8250/8250_omap.c
>> index f1aee915bc02..180466e09605 100644
>> --- a/drivers/tty/serial/8250/8250_omap.c
>> +++ b/drivers/tty/serial/8250/8250_omap.c
>> @@ -1173,16 +1173,6 @@ static int omap_8250_tx_dma(struct uart_8250_port *p)
>>   		return 0;
>>   	}
>>   -	sg_init_table(&sg, 1);
>> -	ret = kfifo_dma_out_prepare_mapped(&tport->xmit_fifo, &sg, 1,
>> -					   UART_XMIT_SIZE, dma->tx_addr);
>> -	if (ret != 1) {
>> -		serial8250_clear_THRI(p);
>> -		return 0;
>> -	}
>> -
>> -	dma->tx_size = sg_dma_len(&sg);
>> -
>>   	if (priv->habit & OMAP_DMA_TX_KICK) {
>>   		unsigned char c;
>>   		u8 tx_lvl;
> ...
>> @@ -1216,11 +1206,12 @@ static int omap_8250_tx_dma(struct uart_8250_port *p)
>>   			goto err;
>>   		}
>>   		skip_byte = c;
>> -		/* now we need to recompute due to kfifo_get */
>> -		kfifo_dma_out_prepare_mapped(&tport->xmit_fifo, &sg, 1,
>> -				UART_XMIT_SIZE, dma->tx_addr);
>>   	}
>>   +	sg_init_table(&sg, 1);
>> +	kfifo_dma_out_prepare_mapped(&tport->xmit_fifo, &sg, 1,
>> +				     UART_XMIT_SIZE, dma->tx_addr);
>
> This can fail (note the first call to this was checked). The latter
> (deliberately) not.

No, it can't.  The fifo has already been checked to contain something
right at the top of the function.  There is no other failure mode for
kfifo_dma_out_prepare_mapped.

>> +
>>   	desc = dmaengine_prep_slave_sg(dma->txchan, &sg, 1, DMA_MEM_TO_DEV,
>>   			DMA_PREP_INTERRUPT | DMA_CTRL_ACK);
>>   	if (!desc) {
> ...
>> @@ -1248,8 +1240,10 @@ static int omap_8250_tx_dma(struct uart_8250_port *p)
>>   err:
>>   	dma->tx_err = 1;
>>   out_skip:
>> -	if (skip_byte >= 0)
>> +	if (skip_byte >= 0) {
>>   		serial_out(p, UART_TX, skip_byte);
>> +		p->port.icount.tx++;
>
> This is still unrelated.

No, it's not.  Your broken code called uart_xmit_advance with the full
amount due to the incorrect tx_size value.  This compensates for that.

You made this mess.  Now fix it.  I don't care how.  It's wasted enough
of my time already.

-- 
Måns Rullgård