Re: [PATCH 1/2] tty: serial: fsl_lpuart: fix the potential bug of division or modulo by zero

[Greg KH <gregkh@linuxfoundation.org>]

On Mon, Apr 26, 2021 at 11:30:47AM +0000, Sherry Sun wrote:
> Hi Greg,
> 
> > -----Original Message-----
> > From: Greg KH <gregkh@linuxfoundation.org>
> > Sent: 2021年4月26日 16:09
> > To: Sherry Sun <sherry.sun@nxp.com>
> > Cc: jirislaby@kernel.org; linux-serial@vger.kernel.org; linux-
> > kernel@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com>
> > Subject: Re: [PATCH 1/2] tty: serial: fsl_lpuart: fix the potential bug of division
> > or modulo by zero
> > 
> > On Mon, Apr 26, 2021 at 03:49:34PM +0800, Sherry Sun wrote:
> > > This issue is reported by Coverity Check.
> > > In lpuart32_console_get_options, division or modulo by zero may
> > > results in undefined behavior.
> > >
> > > Signed-off-by: Sherry Sun <sherry.sun@nxp.com>
> > > ---
> > >  drivers/tty/serial/fsl_lpuart.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > >
> > > diff --git a/drivers/tty/serial/fsl_lpuart.c
> > > b/drivers/tty/serial/fsl_lpuart.c index 794035041744..777d54b593f8
> > > 100644
> > > --- a/drivers/tty/serial/fsl_lpuart.c
> > > +++ b/drivers/tty/serial/fsl_lpuart.c
> > > @@ -2414,6 +2414,9 @@ lpuart32_console_get_options(struct lpuart_port
> > > *sport, int *baud,
> > >
> > >  	bd = lpuart32_read(&sport->port, UARTBAUD);
> > >  	bd &= UARTBAUD_SBR_MASK;
> > > +	if (!bd)
> > > +		return;
> > 
> > How can this ever happen?
> > 
> > Not to say this is a bad check, but it feels like this can't really happen in real
> > life, what code patch could create this result?
> > 
> > And have you tested this on real hardware?
> > 
> 
> Thanks for the reviewing, yes, I have tested the patchset on the real hardware.
> 
> Seems the coverity check is static scan, so cannot judge if UARTBAUD Register will be zero.
> I just found below statement in the uart reference manual: "When SBR is 1 - 8191, the baud rate equals "baud clock / ((OSR+1) × SBR)"."
> Since I am not familiar with uart, do you mean that the value of UARTBAUD Register will never be zero, so this case will not happen in real word?

Given that this never has happened with hardware for such an old device,
perhaps it is impossible.  But it would be good to check.

> If yes, I will drop this patch.

Handling "bad data" from hardware is never a bad idea, so I don't
necessarily want to drop this patch, I just want to try to figure out if
this is a "incase the hardware is broken/malicious" type of change, vs.
a "this bug we are seeing in real hardware" type of change.

thanks,

greg k-h