linux-kernel - Re: [PATCH v2] kconfig: nconf: stop endless search-up loops

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]

Message-ID: <9e8d429f-c21c-7d9e-0dcd-8947846fe9ba@ionic.de>
Date:   Sat, 10 Apr 2021 09:00:33 +0200
From:   Mihai Moldovan <ionic@...ic.de>
To:     Masahiro Yamada <masahiroy@...nel.org>
Cc:     Linux Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v2] kconfig: nconf: stop endless search-up loops

* On 4/10/21 7:47 AM, Masahiro Yamada wrote:
> On Sun, Mar 28, 2021 at 6:52 PM Mihai Moldovan <ionic@...ic.de> wrote:
>> +               if ((index == -1) && (index == match_start))
>> +                       return -1;
> 
> We know 'index' is -1 in the second comparison.
> So, you can also write like this:
> 
>        if (match_start == -1 && index == -1)
>                 return -1;

I know, but I sided for the other form for semantic reasons - this more closely
directly describes what we actually care about (both being the same value and
either one being -1).


> But, it is not the correct fix, either.
> 
> The root cause of the bug is match_start
> becoming -1.
> 
> 
> The following is the correct way to fix the bug
> without increasing the number of lines.
> 
> 
> 
> diff --git a/scripts/kconfig/nconf.c b/scripts/kconfig/nconf.c
> index e0f965529166..af814b39b876 100644
> [...]
> +       match_start = (match_start + items_num) % items_num;
>         index = match_start;
> -       index = (index + items_num) % items_num;

This is probably more elegant and fixes two issues at the same time: match_start
becoming -1 or n (which is likewise invalid, but was implicitly handled through
the remainder operation).

No objections from my side.



Mihai