[PATCH 1/6] kconfig: require a space after '#' for valid input

[PATCH 1/6] kconfig: require a space after '#' for valid input

[Masahiro Yamada]

Currently, when an input line starts with '#', (line + 2) is passed to
memcmp() without checking line[1].

It means that line[1] can be any arbitrary character. For example,
"#KCONFIG_FOO is not set" is accepted as valid input, functioning the
same as "# CONFIG_FOO is not set".

More importantly, this can potentially lead to a buffer overrun if
line[1] == '\0'. It occurs if the input only contains '#', as
(line + 2) points to an uninitialized buffer.

Check line[1], and skip the line if it is not a space.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

 scripts/kconfig/confdata.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
index 2ba4dfdd1aee..556b7f087dbb 100644
--- a/scripts/kconfig/confdata.c
+++ b/scripts/kconfig/confdata.c
@@ -426,6 +426,8 @@ int conf_read_simple(const char *name, int def)
 		conf_lineno++;
 		sym = NULL;
 		if (line[0] == '#') {
+			if (line[1] != ' ')
+				continue;
 			if (memcmp(line + 2, CONFIG_, strlen(CONFIG_)))
 				continue;
 			p = strchr(line + 2 + strlen(CONFIG_), ' ');
-- 
2.40.1

[PATCH 2/6] kconfig: remove unused code for S_DEF_AUTO in conf_read_simple()

[Masahiro Yamada]

The 'else' arm here is unreachable in practical use cases.

include/config/auto.conf does not include "# CONFIG_... is not set"
line unless it is manually hacked.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

 scripts/kconfig/confdata.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
index 556b7f087dbb..92e8e37aca4d 100644
--- a/scripts/kconfig/confdata.c
+++ b/scripts/kconfig/confdata.c
@@ -436,20 +436,15 @@ int conf_read_simple(const char *name, int def)
 			*p++ = 0;
 			if (strncmp(p, "is not set", 10))
 				continue;
-			if (def == S_DEF_USER) {
-				sym = sym_find(line + 2 + strlen(CONFIG_));
-				if (!sym) {
-					if (warn_unknown)
-						conf_warning("unknown symbol: %s",
-							     line + 2 + strlen(CONFIG_));
 
-					conf_set_changed(true);
-					continue;
-				}
-			} else {
-				sym = sym_lookup(line + 2 + strlen(CONFIG_), 0);
-				if (sym->type == S_UNKNOWN)
-					sym->type = S_BOOLEAN;
+			sym = sym_find(line + 2 + strlen(CONFIG_));
+			if (!sym) {
+				if (warn_unknown)
+					conf_warning("unknown symbol: %s",
+						     line + 2 + strlen(CONFIG_));
+
+				conf_set_changed(true);
+				continue;
 			}
 			if (sym->flags & def_flags) {
 				conf_warning("override: reassigning to symbol %s", sym->name);
-- 
2.40.1

[PATCH 3/6] kconfig: deduplicate code in conf_read_simple()

[Masahiro Yamada]

Kconfig accepts both "# CONFIG_FOO is not set" and "CONFIG_FOO=n" as
a valid input, but conf_read_simple() duplicates similar code to handle
them. Factor out the common code.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

 scripts/kconfig/confdata.c | 89 +++++++++++++++-----------------------
 1 file changed, 35 insertions(+), 54 deletions(-)

diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
index 92e8e37aca4d..b6a90f6baea1 100644
--- a/scripts/kconfig/confdata.c
+++ b/scripts/kconfig/confdata.c
@@ -342,11 +342,10 @@ int conf_read_simple(const char *name, int def)
 	FILE *in = NULL;
 	char   *line = NULL;
 	size_t  line_asize = 0;
-	char *p, *p2;
+	char *p, *p2, *val;
 	struct symbol *sym;
 	int i, def_flags;
-	const char *warn_unknown;
-	const char *werror;
+	const char *warn_unknown, *werror, *sym_name;
 
 	warn_unknown = getenv("KCONFIG_WARN_UNKNOWN_SYMBOLS");
 	werror = getenv("KCONFIG_WERROR");
@@ -424,77 +423,34 @@ int conf_read_simple(const char *name, int def)
 
 	while (compat_getline(&line, &line_asize, in) != -1) {
 		conf_lineno++;
-		sym = NULL;
 		if (line[0] == '#') {
 			if (line[1] != ' ')
 				continue;
-			if (memcmp(line + 2, CONFIG_, strlen(CONFIG_)))
+			p = line + 2;
+			if (memcmp(p, CONFIG_, strlen(CONFIG_)))
 				continue;
-			p = strchr(line + 2 + strlen(CONFIG_), ' ');
+			sym_name = p + strlen(CONFIG_);
+			p = strchr(sym_name, ' ');
 			if (!p)
 				continue;
 			*p++ = 0;
 			if (strncmp(p, "is not set", 10))
 				continue;
 
-			sym = sym_find(line + 2 + strlen(CONFIG_));
-			if (!sym) {
-				if (warn_unknown)
-					conf_warning("unknown symbol: %s",
-						     line + 2 + strlen(CONFIG_));
-
-				conf_set_changed(true);
-				continue;
-			}
-			if (sym->flags & def_flags) {
-				conf_warning("override: reassigning to symbol %s", sym->name);
-			}
-			switch (sym->type) {
-			case S_BOOLEAN:
-			case S_TRISTATE:
-				sym->def[def].tri = no;
-				sym->flags |= def_flags;
-				break;
-			default:
-				;
-			}
+			val = "n";
 		} else if (memcmp(line, CONFIG_, strlen(CONFIG_)) == 0) {
-			p = strchr(line + strlen(CONFIG_), '=');
+			sym_name = line + strlen(CONFIG_);
+			p = strchr(sym_name, '=');
 			if (!p)
 				continue;
 			*p++ = 0;
+			val = p;
 			p2 = strchr(p, '\n');
 			if (p2) {
 				*p2-- = 0;
 				if (*p2 == '\r')
 					*p2 = 0;
 			}
-
-			sym = sym_find(line + strlen(CONFIG_));
-			if (!sym) {
-				if (def == S_DEF_AUTO) {
-					/*
-					 * Reading from include/config/auto.conf
-					 * If CONFIG_FOO previously existed in
-					 * auto.conf but it is missing now,
-					 * include/config/FOO must be touched.
-					 */
-					conf_touch_dep(line + strlen(CONFIG_));
-				} else {
-					if (warn_unknown)
-						conf_warning("unknown symbol: %s",
-							     line + strlen(CONFIG_));
-
-					conf_set_changed(true);
-				}
-				continue;
-			}
-
-			if (sym->flags & def_flags) {
-				conf_warning("override: reassigning to symbol %s", sym->name);
-			}
-			if (conf_set_sym_val(sym, def, def_flags, p))
-				continue;
 		} else {
 			if (line[0] != '\r' && line[0] != '\n')
 				conf_warning("unexpected data: %.*s",
@@ -503,6 +459,31 @@ int conf_read_simple(const char *name, int def)
 			continue;
 		}
 
+		sym = sym_find(sym_name);
+		if (!sym) {
+			if (def == S_DEF_AUTO) {
+				/*
+				 * Reading from include/config/auto.conf.
+				 * If CONFIG_FOO previously existed in auto.conf
+				 * but it is missing now, include/config/FOO
+				 * must be touched.
+				 */
+				conf_touch_dep(sym_name);
+			} else {
+				if (warn_unknown)
+					conf_warning("unknown symbol: %s", sym_name);
+
+				conf_set_changed(true);
+			}
+			continue;
+		}
+
+		if (sym->flags & def_flags)
+			conf_warning("override: reassigning to symbol %s", sym->name);
+
+		if (conf_set_sym_val(sym, def, def_flags, val))
+			continue;
+
 		if (sym && sym_is_choice_value(sym)) {
 			struct symbol *cs = prop_get_symbol(sym_get_choice_prop(sym));
 			switch (sym->def[def].tri) {
-- 
2.40.1

[PATCH 4/6] kconfig: introduce getline_stripped() helper

[Masahiro Yamada]

Currently, newline characters are stripped away in multiple places
on the caller.

Doing that in the callee is helpful for further cleanups.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

 scripts/kconfig/confdata.c | 40 +++++++++++++++++++++++++-------------
 1 file changed, 26 insertions(+), 14 deletions(-)

diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
index b6a90f6baea1..795ac6c9378f 100644
--- a/scripts/kconfig/confdata.c
+++ b/scripts/kconfig/confdata.c
@@ -337,12 +337,32 @@ static ssize_t compat_getline(char **lineptr, size_t *n, FILE *stream)
 	return -1;
 }
 
+/* like getline(), but the newline character is stripped away */
+static ssize_t getline_stripped(char **lineptr, size_t *n, FILE *stream)
+{
+	ssize_t len;
+
+	len = compat_getline(lineptr, n, stream);
+
+	if (len > 0 && (*lineptr)[len - 1] == '\n') {
+		len--;
+		(*lineptr)[len] = '\0';
+
+		if (len > 0 && (*lineptr)[len - 1] == '\r') {
+			len--;
+			(*lineptr)[len] = '\0';
+		}
+	}
+
+	return len;
+}
+
 int conf_read_simple(const char *name, int def)
 {
 	FILE *in = NULL;
 	char   *line = NULL;
 	size_t  line_asize = 0;
-	char *p, *p2, *val;
+	char *p, *val;
 	struct symbol *sym;
 	int i, def_flags;
 	const char *warn_unknown, *werror, *sym_name;
@@ -421,7 +441,7 @@ int conf_read_simple(const char *name, int def)
 		}
 	}
 
-	while (compat_getline(&line, &line_asize, in) != -1) {
+	while (getline_stripped(&line, &line_asize, in) != -1) {
 		conf_lineno++;
 		if (line[0] == '#') {
 			if (line[1] != ' ')
@@ -443,19 +463,11 @@ int conf_read_simple(const char *name, int def)
 			p = strchr(sym_name, '=');
 			if (!p)
 				continue;
-			*p++ = 0;
-			val = p;
-			p2 = strchr(p, '\n');
-			if (p2) {
-				*p2-- = 0;
-				if (*p2 == '\r')
-					*p2 = 0;
-			}
+			*p = 0;
+			val = p + 1;
 		} else {
-			if (line[0] != '\r' && line[0] != '\n')
-				conf_warning("unexpected data: %.*s",
-					     (int)strcspn(line, "\r\n"), line);
-
+			if (line[0] != '\0')
+				conf_warning("unexpected data: %s", line);
 			continue;
 		}
 
-- 
2.40.1

[PATCH 5/6] kconfig: require an exact match for "is not set" to disable CONFIG option

[Masahiro Yamada]

Currently, any string starting "is not set" disables a CONFIG option.

For example, "# CONFIG_FOO is not settled down" is accepted as valid
input, functioning the same as "# CONFIG_FOO is not set". It is a
long-standing oddity.

Check the line against the exact pattern "is not set".

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

 scripts/kconfig/confdata.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
index 795ac6c9378f..958be12cd621 100644
--- a/scripts/kconfig/confdata.c
+++ b/scripts/kconfig/confdata.c
@@ -454,7 +454,7 @@ int conf_read_simple(const char *name, int def)
 			if (!p)
 				continue;
 			*p++ = 0;
-			if (strncmp(p, "is not set", 10))
+			if (strcmp(p, "is not set"))
 				continue;
 
 			val = "n";
-- 
2.40.1

[PATCH 6/6] kconfig: massage the loop in conf_read_simple()

[Masahiro Yamada]

Make the while-loop code a little more readable.

The gain is that "CONFIG_FOO" without '=' is warned as unexpected data.

Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
---

 scripts/kconfig/confdata.c | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
index 958be12cd621..bd14aae1db58 100644
--- a/scripts/kconfig/confdata.c
+++ b/scripts/kconfig/confdata.c
@@ -443,6 +443,10 @@ int conf_read_simple(const char *name, int def)
 
 	while (getline_stripped(&line, &line_asize, in) != -1) {
 		conf_lineno++;
+
+		if (!line[0]) /* blank line */
+			continue;
+
 		if (line[0] == '#') {
 			if (line[1] != ' ')
 				continue;
@@ -458,17 +462,20 @@ int conf_read_simple(const char *name, int def)
 				continue;
 
 			val = "n";
-		} else if (memcmp(line, CONFIG_, strlen(CONFIG_)) == 0) {
+		} else {
+			if (memcmp(line, CONFIG_, strlen(CONFIG_))) {
+				conf_warning("unexpected data: %s", line);
+				continue;
+			}
+
 			sym_name = line + strlen(CONFIG_);
 			p = strchr(sym_name, '=');
-			if (!p)
+			if (!p) {
+				conf_warning("unexpected data: %s", line);
 				continue;
+			}
 			*p = 0;
 			val = p + 1;
-		} else {
-			if (line[0] != '\0')
-				conf_warning("unexpected data: %s", line);
-			continue;
 		}
 
 		sym = sym_find(sym_name);
-- 
2.40.1

Re: [PATCH 5/6] kconfig: require an exact match for "is not set" to disable CONFIG option

[Randy Dunlap]

Hi,

On 11/17/23 23:59, Masahiro Yamada wrote:
> Currently, any string starting "is not set" disables a CONFIG option.
> 
> For example, "# CONFIG_FOO is not settled down" is accepted as valid
> input, functioning the same as "# CONFIG_FOO is not set". It is a
> long-standing oddity.
> 
> Check the line against the exact pattern "is not set".
> 

Just to confirm (I hope), using:
CONFIG_FOO=n

will also still work to disable that config option?

Thanks.

> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> ---
> 
>  scripts/kconfig/confdata.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
> index 795ac6c9378f..958be12cd621 100644
> --- a/scripts/kconfig/confdata.c
> +++ b/scripts/kconfig/confdata.c
> @@ -454,7 +454,7 @@ int conf_read_simple(const char *name, int def)
>  			if (!p)
>  				continue;
>  			*p++ = 0;
> -			if (strncmp(p, "is not set", 10))
> +			if (strcmp(p, "is not set"))
>  				continue;
>  
>  			val = "n";

-- 
~Randy

Re: [PATCH 5/6] kconfig: require an exact match for "is not set" to disable CONFIG option

[Masahiro Yamada]

On Sun, Nov 19, 2023 at 1:30 AM Randy Dunlap <rdunlap@infradead.org> wrote:
>
> Hi,
>
> On 11/17/23 23:59, Masahiro Yamada wrote:
> > Currently, any string starting "is not set" disables a CONFIG option.
> >
> > For example, "# CONFIG_FOO is not settled down" is accepted as valid
> > input, functioning the same as "# CONFIG_FOO is not set". It is a
> > long-standing oddity.
> >
> > Check the line against the exact pattern "is not set".
> >
>
> Just to confirm (I hope), using:
> CONFIG_FOO=n
>
> will also still work to disable that config option?



Yes.  =n is still supported.


The code diff is
strncmp() -> strcmp().






> Thanks.
>
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > ---
> >
> >  scripts/kconfig/confdata.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
> > index 795ac6c9378f..958be12cd621 100644
> > --- a/scripts/kconfig/confdata.c
> > +++ b/scripts/kconfig/confdata.c
> > @@ -454,7 +454,7 @@ int conf_read_simple(const char *name, int def)
> >                       if (!p)
> >                               continue;
> >                       *p++ = 0;
> > -                     if (strncmp(p, "is not set", 10))
> > +                     if (strcmp(p, "is not set"))
> >                               continue;
> >
> >                       val = "n";
>
> --
> ~Randy



-- 
Best Regards
Masahiro Yamada