From mboxrd@z Thu Jan 1 00:00:00 1970 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754871AbeAML7a (ORCPT + 1 other); Sat, 13 Jan 2018 06:59:30 -0500 Received: from mail-wr0-f194.google.com ([209.85.128.194]:42367 "EHLO mail-wr0-f194.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754820AbeAML71 (ORCPT ); Sat, 13 Jan 2018 06:59:27 -0500 X-Google-Smtp-Source: ACJfBot3mWjNh318fH3UOk36/BK4fF6y9TG0pTQVL227y2ONzjI0nGRDsTxwg0IHI7F2rEDHRXLqocVUOITUQEa7fKI= MIME-Version: 1.0 In-Reply-To: <20180112134507.9030-1-ed.blake@sondrel.com> References: <20180112134507.9030-1-ed.blake@sondrel.com> From: =?UTF-8?Q?Nuno_Gon=C3=A7alves?= Date: Sat, 13 Jan 2018 12:59:05 +0100 Message-ID: Subject: Re: [PATCH] serial: 8250_dw: Avoid overflow in dw8250_set_termios To: Ed Blake Cc: gregkh@linuxfoundation.org, linux-kernel@vger.kernel.org, linux-serial@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: Dear Ed, Thanks. Tested-by: Nuno Goncalves 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 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 > Signed-off-by: Ed Blake > --- > 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 >