[PATCH v2] kbuild: Don't source kernel config

[PATCH v2] kbuild: Don't source kernel config

[Richard Weinberger]

Don't source the kernel config file in shell scripts.
The config file is not a shell script and often imported from untrusted
sources.
What could possible go wrong? ;-)

Instead, read config file line by line and access config entries using a bash
array.

Cc: Sam Ravnborg <sam@ravnborg.org>
Cc: Arnaud Lacombe <lacombar@gmail.com>
Cc: Nick Bowler <nbowler@elliptictech.com>
Cc: Michal Marek <mmarek@suse.cz>
Cc: Nicolas Pitre <nico@linaro.org>
Cc: Rusty Russell <rusty@rustcorp.com.au>
Fixes: 23121ca2b56b ("kbuild: create/adjust generated/autoksyms.h")
Fixes: 1f2bfbd00e46 ("kbuild: link of vmlinux moved to a script")
Signed-off-by: Richard Weinberger <richard@nod.at>
---
Changes since v1:
 - Fixed out of tree build
---
 scripts/adjust_autoksyms.sh | 13 +++----------
 scripts/importkconf.sh      | 14 ++++++++++++++
 scripts/link-vmlinux.sh     | 23 ++++++++---------------
 3 files changed, 25 insertions(+), 25 deletions(-)
 create mode 100755 scripts/importkconf.sh

diff --git a/scripts/adjust_autoksyms.sh b/scripts/adjust_autoksyms.sh
index 513da1a4a2da..b72a8a0bf08a 100755
--- a/scripts/adjust_autoksyms.sh
+++ b/scripts/adjust_autoksyms.sh
@@ -39,14 +39,7 @@ case "$KBUILD_VERBOSE" in
 esac
 
 # We need access to CONFIG_ symbols
-case "${KCONFIG_CONFIG}" in
-*/*)
-	. "${KCONFIG_CONFIG}"
-	;;
-*)
-	# Force using a file from the current directory
-	. "./${KCONFIG_CONFIG}"
-esac
+. ${KBUILD_SRC}/scripts/importkconf.sh
 
 # In case it doesn't exist yet...
 if [ -e "$cur_ksyms_file" ]; then touch "$cur_ksyms_file"; fi
@@ -62,14 +55,14 @@ EOT
 [ "$(ls -A "$MODVERDIR")" ] &&
 sed -ns -e '3{s/ /\n/g;/^$/!p;}' "$MODVERDIR"/*.mod | sort -u |
 while read sym; do
-	if [ -n "$CONFIG_HAVE_UNDERSCORE_SYMBOL_PREFIX" ]; then
+	if [ -n "${KERNEL_CONFIG[CONFIG_HAVE_UNDERSCORE_SYMBOL_PREFIX]}" ]; then
 		sym="${sym#_}"
 	fi
 	echo "#define __KSYM_${sym} 1"
 done >> "$new_ksyms_file"
 
 # Special case for modversions (see modpost.c)
-if [ -n "$CONFIG_MODVERSIONS" ]; then
+if [ -n "${KERNEL_CONFIG[CONFIG_MODVERSIONS]}" ]; then
 	echo "#define __KSYM_module_layout 1" >> "$new_ksyms_file"
 fi
 
diff --git a/scripts/importkconf.sh b/scripts/importkconf.sh
new file mode 100755
index 000000000000..755a9a2e9c65
--- /dev/null
+++ b/scripts/importkconf.sh
@@ -0,0 +1,14 @@
+#!/bin/bash
+#
+# helper script which reads all kconfig keys from the kernel .config file into
+# a bash associative array.
+# By testing ${KERNEL_CONFIG[CONFIG_FOO_BAR]} shell scripts can check whether
+# CONFIG_FOO_BAR is set in .config or not.
+#
+
+declare -A KERNEL_CONFIG
+
+for cfg_ent in $(awk -F= '/^CONFIG_[A-Z0-9_]+=/{print $1}' < ${KCONFIG_CONFIG})
+do
+	KERNEL_CONFIG[${cfg_ent}]="$cfg_ent"
+done
diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
index c0d129d7f430..f48231f16c2f 100755
--- a/scripts/link-vmlinux.sh
+++ b/scripts/link-vmlinux.sh
@@ -55,7 +55,7 @@ info()
 #
 archive_builtin()
 {
-	if [ -n "${CONFIG_THIN_ARCHIVES}" ]; then
+	if [ -n "${KERNEL_CONFIG[CONFIG_THIN_ARCHIVES]}" ]; then
 		info AR built-in.o
 		rm -f built-in.o;
 		${AR} rcsTP${KBUILD_ARFLAGS} built-in.o			\
@@ -70,7 +70,7 @@ modpost_link()
 {
 	local objects
 
-	if [ -n "${CONFIG_THIN_ARCHIVES}" ]; then
+	if [ -n "${KERNEL_CONFIG[CONFIG_THIN_ARCHIVES]}" ]; then
 		objects="--whole-archive				\
 			built-in.o					\
 			--no-whole-archive				\
@@ -96,7 +96,7 @@ vmlinux_link()
 	local objects
 
 	if [ "${SRCARCH}" != "um" ]; then
-		if [ -n "${CONFIG_THIN_ARCHIVES}" ]; then
+		if [ -n "${KERNEL_CONFIG[CONFIG_THIN_ARCHIVES]}" ]; then
 			objects="--whole-archive			\
 				built-in.o				\
 				--no-whole-archive			\
@@ -116,7 +116,7 @@ vmlinux_link()
 		${LD} ${LDFLAGS} ${LDFLAGS_vmlinux} -o ${2}		\
 			-T ${lds} ${objects}
 	else
-		if [ -n "${CONFIG_THIN_ARCHIVES}" ]; then
+		if [ -n "${KERNEL_CONFIG[CONFIG_THIN_ARCHIVES]}" ]; then
 			objects="-Wl,--whole-archive			\
 				built-in.o				\
 				-Wl,--no-whole-archive			\
@@ -226,14 +226,7 @@ if [ "$1" = "clean" ]; then
 fi
 
 # We need access to CONFIG_ symbols
-case "${KCONFIG_CONFIG}" in
-*/*)
-	. "${KCONFIG_CONFIG}"
-	;;
-*)
-	# Force using a file from the current directory
-	. "./${KCONFIG_CONFIG}"
-esac
+. ${KBUILD_SRC}/scripts/importkconf.sh
 
 # Update version
 info GEN .version
@@ -259,7 +252,7 @@ ${MAKE} -f "${srctree}/scripts/Makefile.modpost" vmlinux.o
 
 kallsymso=""
 kallsyms_vmlinux=""
-if [ -n "${CONFIG_KALLSYMS}" ]; then
+if [ -n "${KERNEL_CONFIG[CONFIG_KALLSYMS]}" ]; then
 
 	# kallsyms support
 	# Generate section listing all symbols and add it into vmlinux
@@ -312,7 +305,7 @@ fi
 info LD vmlinux
 vmlinux_link "${kallsymso}" vmlinux
 
-if [ -n "${CONFIG_BUILDTIME_EXTABLE_SORT}" ]; then
+if [ -n "${KERNEL_CONFIG[CONFIG_BUILDTIME_EXTABLE_SORT]}" ]; then
 	info SORTEX vmlinux
 	sortextable vmlinux
 fi
@@ -321,7 +314,7 @@ info SYSMAP System.map
 mksysmap vmlinux System.map
 
 # step a (see comment above)
-if [ -n "${CONFIG_KALLSYMS}" ]; then
+if [ -n "${KERNEL_CONFIG[CONFIG_KALLSYMS]}" ]; then
 	mksysmap ${kallsyms_vmlinux} .tmp_System.map
 
 	if ! cmp -s System.map .tmp_System.map; then
-- 
2.13.6

Re: [PATCH v2] kbuild: Don't source kernel config

[Masahiro Yamada]

2018-02-19 18:22 GMT+09:00 Richard Weinberger <richard@nod.at>:
> Don't source the kernel config file in shell scripts.
> The config file is not a shell script and often imported from untrusted
> sources.
> What could possible go wrong? ;-)


Please enumerate your real problems.


> Instead, read config file line by line and access config entries using a bash
> array.
>
> Cc: Sam Ravnborg <sam@ravnborg.org>
> Cc: Arnaud Lacombe <lacombar@gmail.com>
> Cc: Nick Bowler <nbowler@elliptictech.com>
> Cc: Michal Marek <mmarek@suse.cz>
> Cc: Nicolas Pitre <nico@linaro.org>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Fixes: 23121ca2b56b ("kbuild: create/adjust generated/autoksyms.h")
> Fixes: 1f2bfbd00e46 ("kbuild: link of vmlinux moved to a script")
> Signed-off-by: Richard Weinberger <richard@nod.at>
> ---
> Changes since v1:
>  - Fixed out of tree build
> ---
>  scripts/adjust_autoksyms.sh | 13 +++----------
>  scripts/importkconf.sh      | 14 ++++++++++++++
>  scripts/link-vmlinux.sh     | 23 ++++++++---------------
>  3 files changed, 25 insertions(+), 25 deletions(-)
>  create mode 100755 scripts/importkconf.sh
>
> diff --git a/scripts/adjust_autoksyms.sh b/scripts/adjust_autoksyms.sh
> index 513da1a4a2da..b72a8a0bf08a 100755
> --- a/scripts/adjust_autoksyms.sh
> +++ b/scripts/adjust_autoksyms.sh
> @@ -39,14 +39,7 @@ case "$KBUILD_VERBOSE" in
>  esac
>
>  # We need access to CONFIG_ symbols
> -case "${KCONFIG_CONFIG}" in
> -*/*)
> -       . "${KCONFIG_CONFIG}"
> -       ;;
> -*)
> -       # Force using a file from the current directory
> -       . "./${KCONFIG_CONFIG}"
> -esac
> +. ${KBUILD_SRC}/scripts/importkconf.sh
>
>  # In case it doesn't exist yet...
>  if [ -e "$cur_ksyms_file" ]; then touch "$cur_ksyms_file"; fi
> @@ -62,14 +55,14 @@ EOT
>  [ "$(ls -A "$MODVERDIR")" ] &&
>  sed -ns -e '3{s/ /\n/g;/^$/!p;}' "$MODVERDIR"/*.mod | sort -u |
>  while read sym; do
> -       if [ -n "$CONFIG_HAVE_UNDERSCORE_SYMBOL_PREFIX" ]; then
> +       if [ -n "${KERNEL_CONFIG[CONFIG_HAVE_UNDERSCORE_SYMBOL_PREFIX]}" ]; then
>                 sym="${sym#_}"
>         fi
>         echo "#define __KSYM_${sym} 1"
>  done >> "$new_ksyms_file"
>
>  # Special case for modversions (see modpost.c)
> -if [ -n "$CONFIG_MODVERSIONS" ]; then
> +if [ -n "${KERNEL_CONFIG[CONFIG_MODVERSIONS]}" ]; then
>         echo "#define __KSYM_module_layout 1" >> "$new_ksyms_file"
>  fi
>
> diff --git a/scripts/importkconf.sh b/scripts/importkconf.sh
> new file mode 100755
> index 000000000000..755a9a2e9c65
> --- /dev/null
> +++ b/scripts/importkconf.sh
> @@ -0,0 +1,14 @@
> +#!/bin/bash
> +#
> +# helper script which reads all kconfig keys from the kernel .config file into
> +# a bash associative array.
> +# By testing ${KERNEL_CONFIG[CONFIG_FOO_BAR]} shell scripts can check whether
> +# CONFIG_FOO_BAR is set in .config or not.
> +#
> +
> +declare -A KERNEL_CONFIG
> +
> +for cfg_ent in $(awk -F= '/^CONFIG_[A-Z0-9_]+=/{print $1}' < ${KCONFIG_CONFIG})
> +do
> +       KERNEL_CONFIG[${cfg_ent}]="$cfg_ent"
> +done
> diff --git a/scripts/link-vmlinux.sh b/scripts/link-vmlinux.sh
> index c0d129d7f430..f48231f16c2f 100755
> --- a/scripts/link-vmlinux.sh
> +++ b/scripts/link-vmlinux.sh
> @@ -55,7 +55,7 @@ info()
>  #
>  archive_builtin()
>  {
> -       if [ -n "${CONFIG_THIN_ARCHIVES}" ]; then
> +       if [ -n "${KERNEL_CONFIG[CONFIG_THIN_ARCHIVES]}" ]; then
>                 info AR built-in.o
>                 rm -f built-in.o;
>                 ${AR} rcsTP${KBUILD_ARFLAGS} built-in.o                 \
> @@ -70,7 +70,7 @@ modpost_link()
>  {
>         local objects
>
> -       if [ -n "${CONFIG_THIN_ARCHIVES}" ]; then
> +       if [ -n "${KERNEL_CONFIG[CONFIG_THIN_ARCHIVES]}" ]; then
>                 objects="--whole-archive                                \
>                         built-in.o                                      \
>                         --no-whole-archive                              \
> @@ -96,7 +96,7 @@ vmlinux_link()
>         local objects
>
>         if [ "${SRCARCH}" != "um" ]; then
> -               if [ -n "${CONFIG_THIN_ARCHIVES}" ]; then
> +               if [ -n "${KERNEL_CONFIG[CONFIG_THIN_ARCHIVES]}" ]; then
>                         objects="--whole-archive                        \
>                                 built-in.o                              \
>                                 --no-whole-archive                      \
> @@ -116,7 +116,7 @@ vmlinux_link()
>                 ${LD} ${LDFLAGS} ${LDFLAGS_vmlinux} -o ${2}             \
>                         -T ${lds} ${objects}
>         else
> -               if [ -n "${CONFIG_THIN_ARCHIVES}" ]; then
> +               if [ -n "${KERNEL_CONFIG[CONFIG_THIN_ARCHIVES]}" ]; then
>                         objects="-Wl,--whole-archive                    \
>                                 built-in.o                              \
>                                 -Wl,--no-whole-archive                  \
> @@ -226,14 +226,7 @@ if [ "$1" = "clean" ]; then
>  fi
>
>  # We need access to CONFIG_ symbols
> -case "${KCONFIG_CONFIG}" in
> -*/*)
> -       . "${KCONFIG_CONFIG}"
> -       ;;
> -*)
> -       # Force using a file from the current directory
> -       . "./${KCONFIG_CONFIG}"
> -esac
> +. ${KBUILD_SRC}/scripts/importkconf.sh
>
>  # Update version
>  info GEN .version
> @@ -259,7 +252,7 @@ ${MAKE} -f "${srctree}/scripts/Makefile.modpost" vmlinux.o
>
>  kallsymso=""
>  kallsyms_vmlinux=""
> -if [ -n "${CONFIG_KALLSYMS}" ]; then
> +if [ -n "${KERNEL_CONFIG[CONFIG_KALLSYMS]}" ]; then
>
>         # kallsyms support
>         # Generate section listing all symbols and add it into vmlinux
> @@ -312,7 +305,7 @@ fi
>  info LD vmlinux
>  vmlinux_link "${kallsymso}" vmlinux
>
> -if [ -n "${CONFIG_BUILDTIME_EXTABLE_SORT}" ]; then
> +if [ -n "${KERNEL_CONFIG[CONFIG_BUILDTIME_EXTABLE_SORT]}" ]; then
>         info SORTEX vmlinux
>         sortextable vmlinux
>  fi
> @@ -321,7 +314,7 @@ info SYSMAP System.map
>  mksysmap vmlinux System.map
>
>  # step a (see comment above)
> -if [ -n "${CONFIG_KALLSYMS}" ]; then
> +if [ -n "${KERNEL_CONFIG[CONFIG_KALLSYMS]}" ]; then
>         mksysmap ${kallsyms_vmlinux} .tmp_System.map
>
>         if ! cmp -s System.map .tmp_System.map; then
> --
> 2.13.6
>



-- 
Best Regards
Masahiro Yamada

Re: [PATCH v2] kbuild: Don't source kernel config

[Richard Weinberger]

Am Dienstag, 20. Februar 2018, 16:18:11 CET schrieb Masahiro Yamada:
> 2018-02-19 18:22 GMT+09:00 Richard Weinberger <richard@nod.at>:
> > Don't source the kernel config file in shell scripts.
> > The config file is not a shell script and often imported from untrusted
> > sources.
> > What could possible go wrong? ;-)
> 
> Please enumerate your real problems.

Build a kernel where the .config contains something like:
CONFIG_CMDLINE_BOOL=y
CONFIG_CMDLINE="`echo hello > world`"

I'll send a v3 because I forgot to convert one function in the shell script to 
the new bash array. kbuild bot FTW. :-)

Thanks,
//richard

Re: [PATCH v2] kbuild: Don't source kernel config

[Masahiro Yamada]

2018-02-21 0:25 GMT+09:00 Richard Weinberger <richard@nod.at>:
> Am Dienstag, 20. Februar 2018, 16:18:11 CET schrieb Masahiro Yamada:
>> 2018-02-19 18:22 GMT+09:00 Richard Weinberger <richard@nod.at>:
>> > Don't source the kernel config file in shell scripts.
>> > The config file is not a shell script and often imported from untrusted
>> > sources.
>> > What could possible go wrong? ;-)
>>
>> Please enumerate your real problems.
>
> Build a kernel where the .config contains something like:
> CONFIG_CMDLINE_BOOL=y
> CONFIG_CMDLINE="`echo hello > world`"


Same for Makefile
if a string symbol is referenced from Makefile, like

CONFIG_CROSS_COMPILE="$(shell echo hello > world)aarch64-linux-gnu-"



> I'll send a v3 because I forgot to convert one function in the shell script to
> the new bash array. kbuild bot FTW. :-)

You do not need to do so.

This patch is so ugly.

Also, changed shell scripts have '#!/bin/sh' shebang,
but you are adding bash as a requirement.




-- 
Best Regards
Masahiro Yamada

Re: [PATCH v2] kbuild: Don't source kernel config

[Richard Weinberger]

Am Dienstag, 20. Februar 2018, 17:00:39 CET schrieb Masahiro Yamada:
> 2018-02-21 0:25 GMT+09:00 Richard Weinberger <richard@nod.at>:
> > Am Dienstag, 20. Februar 2018, 16:18:11 CET schrieb Masahiro Yamada:
> >> 2018-02-19 18:22 GMT+09:00 Richard Weinberger <richard@nod.at>:
> >> > Don't source the kernel config file in shell scripts.
> >> > The config file is not a shell script and often imported from untrusted
> >> > sources.
> >> > What could possible go wrong? ;-)
> >> 
> >> Please enumerate your real problems.
> > 
> > Build a kernel where the .config contains something like:
> > CONFIG_CMDLINE_BOOL=y
> > CONFIG_CMDLINE="`echo hello > world`"
> 
> Same for Makefile
> if a string symbol is referenced from Makefile, like
> 
> CONFIG_CROSS_COMPILE="$(shell echo hello > world)aarch64-linux-gnu-"

Correct. But you forget that the .config file is often imported from untrusted 
sources. Like on LKML, "my kernel explodes, this is the .config".
Jonny random Kernel developer then takes the .config and builds it... 
 
> > I'll send a v3 because I forgot to convert one function in the shell
> > script to the new bash array. kbuild bot FTW. :-)
> 
> You do not need to do so.

Okay, let's wait until toxic .configs appear in the wild. ;-)

> This patch is so ugly.
> 
> Also, changed shell scripts have '#!/bin/sh' shebang,
> but you are adding bash as a requirement.

An alternate approach would be this:
diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
index 5c12dc91ef34..ff0a7c62344b 100644
--- a/scripts/kconfig/confdata.c
+++ b/scripts/kconfig/confdata.c
@@ -161,6 +161,13 @@ static int conf_set_sym_val(struct symbol *sym, int def, 
int def_flags, char *p)
 	case S_STRING:
 		if (*p++ != '"')
 			break;
+
+		p2 = strpbrk(p, "`$");
+		if (p2 && !(p2[0] == '$' && p2[1] != '(')) {
+			conf_warning("string contains forbidden characters");
+			return 1;
+		}
+
 		for (p2 = p; (p2 = strpbrk(p2, "\"\\")); p2++) {
 			if (*p2 == '"') {
 				*p2 = 0;

That way the conf tool will sanitize the .config before shell scripts will 
source it.

Thanks,
//richard

Re: [PATCH v2] kbuild: Don't source kernel config

[Nicolas Pitre]

On Tue, 20 Feb 2018, Richard Weinberger wrote:

> An alternate approach would be this:
> diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
> index 5c12dc91ef34..ff0a7c62344b 100644
> --- a/scripts/kconfig/confdata.c
> +++ b/scripts/kconfig/confdata.c
> @@ -161,6 +161,13 @@ static int conf_set_sym_val(struct symbol *sym, int def, 
> int def_flags, char *p)
>  	case S_STRING:
>  		if (*p++ != '"')
>  			break;
> +
> +		p2 = strpbrk(p, "`$");
> +		if (p2 && !(p2[0] == '$' && p2[1] != '(')) {
> +			conf_warning("string contains forbidden characters");
> +			return 1;
> +		}
> +
>  		for (p2 = p; (p2 = strpbrk(p2, "\"\\")); p2++) {
>  			if (*p2 == '"') {
>  				*p2 = 0;
> 
> That way the conf tool will sanitize the .config before shell scripts will 
> source it.

Looks like a much saner approach to me indeed.


Nicolas

Re: [PATCH v2] kbuild: Don't source kernel config

[Masahiro Yamada]

2018-02-21 1:16 GMT+09:00 Richard Weinberger <richard@nod.at>:
> Am Dienstag, 20. Februar 2018, 17:00:39 CET schrieb Masahiro Yamada:
>> 2018-02-21 0:25 GMT+09:00 Richard Weinberger <richard@nod.at>:
>> > Am Dienstag, 20. Februar 2018, 16:18:11 CET schrieb Masahiro Yamada:
>> >> 2018-02-19 18:22 GMT+09:00 Richard Weinberger <richard@nod.at>:
>> >> > Don't source the kernel config file in shell scripts.
>> >> > The config file is not a shell script and often imported from untrusted
>> >> > sources.
>> >> > What could possible go wrong? ;-)
>> >>
>> >> Please enumerate your real problems.
>> >
>> > Build a kernel where the .config contains something like:
>> > CONFIG_CMDLINE_BOOL=y
>> > CONFIG_CMDLINE="`echo hello > world`"
>>
>> Same for Makefile
>> if a string symbol is referenced from Makefile, like
>>
>> CONFIG_CROSS_COMPILE="$(shell echo hello > world)aarch64-linux-gnu-"
>
> Correct. But you forget that the .config file is often imported from untrusted
> sources. Like on LKML, "my kernel explodes, this is the .config".
> Jonny random Kernel developer then takes the .config and builds it...
>
>> > I'll send a v3 because I forgot to convert one function in the shell
>> > script to the new bash array. kbuild bot FTW. :-)
>>
>> You do not need to do so.
>
> Okay, let's wait until toxic .configs appear in the wild. ;-)
>
>> This patch is so ugly.
>>
>> Also, changed shell scripts have '#!/bin/sh' shebang,
>> but you are adding bash as a requirement.
>
> An alternate approach would be this:
> diff --git a/scripts/kconfig/confdata.c b/scripts/kconfig/confdata.c
> index 5c12dc91ef34..ff0a7c62344b 100644
> --- a/scripts/kconfig/confdata.c
> +++ b/scripts/kconfig/confdata.c
> @@ -161,6 +161,13 @@ static int conf_set_sym_val(struct symbol *sym, int def,
> int def_flags, char *p)
>         case S_STRING:
>                 if (*p++ != '"')
>                         break;
> +
> +               p2 = strpbrk(p, "`$");
> +               if (p2 && !(p2[0] == '$' && p2[1] != '(')) {
> +                       conf_warning("string contains forbidden characters");
> +                       return 1;
> +               }
> +
>                 for (p2 = p; (p2 = strpbrk(p2, "\"\\")); p2++) {
>                         if (*p2 == '"') {
>                                 *p2 = 0;
>
> That way the conf tool will sanitize the .config before shell scripts will
> source it.


This approach seems better.




-- 
Best Regards
Masahiro Yamada

Re: [PATCH v2] kbuild: Don't source kernel config

[Richard Weinberger]

Am Dienstag, 20. Februar 2018, 17:30:10 CET schrieb Masahiro Yamada:
> > That way the conf tool will sanitize the .config before shell scripts will
> > source it.
> 
> This approach seems better.

Okay, let's go for it. :)
I went first for the ugly bash approach because it is a white list and not a 
black list filter.

Thanks,
//richard