| 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
| ||
|
Message-ID: <CAK7LNAS2621mLaaUPSJqLPTCeowYSAXgoO9uKhF8uTeNK1jU8Q@mail.gmail.com>
Date: Wed, 19 Jun 2024 10:36:49 +0900
From: Masahiro Yamada <masahiroy@...nel.org>
To: Yifan Hong <elsk@...gle.com>
Cc: kernel-team@...roid.com, linux-kbuild@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] kconfig: Prevent segfault when getting filename
On Wed, Jun 19, 2024 at 3:56 AM Yifan Hong <elsk@...gle.com> wrote:
>
> ... and lineno in recursive checks.
>
> If the following snippet is found in Kconfig:
>
> config FOO
> tristate
> depends on BAR
> select BAR
> help
> foo
>
> ... without BAR defined; then if one runs
> `make tinyconfig`, there is a segfault.
>
> Kconfig:34:error: recursive dependency detected!
> Kconfig:34: symbol FOO depends on BAR
> make[4]: *** [scripts/kconfig/Makefile:85: allnoconfig] Segmentation fault
>
> This is because of the following. BAR is
> a fake entry created by sym_lookup() with prop
> being NULL. In the recursive check, there is a
> NULL check for prop to fall back to
> stack->sym->prop if stack->prop is NULL. However,
> in this case, stack->sym points to the fake BAR
> entry created by sym_lookup(), so prop is still
> NULL. prop was then referenced without additional
> NULL checks, causing segfault.
>
> Similarly, menu is also accessed without NULL
> checks. However, sym_lookup() creates entry
> that is not a choice, so technically it shouldn't
> fall into the state where menu is NULL for
> choices. But I mechnically apply the NULL check
> anyways for completeness.
>
> This mechnical patch avoids the segfault. The
> above snippet produces the following error with
> this patch:
>
> Kconfig:34:error: recursive dependency detected!
> Kconfig:34: symbol FOO depends on BAR
> ???:-1: symbol BAR is selected by FOO
>
> That being said, I am not sure if it is the right
> fix conceptually and in functionality.
"???:-1: symbol BAR is selected by FOO"
is weird, as there is no property
like "selected by".
It should print the file and lineno of
"select BAR".
The existing code is already wrong.
In the past, I was thinking of fixing it to reference
the relevant menu entry.
Currently, it points to an unrelated location.
[Test Code]
config FOO
bool
config BAR
bool
config FOO
bool "FOO"
depends on BAR
select BAR
$ make defconfig
*** Default configuration is based on 'x86_64_defconfig'
Kconfig:1:error: recursive dependency detected!
Kconfig:1: symbol FOO depends on BAR
Kconfig:4: symbol BAR is selected by FOO
For a resolution refer to Documentation/kbuild/kconfig-language.rst
subsection "Kconfig recursive dependency limitations"
"Kconfig:1: symbol FOO depends on BAR"
points to the other unrelated definition
because "depends on BAR" appears the second
entry starting line 7.
So, I am not keen on applying another cheap fix
to already-wrong code.
If you want to fix it now, please remove all
file/lineno logs from this function.
Then, somebody may rewrite the code some day.
>
> Signed-off-by: Yifan Hong <elsk@...gle.com>
> ---
> scripts/kconfig/symbol.c | 29 +++++++++++++++++++++--------
> 1 file changed, 21 insertions(+), 8 deletions(-)
>
> diff --git a/scripts/kconfig/symbol.c b/scripts/kconfig/symbol.c
> index 8df0a75f40b9..72ab4f274289 100644
> --- a/scripts/kconfig/symbol.c
> +++ b/scripts/kconfig/symbol.c
> @@ -1045,6 +1045,8 @@ static void sym_check_print_recursive(struct symbol *last_sym)
> struct menu *menu = NULL;
> struct property *prop;
> struct dep_stack cv_stack;
> + const char *filename = NULL;
> + int lineno = 0;
>
> if (sym_is_choice_value(last_sym)) {
> dep_stack_insert(&cv_stack, last_sym);
> @@ -1060,6 +1062,10 @@ static void sym_check_print_recursive(struct symbol *last_sym)
> }
>
> for (; stack; stack = stack->next) {
> + filename = "???";
> + lineno = 0;
> + menu = NULL;
> +
> sym = stack->sym;
> next_sym = stack->next ? stack->next->sym : last_sym;
> prop = stack->prop;
> @@ -1073,45 +1079,52 @@ static void sym_check_print_recursive(struct symbol *last_sym)
> if (prop->menu)
> break;
> }
> + if (menu) {
> + filename = menu->filename;
> + lineno = menu->lineno;
> + }
> + } else if (prop) {
> + filename = prop->filename;
> + lineno = prop->lineno;
> }
> if (stack->sym == last_sym)
> fprintf(stderr, "%s:%d:error: recursive dependency detected!\n",
> - prop->filename, prop->lineno);
> + filename, lineno);
>
> if (sym_is_choice(sym)) {
> fprintf(stderr, "%s:%d:\tchoice %s contains symbol %s\n",
> - menu->filename, menu->lineno,
> + filename, lineno,
> sym->name ? sym->name : "<choice>",
> next_sym->name ? next_sym->name : "<choice>");
> } else if (sym_is_choice_value(sym)) {
> fprintf(stderr, "%s:%d:\tsymbol %s is part of choice %s\n",
> - menu->filename, menu->lineno,
> + filename, lineno,
> sym->name ? sym->name : "<choice>",
> next_sym->name ? next_sym->name : "<choice>");
> } else if (stack->expr == &sym->dir_dep.expr) {
> fprintf(stderr, "%s:%d:\tsymbol %s depends on %s\n",
> - prop->filename, prop->lineno,
> + filename, lineno,
> sym->name ? sym->name : "<choice>",
> next_sym->name ? next_sym->name : "<choice>");
> } else if (stack->expr == &sym->rev_dep.expr) {
> fprintf(stderr, "%s:%d:\tsymbol %s is selected by %s\n",
> - prop->filename, prop->lineno,
> + filename, lineno,
> sym->name ? sym->name : "<choice>",
> next_sym->name ? next_sym->name : "<choice>");
> } else if (stack->expr == &sym->implied.expr) {
> fprintf(stderr, "%s:%d:\tsymbol %s is implied by %s\n",
> - prop->filename, prop->lineno,
> + filename, lineno,
> sym->name ? sym->name : "<choice>",
> next_sym->name ? next_sym->name : "<choice>");
> } else if (stack->expr) {
> fprintf(stderr, "%s:%d:\tsymbol %s %s value contains %s\n",
> - prop->filename, prop->lineno,
> + filename, lineno,
> sym->name ? sym->name : "<choice>",
> prop_get_type_name(prop->type),
> next_sym->name ? next_sym->name : "<choice>");
> } else {
> fprintf(stderr, "%s:%d:\tsymbol %s %s is visible depending on %s\n",
> - prop->filename, prop->lineno,
> + filename, lineno,
> sym->name ? sym->name : "<choice>",
> prop_get_type_name(prop->type),
> next_sym->name ? next_sym->name : "<choice>");
> --
> 2.45.2.627.g7a2c4fd464-goog
>
>
--
Best Regards
Masahiro Yamada
Powered by blists - more mailing lists