Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 18 Jul 2023 12:56:20 +0300 (EEST)
From:   =?ISO-8859-15?Q?Ilpo_J=E4rvinen?= <ilpo.jarvinen@linux.intel.com>
To:     Sherry Sun <sherry.sun@nxp.com>
cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        shenwei.wang@nxp.com, linux-serial <linux-serial@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>, linux-imx@nxp.com,
        Jiri Slaby <jirislaby@kernel.org>
Subject: Re: [PATCH] tty: serial: fsl_lpuart: Fix possible integer overflow
In-Reply-To: <3652da4b-8ccf-34a8-bdb7-757a3109ac54@kernel.org>
Message-ID: <e0fe2036-e22-3bb4-a5a-afab1a5869e@linux.intel.com>
References: <20230718065645.6588-1-sherry.sun@nxp.com> <3652da4b-8ccf-34a8-bdb7-757a3109ac54@kernel.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Precedence: bulk

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.