[PATCH] serial: lpuart: Avoid division by zero when requested baudrate is

[PATCH] serial: lpuart: Avoid division by zero when requested baudrate is

[Andrey Smirnov]

With serdev device support added there's now a corner case where:

 1. There is a DT node for a serdev device on one of the UARTs
 2. There is no driver that binds against serdev device's compatibility
    string

with 1 and 2 being true it is possible to end up in a situation where
a particualr UART has not been initalized to any baudrate when
clock_notifier_call_chain() gets called. This effectively translates
to

	set_baudrate(uart, 0);

which for LPUART driver result in a division by zero.

To avoid this problem, convert lpuart_serial_setbaudrate() to treat
zero baudrate as a request to disable the UART. While we are at it add
a BUG_ON() to lpuart_setbrg() to simplify finding any future bugs.

Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
 drivers/serial/serial_lpuart.c | 13 ++++++++-----
 include/serial/lpuart.h        |  2 ++
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/serial/serial_lpuart.c b/drivers/serial/serial_lpuart.c
index f28035a32..246fc3d3a 100644
--- a/drivers/serial/serial_lpuart.c
+++ b/drivers/serial/serial_lpuart.c
@@ -68,11 +68,14 @@ static int lpuart_serial_setbaudrate(struct console_device *cdev,
 
 	lpuart_enable(lpuart, false);
 
-	lpuart_setbrg(lpuart->base,
-		      clk_get_rate(lpuart->clk),
-		      baudrate);
-
-	lpuart_enable(lpuart, true);
+	/*
+	 * We treat baudrate of 0 as a request to disable UART
+	 */
+	if (baudrate) {
+		lpuart_setbrg(lpuart->base, clk_get_rate(lpuart->clk),
+			      baudrate);
+		lpuart_enable(lpuart, true);
+	}
 
 	lpuart->baudrate = baudrate;
 
diff --git a/include/serial/lpuart.h b/include/serial/lpuart.h
index a920291de..9c6e271eb 100644
--- a/include/serial/lpuart.h
+++ b/include/serial/lpuart.h
@@ -228,6 +228,8 @@ static inline void lpuart_setbrg(void __iomem *base,
 	unsigned int bfra;
 	u16 sbr;
 
+	BUG_ON(!baudrate);
+
 	sbr = (u16) (refclock / (16 * baudrate));
 
 	writeb(sbr >> 8,   base + UARTBDH);
-- 
2.17.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] serial: lpuart: Avoid division by zero when requested baudrate is

[Sascha Hauer]

On Tue, May 08, 2018 at 02:15:02PM -0700, Andrey Smirnov wrote:
> With serdev device support added there's now a corner case where:
> 
>  1. There is a DT node for a serdev device on one of the UARTs
>  2. There is no driver that binds against serdev device's compatibility
>     string
> 
> with 1 and 2 being true it is possible to end up in a situation where
> a particualr UART has not been initalized to any baudrate when
> clock_notifier_call_chain() gets called. This effectively translates
> to
> 
> 	set_baudrate(uart, 0);
> 
> which for LPUART driver result in a division by zero.

This probably leads to a division by zero for most drivers since
dividing by the baudrate is a common pattern. Wouldn't it be better to
catch this is console_set_baudrate()?

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] serial: lpuart: Avoid division by zero when requested baudrate is

[Andrey Smirnov]

On Mon, May 14, 2018 at 12:14 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> On Tue, May 08, 2018 at 02:15:02PM -0700, Andrey Smirnov wrote:
>> With serdev device support added there's now a corner case where:
>>
>>  1. There is a DT node for a serdev device on one of the UARTs
>>  2. There is no driver that binds against serdev device's compatibility
>>     string
>>
>> with 1 and 2 being true it is possible to end up in a situation where
>> a particualr UART has not been initalized to any baudrate when
>> clock_notifier_call_chain() gets called. This effectively translates
>> to
>>
>>       set_baudrate(uart, 0);
>>
>> which for LPUART driver result in a division by zero.
>
> This probably leads to a division by zero for most drivers since
> dividing by the baudrate is a common pattern. Wouldn't it be better to
> catch this is console_set_baudrate()?
>

It would, but none of the drivers that register a callback with
clock_register_client() (serial_imx.c, serial_auart.c, stm-serial.c,
serial_lpuart.c and serial_cadence.c) use console_set_badrate() and
instead call ->setbrg() directly. Probably because
console_set_baudrate() also handles interactive baudrate switch for
f_active console devices. Serial_imx.c doesn't preform any division,
so it is safe and I can go through the rest of them to add appropriate
checks? Another option is probably to try to come up with common
clock_register_client() infrastructure(since all of the handlers are
the same) and keep that check there.

Let me know if you want me to do either.

Thanks,
Andrey Smirnov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] serial: lpuart: Avoid division by zero when requested baudrate is

[Sascha Hauer]

On Mon, May 14, 2018 at 05:20:56PM -0700, Andrey Smirnov wrote:
> On Mon, May 14, 2018 at 12:14 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > On Tue, May 08, 2018 at 02:15:02PM -0700, Andrey Smirnov wrote:
> >> With serdev device support added there's now a corner case where:
> >>
> >>  1. There is a DT node for a serdev device on one of the UARTs
> >>  2. There is no driver that binds against serdev device's compatibility
> >>     string
> >>
> >> with 1 and 2 being true it is possible to end up in a situation where
> >> a particualr UART has not been initalized to any baudrate when
> >> clock_notifier_call_chain() gets called. This effectively translates
> >> to
> >>
> >>       set_baudrate(uart, 0);
> >>
> >> which for LPUART driver result in a division by zero.
> >
> > This probably leads to a division by zero for most drivers since
> > dividing by the baudrate is a common pattern. Wouldn't it be better to
> > catch this is console_set_baudrate()?
> >
> 
> It would, but none of the drivers that register a callback with
> clock_register_client() (serial_imx.c, serial_auart.c, stm-serial.c,
> serial_lpuart.c and serial_cadence.c) use console_set_badrate() and
> instead call ->setbrg() directly. Probably because
> console_set_baudrate() also handles interactive baudrate switch for
> f_active console devices. Serial_imx.c doesn't preform any division,
> so it is safe and I can go through the rest of them to add appropriate
> checks? Another option is probably to try to come up with common
> clock_register_client() infrastructure(since all of the handlers are
> the same) and keep that check there.
> 
> Let me know if you want me to do either.

I see. For the sake of simplicity I think we can go with your original
patch. In the auart and cadence driver the clock notify stuff is unused,
only copy paste leftover from copying the i.MX driver. That leaves the
lpuart driver which really has this problem.

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] serial: lpuart: Avoid division by zero when requested baudrate is

[Andrey Smirnov]

On Mon, May 14, 2018 at 11:07 PM Sascha Hauer <s.hauer@pengutronix.de> wrote:
>
> On Mon, May 14, 2018 at 05:20:56PM -0700, Andrey Smirnov wrote:
> > On Mon, May 14, 2018 at 12:14 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > > On Tue, May 08, 2018 at 02:15:02PM -0700, Andrey Smirnov wrote:
> > >> With serdev device support added there's now a corner case where:
> > >>
> > >>  1. There is a DT node for a serdev device on one of the UARTs
> > >>  2. There is no driver that binds against serdev device's compatibility
> > >>     string
> > >>
> > >> with 1 and 2 being true it is possible to end up in a situation where
> > >> a particualr UART has not been initalized to any baudrate when
> > >> clock_notifier_call_chain() gets called. This effectively translates
> > >> to
> > >>
> > >>       set_baudrate(uart, 0);
> > >>
> > >> which for LPUART driver result in a division by zero.
> > >
> > > This probably leads to a division by zero for most drivers since
> > > dividing by the baudrate is a common pattern. Wouldn't it be better to
> > > catch this is console_set_baudrate()?
> > >
> >
> > It would, but none of the drivers that register a callback with
> > clock_register_client() (serial_imx.c, serial_auart.c, stm-serial.c,
> > serial_lpuart.c and serial_cadence.c) use console_set_badrate() and
> > instead call ->setbrg() directly. Probably because
> > console_set_baudrate() also handles interactive baudrate switch for
> > f_active console devices. Serial_imx.c doesn't preform any division,
> > so it is safe and I can go through the rest of them to add appropriate
> > checks? Another option is probably to try to come up with common
> > clock_register_client() infrastructure(since all of the handlers are
> > the same) and keep that check there.
> >
> > Let me know if you want me to do either.
>
> I see. For the sake of simplicity I think we can go with your original
> patch. In the auart and cadence driver the clock notify stuff is unused,
> only copy paste leftover from copying the i.MX driver. That leaves the
> lpuart driver which really has this problem.
>

Sascha, I don't think this patch ever made it into master. I am
assuming it just slipped through the cracks. Let me know if anything
needs changing for it to be applied.

Thanks,
Andrey Smirnov

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] serial: lpuart: Avoid division by zero when requested baudrate is

[Sascha Hauer]

On Sat, Aug 11, 2018 at 12:35:39PM -0700, Andrey Smirnov wrote:
> On Mon, May 14, 2018 at 11:07 PM Sascha Hauer <s.hauer@pengutronix.de> wrote:
> >
> > On Mon, May 14, 2018 at 05:20:56PM -0700, Andrey Smirnov wrote:
> > > On Mon, May 14, 2018 at 12:14 AM, Sascha Hauer <s.hauer@pengutronix.de> wrote:
> > > > On Tue, May 08, 2018 at 02:15:02PM -0700, Andrey Smirnov wrote:
> > > >> With serdev device support added there's now a corner case where:
> > > >>
> > > >>  1. There is a DT node for a serdev device on one of the UARTs
> > > >>  2. There is no driver that binds against serdev device's compatibility
> > > >>     string
> > > >>
> > > >> with 1 and 2 being true it is possible to end up in a situation where
> > > >> a particualr UART has not been initalized to any baudrate when
> > > >> clock_notifier_call_chain() gets called. This effectively translates
> > > >> to
> > > >>
> > > >>       set_baudrate(uart, 0);
> > > >>
> > > >> which for LPUART driver result in a division by zero.
> > > >
> > > > This probably leads to a division by zero for most drivers since
> > > > dividing by the baudrate is a common pattern. Wouldn't it be better to
> > > > catch this is console_set_baudrate()?
> > > >
> > >
> > > It would, but none of the drivers that register a callback with
> > > clock_register_client() (serial_imx.c, serial_auart.c, stm-serial.c,
> > > serial_lpuart.c and serial_cadence.c) use console_set_badrate() and
> > > instead call ->setbrg() directly. Probably because
> > > console_set_baudrate() also handles interactive baudrate switch for
> > > f_active console devices. Serial_imx.c doesn't preform any division,
> > > so it is safe and I can go through the rest of them to add appropriate
> > > checks? Another option is probably to try to come up with common
> > > clock_register_client() infrastructure(since all of the handlers are
> > > the same) and keep that check there.
> > >
> > > Let me know if you want me to do either.
> >
> > I see. For the sake of simplicity I think we can go with your original
> > patch. In the auart and cadence driver the clock notify stuff is unused,
> > only copy paste leftover from copying the i.MX driver. That leaves the
> > lpuart driver which really has this problem.
> >
> 
> Sascha, I don't think this patch ever made it into master. I am
> assuming it just slipped through the cracks. Let me know if anything
> needs changing for it to be applied.

Applied now, thanks

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox