linux-kernel - Re: [PATCH v2] kconfig: avoid an infinite loop in oldconfig/syncconfig

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:   Mon, 7 Aug 2023 13:25:07 -0700
From:   Randy Dunlap <rdunlap@...radead.org>
To:     Masahiro Yamada <masahiroy@...nel.org>,
        Yoann Congal <yoann.congal@...le.fr>
Cc:     linux-kbuild@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] kconfig: avoid an infinite loop in
 oldconfig/syncconfig



On 8/7/23 12:25, Masahiro Yamada wrote:
> On Sat, Aug 5, 2023 at 6:57 PM Yoann Congal <yoann.congal@...le.fr> wrote:
>>
>> Exit on error when asking for value that has an invalid default value
>> and stdin has reached EOF. This happens in particular for hex/int
>> configs without an explicit default value.
>>
>> Previously, this case would loop:
>> * oldconfig prompts for the value but stdin has reached EOF
>> * It gets the global default value : an empty string
>> * This is not a valid hex/int value so it prompts again, hence the infinite loop.
>>
>> This case happens with a configuration like this (a hex config without a
>> valid default value):
>>   config TEST_KCONFIG
>>        hex "Test KConfig"
>>        # default 0x0
>>
>> And using:
>>   make oldconfig < /dev/null
>>
>> This was discovered when working on Yocto bug[0] on a downstream
>> kconfig user (U-boot)
>>
>> [0]: https://bugzilla.yoctoproject.org/show_bug.cgi?id=14136
>>
>> Signed-off-by: Yoann Congal <yoann.congal@...le.fr>
>> ---
>> v1->v2:
>>  * Improve coding style
>>  * Put more info in the commit message
>>
>>  scripts/kconfig/conf.c | 10 +++++++++-
>>  1 file changed, 9 insertions(+), 1 deletion(-)
>>
>> diff --git a/scripts/kconfig/conf.c b/scripts/kconfig/conf.c
>> index 7cf63261d951c..8f32cbbce4805 100644
>> --- a/scripts/kconfig/conf.c
>> +++ b/scripts/kconfig/conf.c
>> @@ -377,8 +377,16 @@ static int conf_string(struct menu *menu)
>>                         line[strlen(line)-1] = 0;
>>                         def = line;
>>                 }
>> -               if (def && sym_set_string_value(sym, def))
>> +               if (def && sym_set_string_value(sym, def)) {
>>                         return 0;
>> +               } else {
>> +                       if (feof(stdin) && !sym_string_valid(sym, sym_get_string_value(sym))) {
>> +                               fprintf(stderr,
>> +                                       "Symbol %s has invalid default value and stdin reached EOF\n",
>> +                                       sym->name);
>> +                               exit(1);
>> +                       }
>> +               }
>>         }
>>  }
>>
>> --
>> 2.30.2
>>
> 
> 
> It is strange (and consistent) to bail out
> only for particular types.
> 

It's still very helpful to know the symbol name that is causing
the issue.

> 
> I would change the code simply as follows:
> 
> 
> 
> --- a/scripts/kconfig/conf.c
> +++ b/scripts/kconfig/conf.c
> @@ -76,8 +76,10 @@ static void strip(char *str)
>  /* Helper function to facilitate fgets() by Jean Sacren. */
>  static void xfgets(char *str, int size, FILE *in)
>  {
> -       if (!fgets(str, size, in))
> +       if (!fgets(str, size, in)) {
>                 fprintf(stderr, "\nError in reading or end of file.\n");
> +               exit(1);
> +       }
> 
>         if (!tty_stdio)
>                 printf("%s", str);
> 
> 
> 
> 
> 
> 
> yes "" | make config
> 
> will succeed.
> 
> 
> 
> make config < /dev/null
> 
> will fail.
> 
> 
> 
> 
> 
> People expecting the closed stdin to succeed
> may start complaining, but I believe
> they must fix their wrong scripts.
> 
> 

-- 
~Randy