linux-kernel - Re: [PATCH] 8250_dw: do not int overflow when rate can not be aplied

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 11 Jan 2018 17:48:47 +0000
From:   Ed Blake <ed.blake@...drel.com>
To:     Nuno Gonçalves <nunojpg@...il.com>
Cc:     gregkh@...uxfoundation.org, linux-kernel@...r.kernel.org,
        linux-serial@...r.kernel.org
Subject: Re: [PATCH] 8250_dw: do not int overflow when rate can not be aplied

On 11/01/18 17:28, Nuno Gonçalves wrote:
> I have to disagree :)
>
> if (rate < i * min_rate) is true to i=a, then
>
> (rate >= i * min_rate && rate <= i * max_rate) will always be false
> for any i=b, where b>a.

No, because 'rate' is assigned from clk_round_rate() each iteration.

The idea of this code is to iterate through integer multiples of baud *
16 until you find an achievable rate that is within the +/- 1.6% range. 
Until then, the rate returned from clk_round_rate() could be lower than
i * min_rate or higher than i * max_rate, in which case you keep going.

> If this condition is true, it means the old condition would be always
> false for the remaining of the iteration.
>
> My patch "only" avoids integer overflow and terminates the search as
> soon as possible, since no need to search for bigger dividers if the
> current one would already mean a rate below min_rate (that it, this is
> the closer).

It terminates the search as soon as the rate returned from
clk_round_rate() is lower than i * min_rate, which is too soon.

> So in fact we also break when the closer divider have been found.
>
> Thanks,
> Nuno
>
> On Thu, Jan 11, 2018 at 6:18 PM, Ed Blake <ed.blake@...drel.com> wrote:
>> Hi Nuno,
>>
>> Thanks for reporting this and the patch.
>>
>> On 11/01/18 13:38, Nuno Goncalves wrote:
>>> When target_rate is big enough and not permitted in hardware,
>>> then i is looped to UART_DIV_MAX (0xFFFF), and i * max_rate will overflow
>>> (32b signed).
>>>
>>> A fix is to quit the loop early enough, as soon as rate < i * min_rate as it
>>> means the rate is not permitted.
>> 'rate < i * min_rate' does not mean the rate is not permitted.  clk_round_rate() gives the nearest achievable rate to the one requested, which may be lower than i * min_rate.  This is not an error and in this case we want to continue the loop searching for an acceptable rate.
>>
>>
>>> This avoids arbitraty rates to be applied. Still in my hardware the max
>>> allowed rate (1500000) is aplied when a higher is requested. This seems a
>>> artifact of clk_round_rate which is not understood by me and independent of
>>> this fix. Might or might not be another bug.
>>>
>>> Signed-off-by: Nuno Goncalves <nunojpg@...il.com>
>>> ---
>>>  drivers/tty/serial/8250/8250_dw.c | 8 +++++++-
>>>  1 file changed, 7 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
>>> index 5bb0c42c88dd..a27ea916abbf 100644
>>> --- a/drivers/tty/serial/8250/8250_dw.c
>>> +++ b/drivers/tty/serial/8250/8250_dw.c
>>> @@ -267,7 +267,13 @@ static void dw8250_set_termios(struct uart_port *p, struct ktermios *termios,
>>>
>>>       for (i = 1; i <= UART_DIV_MAX; i++) {
>>>               rate = clk_round_rate(d->clk, i * target_rate);
>>> -             if (rate >= i * min_rate && rate <= i * max_rate)
>>> +
>>> +             if (rate < i * min_rate) {
>>> +                     i = UART_DIV_MAX + 1;
>>> +                     break;
>>> +             }
>>> +
>>> +             if (rate <= i * max_rate)
>>>                       break;
>>>       }
>>>       if (i <= UART_DIV_MAX) {
>> --
>> Ed
>>

-- 
Ed