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

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]

Date:   Tue, 18 Jul 2023 12:56:20 +0300 (EEST)
From:   Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
To:     Sherry Sun <sherry.sun@....com>
cc:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        shenwei.wang@....com, linux-serial <linux-serial@...r.kernel.org>,
        LKML <linux-kernel@...r.kernel.org>, linux-imx@....com,
        Jiri Slaby <jirislaby@...nel.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@....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.