* [PATCH] serial: 8250_dw: Avoid overflow in dw8250_set_termios @ 2018-01-12 13:45 Ed Blake 2018-01-13 11:59 ` Nuno Gonçalves 2018-01-15 20:30 ` Andy Shevchenko 0 siblings, 2 replies; 5+ messages in thread From: Ed Blake @ 2018-01-12 13:45 UTC (permalink / raw) To: gregkh, nunojpg; +Cc: linux-kernel, linux-serial, Ed Blake When searching for an achievable input clock rate that is within +/-1.6% of an integer multiple of the target baudx16 rate, there is the potential to overflow the i * rate calculations. For example, on a 32-bit system with a baud rate of 4000000, the i * max_rate calculation will overflow if i reaches 67 without finding an acceptable rate. Fix this by setting the upper boundary of the loop appropriately to avoid overflow. Reported-by: Nuno Goncalves <nunojpg@gmail.com> Signed-off-by: Ed Blake <ed.blake@sondrel.com> --- drivers/tty/serial/8250/8250_dw.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c index 5bb0c42c88dd..04b44829f0e3 100644 --- a/drivers/tty/serial/8250/8250_dw.c +++ b/drivers/tty/serial/8250/8250_dw.c @@ -252,7 +252,7 @@ static void dw8250_set_termios(struct uart_port *p, struct ktermios *termios, struct ktermios *old) { unsigned int baud = tty_termios_baud_rate(termios); - unsigned int target_rate, min_rate, max_rate; + unsigned int target_rate, min_rate, max_rate, div_max; struct dw8250_data *d = p->private_data; long rate; int i, ret; @@ -265,12 +265,14 @@ static void dw8250_set_termios(struct uart_port *p, struct ktermios *termios, min_rate = target_rate - (target_rate >> 6); max_rate = target_rate + (target_rate >> 6); - for (i = 1; i <= UART_DIV_MAX; i++) { + /* Avoid overflow */ + div_max = min(UINT_MAX / max_rate, (unsigned int)UART_DIV_MAX); + for (i = 1; i <= div_max; i++) { rate = clk_round_rate(d->clk, i * target_rate); if (rate >= i * min_rate && rate <= i * max_rate) break; } - if (i <= UART_DIV_MAX) { + if (i <= div_max) { clk_disable_unprepare(d->clk); ret = clk_set_rate(d->clk, rate); clk_prepare_enable(d->clk); -- 2.11.0 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] serial: 8250_dw: Avoid overflow in dw8250_set_termios 2018-01-12 13:45 [PATCH] serial: 8250_dw: Avoid overflow in dw8250_set_termios Ed Blake @ 2018-01-13 11:59 ` Nuno Gonçalves 2018-01-15 11:27 ` Ed Blake 2018-01-15 20:30 ` Andy Shevchenko 1 sibling, 1 reply; 5+ messages in thread From: Nuno Gonçalves @ 2018-01-13 11:59 UTC (permalink / raw) To: Ed Blake; +Cc: gregkh, linux-kernel, linux-serial Dear Ed, Thanks. Tested-by: Nuno Goncalves <nunojpg@gmail.com> I just would like to report a aditional issue I find, which I am not sure if it is intend behaviour or not. If I set bauds 1152000, 1500000, 2000000, 2500000, 3000000, I always get a actually set baud of 1500000, because it appears to be the closest baud by my hardware, but if I set 3500000, then the port gets disabled. This is because up to 3000000 the closest integer divider is 1, but then it becomes 0, which is invalid. If we still should return the closest baud, then we must return a minimum of 1, and never 0, at uart_get_divisor, or serial8250_get_divisor. Thanks, Nuno On Fri, Jan 12, 2018 at 2:45 PM, Ed Blake <ed.blake@sondrel.com> wrote: > When searching for an achievable input clock rate that is within > +/-1.6% of an integer multiple of the target baudx16 rate, there is the > potential to overflow the i * rate calculations. > > For example, on a 32-bit system with a baud rate of 4000000, the > i * max_rate calculation will overflow if i reaches 67 without finding > an acceptable rate. > > Fix this by setting the upper boundary of the loop appropriately to > avoid overflow. > > Reported-by: Nuno Goncalves <nunojpg@gmail.com> > Signed-off-by: Ed Blake <ed.blake@sondrel.com> > --- > drivers/tty/serial/8250/8250_dw.c | 8 +++++--- > 1 file changed, 5 insertions(+), 3 deletions(-) > > diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c > index 5bb0c42c88dd..04b44829f0e3 100644 > --- a/drivers/tty/serial/8250/8250_dw.c > +++ b/drivers/tty/serial/8250/8250_dw.c > @@ -252,7 +252,7 @@ static void dw8250_set_termios(struct uart_port *p, struct ktermios *termios, > struct ktermios *old) > { > unsigned int baud = tty_termios_baud_rate(termios); > - unsigned int target_rate, min_rate, max_rate; > + unsigned int target_rate, min_rate, max_rate, div_max; > struct dw8250_data *d = p->private_data; > long rate; > int i, ret; > @@ -265,12 +265,14 @@ static void dw8250_set_termios(struct uart_port *p, struct ktermios *termios, > min_rate = target_rate - (target_rate >> 6); > max_rate = target_rate + (target_rate >> 6); > > - for (i = 1; i <= UART_DIV_MAX; i++) { > + /* Avoid overflow */ > + div_max = min(UINT_MAX / max_rate, (unsigned int)UART_DIV_MAX); > + for (i = 1; i <= div_max; i++) { > rate = clk_round_rate(d->clk, i * target_rate); > if (rate >= i * min_rate && rate <= i * max_rate) > break; > } > - if (i <= UART_DIV_MAX) { > + if (i <= div_max) { > clk_disable_unprepare(d->clk); > ret = clk_set_rate(d->clk, rate); > clk_prepare_enable(d->clk); > -- > 2.11.0 > ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] serial: 8250_dw: Avoid overflow in dw8250_set_termios 2018-01-13 11:59 ` Nuno Gonçalves @ 2018-01-15 11:27 ` Ed Blake 0 siblings, 0 replies; 5+ messages in thread From: Ed Blake @ 2018-01-15 11:27 UTC (permalink / raw) To: Nuno Gonçalves; +Cc: gregkh, linux-kernel, linux-serial On 13/01/18 11:59, Nuno Gonçalves wrote: > Dear Ed, > > Thanks. > > Tested-by: Nuno Goncalves <nunojpg@gmail.com> > > I just would like to report a aditional issue I find, which I am not > sure if it is intend behaviour or not. If I set bauds 1152000, > 1500000, 2000000, 2500000, 3000000, I always get a actually set baud > of 1500000, because it appears to be the closest baud by my hardware, > but if I set 3500000, then the port gets disabled. > > This is because up to 3000000 the closest integer divider is 1, but > then it becomes 0, which is invalid. > > If we still should return the closest baud, then we must return a > minimum of 1, and never 0, at uart_get_divisor, or > serial8250_get_divisor. > > Thanks, > Nuno Yes, returning a divisor of zero doesn't sound very sensible. Care to submit a patch? -- Ed ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] serial: 8250_dw: Avoid overflow in dw8250_set_termios 2018-01-12 13:45 [PATCH] serial: 8250_dw: Avoid overflow in dw8250_set_termios Ed Blake 2018-01-13 11:59 ` Nuno Gonçalves @ 2018-01-15 20:30 ` Andy Shevchenko 2018-01-16 8:48 ` Nuno Gonçalves 1 sibling, 1 reply; 5+ messages in thread From: Andy Shevchenko @ 2018-01-15 20:30 UTC (permalink / raw) To: Ed Blake Cc: Greg Kroah-Hartman, nunojpg, Linux Kernel Mailing List, linux-serial On Fri, Jan 12, 2018 at 3:45 PM, Ed Blake <ed.blake@sondrel.com> wrote: > When searching for an achievable input clock rate that is within > +/-1.6% of an integer multiple of the target baudx16 rate, there is the > potential to overflow the i * rate calculations. > > For example, on a 32-bit system with a baud rate of 4000000, the > i * max_rate calculation will overflow if i reaches 67 without finding > an acceptable rate. > > Fix this by setting the upper boundary of the loop appropriately to > avoid overflow. > It seems the change broke Bluetooth on some Intel platforms. I'm not sure yet, but see here: https://github.com/Dunedan/mbp-2016-linux/issues/29#issuecomment-357583782 And for me the change is odd. Why by the way neither Heikki, nor me had been Cc'ed with the change? Perhaps I need to submit something to maintainer data base. For now I would rather revert it and give another try. -- With Best Regards, Andy Shevchenko ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] serial: 8250_dw: Avoid overflow in dw8250_set_termios 2018-01-15 20:30 ` Andy Shevchenko @ 2018-01-16 8:48 ` Nuno Gonçalves 0 siblings, 0 replies; 5+ messages in thread From: Nuno Gonçalves @ 2018-01-16 8:48 UTC (permalink / raw) To: Andy Shevchenko Cc: Ed Blake, Greg Kroah-Hartman, Linux Kernel Mailing List, linux-serial On Mon, Jan 15, 2018 at 9:30 PM, Andy Shevchenko <andy.shevchenko@gmail.com> wrote: > It seems the change broke Bluetooth on some Intel platforms. I'm not > sure yet, but > see here: > > https://github.com/Dunedan/mbp-2016-linux/issues/29#issuecomment-357583782 > Hi Andy, I guess you mean de9e33b [1] and not this patch (which is not in mainline). Can you double check? Thanks, Nuno [1] https://github.com/torvalds/linux/commit/de9e33bdfa22e607a88494ff21e9196d00bf4550 ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2018-01-16 8:48 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-01-12 13:45 [PATCH] serial: 8250_dw: Avoid overflow in dw8250_set_termios Ed Blake 2018-01-13 11:59 ` Nuno Gonçalves 2018-01-15 11:27 ` Ed Blake 2018-01-15 20:30 ` Andy Shevchenko 2018-01-16 8:48 ` Nuno Gonçalves
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).