| 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: <CAABy=s3_mjevo7U5w-F35DPNGzooht4OW1nTDrU+DttZUhpmkA@mail.gmail.com>
Date: Thu, 20 Jun 2024 12:46:37 -0700
From: Yifan Hong <elsk@...gle.com>
To: Masahiro Yamada <masahiroy@...nel.org>
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 Tue, Jun 18, 2024 at 6:37 PM Masahiro Yamada <masahiroy@...nel.org> wrote:
>
> 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.
Thanks for your reply, Masahiro!
Do you mean to delete the file/lineno for "selected by" only, or for the whole
sym_check_print_recursive function? It appears to me that some file/lineno
information (Like `Kconfig:1: symbol FOO` above) is still useful. I
agree that the
`Kconfig:4` is wrong, though.
I think a segfault indicates that the program is
not robust against bad user input. In my opinion, it should fail gracefully with
some information to tell the user. I wish I can fix this properly; but
unfortunately
I am not too familiar with this code. That was why I only provided a
mechanical fix.
I am happy to remove the file/lineno's; I just want to confirm which to remove.
>
> 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
On Tue, Jun 18, 2024 at 6:37 PM Masahiro Yamada <masahiroy@...nel.org> wrote:
>
> 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