[PATCH] tty: serial: fsl_lpuart: Fix possible integer overflow

[PATCH] tty: serial: fsl_lpuart: Fix possible integer overflow

[Sherry Sun]

This patch addresses the following Coverity report, fix it by casting
sport->port.frame_time to type u64.

CID 32305660: Unintentional integer overflow (OVERFLOW_BEFORE_WIDEN)
Potentially overflowing expression sport->port.frame_time * 8U with type
unsigned int (32 bits, unsigned) is evaluated using 32-bit arithmetic,
and then used in a context that expects an expression of type u64 (64
bits, unsigned).

Fixes: cf9aa72d2f91 ("tty: serial: fsl_lpuart: optimize the timer based EOP logic")
Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
---
 drivers/tty/serial/fsl_lpuart.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
index c1980ea52666..07b3b26732db 100644
--- a/drivers/tty/serial/fsl_lpuart.c
+++ b/drivers/tty/serial/fsl_lpuart.c
@@ -1373,7 +1373,7 @@ static inline int lpuart_start_rx_dma(struct lpuart_port *sport)
 
 	sport->last_residue = 0;
 	sport->dma_rx_timeout = max(nsecs_to_jiffies(
-		sport->port.frame_time * DMA_RX_IDLE_CHARS), 1UL);
+		(u64)sport->port.frame_time * DMA_RX_IDLE_CHARS), 1UL);
 
 	ring->buf = kzalloc(sport->rx_dma_rng_buf_len, GFP_ATOMIC);
 	if (!ring->buf)
-- 
2.17.1

Re: [PATCH] tty: serial: fsl_lpuart: Fix possible integer overflow

[Jiri Slaby]

On 18. 07. 23, 8:56, Sherry Sun wrote:
> This patch addresses the following Coverity report, fix it by casting
> sport->port.frame_time to type u64.
> 
> CID 32305660: Unintentional integer overflow (OVERFLOW_BEFORE_WIDEN)
> Potentially overflowing expression sport->port.frame_time * 8U with type
> unsigned int (32 bits, unsigned) is evaluated using 32-bit arithmetic,
> and then used in a context that expects an expression of type u64 (64
> bits, unsigned).
> 
> Fixes: cf9aa72d2f91 ("tty: serial: fsl_lpuart: optimize the timer based EOP logic")
> Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
> ---
>   drivers/tty/serial/fsl_lpuart.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/tty/serial/fsl_lpuart.c b/drivers/tty/serial/fsl_lpuart.c
> index c1980ea52666..07b3b26732db 100644
> --- a/drivers/tty/serial/fsl_lpuart.c
> +++ b/drivers/tty/serial/fsl_lpuart.c
> @@ -1373,7 +1373,7 @@ static inline int lpuart_start_rx_dma(struct lpuart_port *sport)
>   
>   	sport->last_residue = 0;
>   	sport->dma_rx_timeout = max(nsecs_to_jiffies(
> -		sport->port.frame_time * DMA_RX_IDLE_CHARS), 1UL);
> +		(u64)sport->port.frame_time * DMA_RX_IDLE_CHARS), 1UL);

Can you explain how that can overflow? In the worst case (1 start bit, 8 
data bits, 2 stop bits, parity bit, address bit, 50 bauds), frame_time 
would contain:
13*1e9/50 = 260,000,000. (260 ms)

Then the multiplication above is:
260,000,000*8 = 2,080,000,000. (2 seconds)

which is still less than 2^32-1.

thanks,
-- 
js
suse labs

Re: [PATCH] tty: serial: fsl_lpuart: Fix possible integer overflow

[Ilpo Järvinen]

On Tue, 18 Jul 2023, Jiri Slaby wrote:

> On 18. 07. 23, 8:56, Sherry Sun wrote:
> > This patch addresses the following Coverity report, fix it by casting
> > sport->port.frame_time to type u64.
> > 
> > CID 32305660: Unintentional integer overflow (OVERFLOW_BEFORE_WIDEN)
> > Potentially overflowing expression sport->port.frame_time * 8U with type
> > unsigned int (32 bits, unsigned) is evaluated using 32-bit arithmetic,
> > and then used in a context that expects an expression of type u64 (64
> > bits, unsigned).
> > 
> > Fixes: cf9aa72d2f91 ("tty: serial: fsl_lpuart: optimize the timer based EOP
> > logic")
> > Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
> > ---
> >   drivers/tty/serial/fsl_lpuart.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/tty/serial/fsl_lpuart.c
> > b/drivers/tty/serial/fsl_lpuart.c
> > index c1980ea52666..07b3b26732db 100644
> > --- a/drivers/tty/serial/fsl_lpuart.c
> > +++ b/drivers/tty/serial/fsl_lpuart.c
> > @@ -1373,7 +1373,7 @@ static inline int lpuart_start_rx_dma(struct
> > lpuart_port *sport)
> >     	sport->last_residue = 0;
> >   	sport->dma_rx_timeout = max(nsecs_to_jiffies(
> > -		sport->port.frame_time * DMA_RX_IDLE_CHARS), 1UL);
> > +		(u64)sport->port.frame_time * DMA_RX_IDLE_CHARS), 1UL);
> 
> Can you explain how that can overflow? In the worst case (1 start bit, 8 data
> bits, 2 stop bits, parity bit, address bit, 50 bauds), frame_time would
> contain:
> 13*1e9/50 = 260,000,000. (260 ms)
> 
> Then the multiplication above is:
> 260,000,000*8 = 2,080,000,000. (2 seconds)
> 
> which is still less than 2^32-1.

I was wondering the same thing.

This isn't a real bug. All findings from code analysis tools must be 
carefully evaluated to filter wheat out of chaff and this falls into the 
latter category. Please make sure next time you understand and explain 
also in the changelog how the problem can be manifested for real before 
sending this kind of patches.


-- 
 i.

RE: [PATCH] tty: serial: fsl_lpuart: Fix possible integer overflow

[Sherry Sun]

> -----Original Message-----
> From: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> Sent: 2023年7月18日 17:56
> To: Sherry Sun <sherry.sun@nxp.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>; Shenwei Wang
> <shenwei.wang@nxp.com>; linux-serial <linux-serial@vger.kernel.org>;
> LKML <linux-kernel@vger.kernel.org>; dl-linux-imx <linux-imx@nxp.com>;
> Jiri Slaby <jirislaby@kernel.org>
> Subject: Re: [PATCH] tty: serial: fsl_lpuart: Fix possible integer overflow
> 
> On Tue, 18 Jul 2023, Jiri Slaby wrote:
> 
> > On 18. 07. 23, 8:56, Sherry Sun wrote:
> > > This patch addresses the following Coverity report, fix it by
> > > casting
> > > sport->port.frame_time to type u64.
> > >
> > > CID 32305660: Unintentional integer overflow
> (OVERFLOW_BEFORE_WIDEN)
> > > Potentially overflowing expression sport->port.frame_time * 8U with
> > > type unsigned int (32 bits, unsigned) is evaluated using 32-bit
> > > arithmetic, and then used in a context that expects an expression of
> > > type u64 (64 bits, unsigned).
> > >
> > > Fixes: cf9aa72d2f91 ("tty: serial: fsl_lpuart: optimize the timer
> > > based EOP
> > > logic")
> > > Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
> > > ---
> > >   drivers/tty/serial/fsl_lpuart.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/tty/serial/fsl_lpuart.c
> > > b/drivers/tty/serial/fsl_lpuart.c index c1980ea52666..07b3b26732db
> > > 100644
> > > --- a/drivers/tty/serial/fsl_lpuart.c
> > > +++ b/drivers/tty/serial/fsl_lpuart.c
> > > @@ -1373,7 +1373,7 @@ static inline int lpuart_start_rx_dma(struct
> > > lpuart_port *sport)
> > >     	sport->last_residue = 0;
> > >   	sport->dma_rx_timeout = max(nsecs_to_jiffies(
> > > -		sport->port.frame_time * DMA_RX_IDLE_CHARS), 1UL);
> > > +		(u64)sport->port.frame_time * DMA_RX_IDLE_CHARS), 1UL);
> >
> > Can you explain how that can overflow? In the worst case (1 start bit,
> > 8 data bits, 2 stop bits, parity bit, address bit, 50 bauds),
> > frame_time would
> > contain:
> > 13*1e9/50 = 260,000,000. (260 ms)
> >
> > Then the multiplication above is:
> > 260,000,000*8 = 2,080,000,000. (2 seconds)
> >
> > which is still less than 2^32-1.
> 
> I was wondering the same thing.
> 
> This isn't a real bug. All findings from code analysis tools must be carefully
> evaluated to filter wheat out of chaff and this falls into the latter category.
> Please make sure next time you understand and explain also in the
> changelog how the problem can be manifested for real before sending this
> kind of patches.
> 

Hi Ilpo and Jiri,
You are right, now the DMA_RX_IDLE_CHARS is 8, so even the worst case frametime won't overflow uint32.
Thanks for the reminder, I will drop the patch and pay attention next time.

Best Regards
Sherry