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 <[email protected]>
---
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
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 <[email protected]>
> ---
> 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
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 <[email protected]>
> > ---
> > 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.
> -----Original Message-----
> From: Ilpo Järvinen <[email protected]>
> Sent: 2023年7月18日 17:56
> To: Sherry Sun <[email protected]>
> Cc: Greg Kroah-Hartman <[email protected]>; Shenwei Wang
> <[email protected]>; linux-serial <[email protected]>;
> LKML <[email protected]>; dl-linux-imx <[email protected]>;
> Jiri Slaby <[email protected]>
> 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 <[email protected]>
> > > ---
> > > 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