[Buildroot] [PATCH v2 1/7] tpm2-tss: do not enforce -fstack-protector-all

[Buildroot] [PATCH v2 1/7] tpm2-tss: do not enforce -fstack-protector-all

[Peter Korsgaard]

Stack protection is now controlled buildroot wide with the BR2_SSP_*
options, so disable the explicit -fstack-protector-all so the SSP logic in
the toolchain wrapper is used instead.

Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
---
 package/tpm2-tss/tpm2-tss.mk | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/package/tpm2-tss/tpm2-tss.mk b/package/tpm2-tss/tpm2-tss.mk
index f005e5d6b8..e2e58a092d 100644
--- a/package/tpm2-tss/tpm2-tss.mk
+++ b/package/tpm2-tss/tpm2-tss.mk
@@ -12,8 +12,9 @@ TPM2_TSS_INSTALL_STAGING = YES
 TPM2_TSS_DEPENDENCIES = liburiparser openssl host-pkgconf
 TPM2_TSS_CONF_OPTS = --with-crypto=ossl --disable-doxygen-doc
 
-# configure.ac doesn't contain a link test, so it doesn't detect when
-# libssp is missing.
-TPM2_TSS_CONF_ENV = ax_cv_check_cflags___________Wall__Werror_______fstack_protector_all=$(if $(BR2_TOOLCHAIN_HAS_SSP),yes,no)
+# -fstack-protector-all is used by default. Disable that so the
+# BR2_SSP_* options in the toolchain wrapper are used instead
+TPM2_TSS_CONF_ENV = \
+	ax_cv_check_cflags___________Wall__Werror_______fstack_protector_all=no
 
 $(eval $(autotools-package))
-- 
2.11.0

[Buildroot] [PATCH v2 2/7] tpm2-tss: fix build with BR2_FORTIFY_SOURCE_1

[Peter Korsgaard]

The configure script passes -U FORTIFY_SOURCE -D FORTIFY_SOURCE=2 by
default, which conflicts with BR2_FORTIFY_SOURCE_1 as -Werror is used:

<cross>-gcc ..  -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 .. -D_FORTIFY_SOURCE=1
<command-line>:0:0: error: "_FORTIFY_SOURCE" redefined [-Werror]

Disable this so the FORTIFY_SOURCE flags in TARGET_CFLAGS (if any) is used
instead.

Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
---
 package/tpm2-tss/tpm2-tss.mk | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/package/tpm2-tss/tpm2-tss.mk b/package/tpm2-tss/tpm2-tss.mk
index e2e58a092d..ecd9db11fa 100644
--- a/package/tpm2-tss/tpm2-tss.mk
+++ b/package/tpm2-tss/tpm2-tss.mk
@@ -12,9 +12,12 @@ TPM2_TSS_INSTALL_STAGING = YES
 TPM2_TSS_DEPENDENCIES = liburiparser openssl host-pkgconf
 TPM2_TSS_CONF_OPTS = --with-crypto=ossl --disable-doxygen-doc
 
-# -fstack-protector-all is used by default. Disable that so the
-# BR2_SSP_* options in the toolchain wrapper are used instead
+# -fstack-protector-all and FORTIFY_SOURCE=2 is used by
+# default. Disable that so the BR2_SSP_* / BR2_FORTIFY_SOURCE_* options
+# in the toolchain wrapper and CFLAGS are used instead
 TPM2_TSS_CONF_ENV = \
-	ax_cv_check_cflags___________Wall__Werror_______fstack_protector_all=no
+	ax_cv_check_cflags___________Wall__Werror_______fstack_protector_all=no \
+	ax_cv_check_ccppflags___________Wall__Werror_______U_FORTIFY_SOURCE=no \
+	ax_cv_check_ccppflags___________Wall__Werror_______D_FORTIFY_SOURCE_2=no
 
 $(eval $(autotools-package))
-- 
2.11.0

[Buildroot] [PATCH v2 3/7] tpm2-tools: always disable hardening options

[Peter Korsgaard]

Building with --enable-hardening (the default), forces -fstack-protector-all
/ FORTIFY_SOURCE=2.  These options are now controlled Buildroot wide with
the BR2_SSP_* / BR2_FORTIFY_SOURCE_* options.  Disable hardening so the
ssp/fortify settings in the toolchain wrapper / CFLAGS is used instead.

Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
---
 package/tpm2-tools/tpm2-tools.mk | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/package/tpm2-tools/tpm2-tools.mk b/package/tpm2-tools/tpm2-tools.mk
index 9c224018ce..f7e2f85228 100644
--- a/package/tpm2-tools/tpm2-tools.mk
+++ b/package/tpm2-tools/tpm2-tools.mk
@@ -10,8 +10,9 @@ TPM2_TOOLS_LICENSE = BSD-2-Clause
 TPM2_TOOLS_LICENSE_FILES = LICENSE
 TPM2_TOOLS_DEPENDENCIES = dbus libcurl libglib2 openssl tpm2-tss host-pkgconf
 
-# configure.ac doesn't contain a link test, so it doesn't detect when
-# libssp is missing.
-TPM2_TOOLS_CONF_OPTS = $(if $(BR2_TOOLCHAIN_HAS_SSP),--enable-hardening,--disable-hardening)
+# -fstack-protector-all and FORTIFY_SOURCE=2 is used by
+# default. Disable that so the BR2_SSP_* / BR2_FORTIFY_SOURCE_* options
+# in the toolchain wrapper and CFLAGS are used instead
+TPM2_TOOLS_CONF_OPTS = --disable-hardening
 
 $(eval $(autotools-package))
-- 
2.11.0

[Buildroot] [PATCH v2 4/7] tpm2-abrmd: do not enforce -fstack-protector-all

[Peter Korsgaard]

Stack protection is now controlled Buildroot wide with the BR2_SSP_*
options, so disable the explicit -fstack-protector-all so the SSP logic in
the toolchain wrapper is used instead.

Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
---
 package/tpm2-abrmd/tpm2-abrmd.mk | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/package/tpm2-abrmd/tpm2-abrmd.mk b/package/tpm2-abrmd/tpm2-abrmd.mk
index 74cc66ba20..2834615ac3 100644
--- a/package/tpm2-abrmd/tpm2-abrmd.mk
+++ b/package/tpm2-abrmd/tpm2-abrmd.mk
@@ -11,9 +11,10 @@ TPM2_ABRMD_LICENSE_FILES = LICENSE
 TPM2_ABRMD_INSTALL_STAGING = YES
 TPM2_ABRMD_DEPENDENCIES = dbus libglib2 tpm2-tss host-pkgconf
 
-# configure.ac doesn't contain a link test, so it doesn't detect when
-# libssp is missing.
-TPM2_ABRMD_CONF_ENV = ax_cv_check_cflags___________Werror_______fstack_protector_all=$(if $(BR2_TOOLCHAIN_HAS_SSP),yes,no)
+# -fstack-protector-all is used by default. Disable that so the
+# BR2_SSP_* options in the toolchain wrapper are used instead
+TPM2_ABRMD_CONF_ENV = \
+	ax_cv_check_cflags___________Werror_______fstack_protector_all=no
 
 TPM2_ABRMD_CONF_OPTS += \
 	--with-systemdsystemunitdir=$(if $(BR2_INIT_SYSTEMD),/usr/lib/systemd/system,no) \
-- 
2.11.0

[Buildroot] [PATCH v2 5/7] tpm2-abrmd: fix build with BR2_FORTIFY_SOURCE_1

[Peter Korsgaard]

The configure script passes -U FORTIFY_SOURCE -D FORTIFY_SOURCE=2 by
default, which conflicts with BR2_FORTIFY_SOURCE_1 as -Werror is used:

<cross>-gcc ..  -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 .. -D_FORTIFY_SOURCE=1
<command-line>:0:0: error: "_FORTIFY_SOURCE" redefined [-Werror]

Disable this so the FORTIFY_SOURCE flags in TARGET_CFLAGS (if any) is used
instead.

Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
---
 package/tpm2-abrmd/tpm2-abrmd.mk | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/package/tpm2-abrmd/tpm2-abrmd.mk b/package/tpm2-abrmd/tpm2-abrmd.mk
index 2834615ac3..31f19f5d01 100644
--- a/package/tpm2-abrmd/tpm2-abrmd.mk
+++ b/package/tpm2-abrmd/tpm2-abrmd.mk
@@ -11,10 +11,13 @@ TPM2_ABRMD_LICENSE_FILES = LICENSE
 TPM2_ABRMD_INSTALL_STAGING = YES
 TPM2_ABRMD_DEPENDENCIES = dbus libglib2 tpm2-tss host-pkgconf
 
-# -fstack-protector-all is used by default. Disable that so the
-# BR2_SSP_* options in the toolchain wrapper are used instead
+# -fstack-protector-all and FORTIFY_SOURCE=2 is used by
+# default. Disable that so the BR2_SSP_* / BR2_FORTIFY_SOURCE_* options
+# in the toolchain wrapper and CFLAGS are used instead
 TPM2_ABRMD_CONF_ENV = \
-	ax_cv_check_cflags___________Werror_______fstack_protector_all=no
+	ax_cv_check_cflags___________Werror_______fstack_protector_all=no \
+	ax_cv_check_ccppflags___________Werror_______U_FORTIFY_SOURCE=no \
+	ax_cv_check_ccppflags___________Werror_______D_FORTIFY_SOURCE_2=no
 
 TPM2_ABRMD_CONF_OPTS += \
 	--with-systemdsystemunitdir=$(if $(BR2_INIT_SYSTEMD),/usr/lib/systemd/system,no) \
-- 
2.11.0

[Buildroot] [PATCH v2 6/7] tpm2-tools: do not enforce dependency on tpm2-abrmd

[Peter Korsgaard]

tpm2-tools is commonly used with the resource manager, tpm2-abrmd - But it
CAN be used without, E.G.  by setting the TPM2TOOLS_TCTI_NAME environment
variable to communicate directly with the kernel driver:

export TPM2TOOLS_TCTI_NAME=device

For some use cases (E.G.  initramfs) it makes sense to use tpm2-tools
without abrmd, so downgrade the dependency from select to imply, so abrmd is
enabled by default but can be explicitly disabled.

Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
---
 package/tpm2-tools/Config.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/package/tpm2-tools/Config.in b/package/tpm2-tools/Config.in
index cc87e2a1bf..f4622b4ec9 100644
--- a/package/tpm2-tools/Config.in
+++ b/package/tpm2-tools/Config.in
@@ -8,7 +8,7 @@ config BR2_PACKAGE_TPM2_TOOLS
 	select BR2_PACKAGE_LIBCURL
 	select BR2_PACKAGE_LIBGLIB2
 	select BR2_PACKAGE_OPENSSL
-	select BR2_PACKAGE_TPM2_ABRMD # run-time
+	imply BR2_PACKAGE_TPM2_ABRMD # run-time
 	select BR2_PACKAGE_TPM2_TSS
 	help
 	  TPM (Trusted Platform Module) 2.0 CLI tools based on system
-- 
2.11.0

[Buildroot] [PATCH v2 7/7] tpm2-abrmd: S80tpm2-abrmd: create pid file at startup

[Peter Korsgaard]

The start-stop-daemon invocation to start abrmd was missing the -m (make
pidfile) option, causing stop to fail.

Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
---
 package/tpm2-abrmd/S80tpm2-abrmd | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/package/tpm2-abrmd/S80tpm2-abrmd b/package/tpm2-abrmd/S80tpm2-abrmd
index cfa9d44771..6a0bef0a31 100755
--- a/package/tpm2-abrmd/S80tpm2-abrmd
+++ b/package/tpm2-abrmd/S80tpm2-abrmd
@@ -28,7 +28,7 @@ start() {
 	printf "Starting tpm2-abrmd: "
 	check_device
 	rm_stale_pidfile /var/run/tpm2-abrmd.pid /usr/sbin/tpm2-abrmd
-	start-stop-daemon -S -q -o -b -p /var/run/tpm2-abrmd.pid -c tss:tss -x /usr/sbin/tpm2-abrmd -- ${DAEMON_OPTS} || {
+	start-stop-daemon -S -q -o -b -m -p /var/run/tpm2-abrmd.pid -c tss:tss -x /usr/sbin/tpm2-abrmd -- ${DAEMON_OPTS} || {
 		echo "FAIL"
 		exit 1
 	}
-- 
2.11.0

[Buildroot] [PATCH v2 6/7] tpm2-tools: do not enforce dependency on tpm2-abrmd

[Yann E. MORIN]

Peter, All,

On 2019-01-15 11:15 +0100, Peter Korsgaard spake thusly:
> tpm2-tools is commonly used with the resource manager, tpm2-abrmd - But it
> CAN be used without, E.G.  by setting the TPM2TOOLS_TCTI_NAME environment
> variable to communicate directly with the kernel driver:
> 
> export TPM2TOOLS_TCTI_NAME=device
> 
> For some use cases (E.G.  initramfs) it makes sense to use tpm2-tools
> without abrmd, so downgrade the dependency from select to imply, so abrmd is
> enabled by default but can be explicitly disabled.
> 
> Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
> ---
>  package/tpm2-tools/Config.in | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/package/tpm2-tools/Config.in b/package/tpm2-tools/Config.in
> index cc87e2a1bf..f4622b4ec9 100644
> --- a/package/tpm2-tools/Config.in
> +++ b/package/tpm2-tools/Config.in
> @@ -8,7 +8,7 @@ config BR2_PACKAGE_TPM2_TOOLS
>  	select BR2_PACKAGE_LIBCURL
>  	select BR2_PACKAGE_LIBGLIB2
>  	select BR2_PACKAGE_OPENSSL
> -	select BR2_PACKAGE_TPM2_ABRMD # run-time
> +	imply BR2_PACKAGE_TPM2_ABRMD # run-time

Sorry, but I reiterate my position: I don't like the use of 'imply'.

Either the thing is mandatory, in which case we select it or depend on
it, or the thing is optional, in which case we elt the user enable it.

Use of imply does not sound nice to me, because it is not authoritative.
I'm afraid we get reports of users complaining that "sometimes the stuff
is enabled when I do X, while sometmes it is not enabled when I do the
same X.'

The coutner argument has been that we were now trying to make sensible
choices for the user, so that things "work out of the box". My position
is that it is an illusion, because making things "just work" is more
often than not more involving than just enabling a package.

For example, when dealing with TPM and such: keys and certs provisionning
and checking the chain of trust and such is only scratching the surface.
People that want to deal with this topic better know what they *are* doing,
as it is a sensible topic. Those people will have to understand what they
need if they do not already know.

Regards,
Yann E. MORIN.

>  	select BR2_PACKAGE_TPM2_TSS
>  	help
>  	  TPM (Trusted Platform Module) 2.0 CLI tools based on system
> -- 
> 2.11.0
> 
> _______________________________________________
> buildroot mailing list
> buildroot at busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH v2 6/7] tpm2-tools: do not enforce dependency on tpm2-abrmd

[Peter Korsgaard]

>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:

 > Peter, All,
 > On 2019-01-15 11:15 +0100, Peter Korsgaard spake thusly:
 >> tpm2-tools is commonly used with the resource manager, tpm2-abrmd - But it
 >> CAN be used without, E.G.  by setting the TPM2TOOLS_TCTI_NAME environment
 >> variable to communicate directly with the kernel driver:
 >> 
 >> export TPM2TOOLS_TCTI_NAME=device
 >> 
 >> For some use cases (E.G.  initramfs) it makes sense to use tpm2-tools
 >> without abrmd, so downgrade the dependency from select to imply, so abrmd is
 >> enabled by default but can be explicitly disabled.
 >> 
 >> Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
 >> ---
 >> package/tpm2-tools/Config.in | 2 +-
 >> 1 file changed, 1 insertion(+), 1 deletion(-)
 >> 
 >> diff --git a/package/tpm2-tools/Config.in b/package/tpm2-tools/Config.in
 >> index cc87e2a1bf..f4622b4ec9 100644
 >> --- a/package/tpm2-tools/Config.in
 >> +++ b/package/tpm2-tools/Config.in
 >> @@ -8,7 +8,7 @@ config BR2_PACKAGE_TPM2_TOOLS
 >> select BR2_PACKAGE_LIBCURL
 >> select BR2_PACKAGE_LIBGLIB2
 >> select BR2_PACKAGE_OPENSSL
 >> -	select BR2_PACKAGE_TPM2_ABRMD # run-time
 >> +	imply BR2_PACKAGE_TPM2_ABRMD # run-time

 > Sorry, but I reiterate my position: I don't like the use of 'imply'.

 > Either the thing is mandatory, in which case we select it or depend on
 > it, or the thing is optional, in which case we elt the user enable it.

I understand you don't like it, but what is the alternative? Just
mention the optional-but-likely-to-be-needed dependency in the help
text? That is IMHO worse than imply.

For this specific case, tpm2-tools fails with a somewhat confusing error
message if tpm2-abrmd isn't available unless a specific command line
option / environment variable is used:

# tpm2_pcrlist

** (process:8628): WARNING **: 11:38:39.606: Failed to create connection with service: GDBus.Error:org.freedesktop.DBus.Error.ServiceUnknown: The name com.intel.tss2.Tabrmd was not provided by any .service files
ERROR: Failed to initialize TABRMD TCTI context: 0xa0008

The solution is to set the TCTI name to device, either through the
TPM2TOOLS_TCTI_NAME environment variable or the --tcti option.

This is imho exactly the kind of use cases imply has been made for.


 > Use of imply does not sound nice to me, because it is not authoritative.
 > I'm afraid we get reports of users complaining that "sometimes the stuff
 > is enabled when I do X, while sometmes it is not enabled when I do the
 > same X.'

Is that any different than changing toolchain options or toggling
BR2_PACKAGE_BUSYBOX_SHOW_OTHERS?


 > The coutner argument has been that we were now trying to make sensible
 > choices for the user, so that things "work out of the box". My position
 > is that it is an illusion, because making things "just work" is more
 > often than not more involving than just enabling a package.

I agree that we probably cannot do this perfectly, but a solution for
E.G. 80% of the use cases is still an improvement, as long as the
remaining 20% can still change things.

 > For example, when dealing with TPM and such: keys and certs provisionning
 > and checking the chain of trust and such is only scratching the surface.
 > People that want to deal with this topic better know what they *are* doing,
 > as it is a sensible topic. Those people will have to understand what they
 > need if they do not already know.

Sure, but we can atleast ensure that the tpm2-tools utilites do not fail
out of the box because of a missing obscure dependency and that fairly
common use cases are possible.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH v2 1/7] tpm2-tss: do not enforce -fstack-protector-all

[Peter Korsgaard]

>>>>> "Peter" == Peter Korsgaard <peter@korsgaard.com> writes:

 > Stack protection is now controlled buildroot wide with the BR2_SSP_*
 > options, so disable the explicit -fstack-protector-all so the SSP logic in
 > the toolchain wrapper is used instead.

 > Signed-off-by: Peter Korsgaard <peter@korsgaard.com>

Committed, thanks.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH v2 5/7] tpm2-abrmd: fix build with BR2_FORTIFY_SOURCE_1

[Peter Korsgaard]

>>>>> "Peter" == Peter Korsgaard <peter@korsgaard.com> writes:

 > The configure script passes -U FORTIFY_SOURCE -D FORTIFY_SOURCE=2 by
 > default, which conflicts with BR2_FORTIFY_SOURCE_1 as -Werror is used:

 > <cross>-gcc ..  -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 .. -D_FORTIFY_SOURCE=1
 > <command-line>:0:0: error: "_FORTIFY_SOURCE" redefined [-Werror]

 > Disable this so the FORTIFY_SOURCE flags in TARGET_CFLAGS (if any) is used
 > instead.

 > Signed-off-by: Peter Korsgaard <peter@korsgaard.com>

Committed, thanks.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH v2 4/7] tpm2-abrmd: do not enforce -fstack-protector-all

[Peter Korsgaard]

>>>>> "Peter" == Peter Korsgaard <peter@korsgaard.com> writes:

 > Stack protection is now controlled Buildroot wide with the BR2_SSP_*
 > options, so disable the explicit -fstack-protector-all so the SSP logic in
 > the toolchain wrapper is used instead.

 > Signed-off-by: Peter Korsgaard <peter@korsgaard.com>

Committed, thanks.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH v2 3/7] tpm2-tools: always disable hardening options

[Peter Korsgaard]

>>>>> "Peter" == Peter Korsgaard <peter@korsgaard.com> writes:

 > Building with --enable-hardening (the default), forces -fstack-protector-all
 > / FORTIFY_SOURCE=2.  These options are now controlled Buildroot wide with
 > the BR2_SSP_* / BR2_FORTIFY_SOURCE_* options.  Disable hardening so the
 > ssp/fortify settings in the toolchain wrapper / CFLAGS is used instead.

 > Signed-off-by: Peter Korsgaard <peter@korsgaard.com>

Committed, thanks.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH v2 7/7] tpm2-abrmd: S80tpm2-abrmd: create pid file at startup

[Peter Korsgaard]

>>>>> "Peter" == Peter Korsgaard <peter@korsgaard.com> writes:

 > The start-stop-daemon invocation to start abrmd was missing the -m (make
 > pidfile) option, causing stop to fail.

 > Signed-off-by: Peter Korsgaard <peter@korsgaard.com>

Committed, thanks.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH v2 2/7] tpm2-tss: fix build with BR2_FORTIFY_SOURCE_1

[Peter Korsgaard]

>>>>> "Peter" == Peter Korsgaard <peter@korsgaard.com> writes:

 > The configure script passes -U FORTIFY_SOURCE -D FORTIFY_SOURCE=2 by
 > default, which conflicts with BR2_FORTIFY_SOURCE_1 as -Werror is used:

 > <cross>-gcc ..  -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 .. -D_FORTIFY_SOURCE=1
 > <command-line>:0:0: error: "_FORTIFY_SOURCE" redefined [-Werror]

 > Disable this so the FORTIFY_SOURCE flags in TARGET_CFLAGS (if any) is used
 > instead.

 > Signed-off-by: Peter Korsgaard <peter@korsgaard.com>

Committed, thanks.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH v2 6/7] tpm2-tools: do not enforce dependency on tpm2-abrmd

[Yann E. MORIN]

Peter, All,

On 2019-01-16 12:43 +0100, Peter Korsgaard spake thusly:
> >>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:
>  > Peter, All,
>  > On 2019-01-15 11:15 +0100, Peter Korsgaard spake thusly:
>  >> tpm2-tools is commonly used with the resource manager, tpm2-abrmd - But it
>  >> CAN be used without, E.G.  by setting the TPM2TOOLS_TCTI_NAME environment
>  >> variable to communicate directly with the kernel driver:
>  >> 
>  >> export TPM2TOOLS_TCTI_NAME=device
>  >> 
>  >> For some use cases (E.G.  initramfs) it makes sense to use tpm2-tools
>  >> without abrmd, so downgrade the dependency from select to imply, so abrmd is
>  >> enabled by default but can be explicitly disabled.
>  >> 
>  >> Signed-off-by: Peter Korsgaard <peter@korsgaard.com>
>  >> ---
>  >> package/tpm2-tools/Config.in | 2 +-
>  >> 1 file changed, 1 insertion(+), 1 deletion(-)
>  >> 
>  >> diff --git a/package/tpm2-tools/Config.in b/package/tpm2-tools/Config.in
>  >> index cc87e2a1bf..f4622b4ec9 100644
>  >> --- a/package/tpm2-tools/Config.in
>  >> +++ b/package/tpm2-tools/Config.in
>  >> @@ -8,7 +8,7 @@ config BR2_PACKAGE_TPM2_TOOLS
>  >> select BR2_PACKAGE_LIBCURL
>  >> select BR2_PACKAGE_LIBGLIB2
>  >> select BR2_PACKAGE_OPENSSL
>  >> -	select BR2_PACKAGE_TPM2_ABRMD # run-time
>  >> +	imply BR2_PACKAGE_TPM2_ABRMD # run-time
> 
>  > Sorry, but I reiterate my position: I don't like the use of 'imply'.
> 
>  > Either the thing is mandatory, in which case we select it or depend on
>  > it, or the thing is optional, in which case we elt the user enable it.
> 
> I understand you don't like it, but what is the alternative? Just
> mention the optional-but-likely-to-be-needed dependency in the help
> text? That is IMHO worse than imply.

> For this specific case, tpm2-tools fails with a somewhat confusing error
> message if tpm2-abrmd isn't available unless a specific command line
> option / environment variable is used:
> 
> # tpm2_pcrlist
> 
> ** (process:8628): WARNING **: 11:38:39.606: Failed to create connection with service: GDBus.Error:org.freedesktop.DBus.Error.ServiceUnknown: The name com.intel.tss2.Tabrmd was not provided by any .service files
> ERROR: Failed to initialize TABRMD TCTI context: 0xa0008
> 
> The solution is to set the TCTI name to device, either through the
> TPM2TOOLS_TCTI_NAME environment variable or the --tcti option.

Well, this is very clearly explained in the man page for tpm2_pcrlist. ;-)

And it turns out that the use of tpm2-abrmd is in fact totally unneeded,
even to get proper concurrent access, when using the in-kernel manager
exposed as /dev/tmprmN (so says the man page). So, in this case, a
userland daemon is not even needed to begin with.

But since any person not completely insane would still want it, then it
should stay a select. The very minor minority who really, like
really-really, do not want tpm2-abrmd can still remove it with a
post-build script.

> This is imho exactly the kind of use cases imply has been made for.

I still think its semantics are much more fuzzy than select or depends-on,
and it can cause confusion.

Also, I'm still on the side that I prefer the user to know what they are
doing rather than us baby-sitting them.

>  > Use of imply does not sound nice to me, because it is not authoritative.
>  > I'm afraid we get reports of users complaining that "sometimes the stuff
>  > is enabled when I do X, while sometmes it is not enabled when I do the
>  > same X.'
> 
> Is that any different than changing toolchain options or toggling
> BR2_PACKAGE_BUSYBOX_SHOW_OTHERS?

Sorry, I don't follow you. When you toggle those options, they always do
the same thing.

But with imply, if you do something like:

    make distclean
    make menuconfig
        --> enable tpm2-tools

Then tpm2-abrmd is enabled.

But if you now go with:

    make distclean
    make defconfig
    make menuconfig
        --> enable tpm2-tools

Then tpm2-abrmd is not enabled, because it was already disabled in the
.config.

So, this is more confusing than anything else to me, because "sometimes
it works, sometimes it does not".

Whereas, the toolchain options or the busybox-show-others will always do
the same thing.

>  > The coutner argument has been that we were now trying to make sensible
>  > choices for the user, so that things "work out of the box". My position
>  > is that it is an illusion, because making things "just work" is more
>  > often than not more involving than just enabling a package.
> 
> I agree that we probably cannot do this perfectly, but a solution for
> E.G. 80% of the use cases is still an improvement, as long as the
> remaining 20% can still change things.
> 
>  > For example, when dealing with TPM and such: keys and certs provisionning
>  > and checking the chain of trust and such is only scratching the surface.
>  > People that want to deal with this topic better know what they *are* doing,
>  > as it is a sensible topic. Those people will have to understand what they
>  > need if they do not already know.
> 
> Sure, but we can atleast ensure that the tpm2-tools utilites do not fail
> out of the box because of a missing obscure dependency and that fairly
> common use cases are possible.

If you go that route, then you must ensure that the kernel has TPM
support configured in. That is, for the kernel we build; we're leaving
out in the cold those who build their kernel out of Buildroot...

Yes, tangential...

So, as usual, that's my opinion.

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 223 225 172 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH v2 6/7] tpm2-tools: do not enforce dependency on tpm2-abrmd

[Peter Korsgaard]

>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:

Hi,

 >> ** (process:8628): WARNING **: 11:38:39.606: Failed to create
 >> connection with service:
 >> GDBus.Error:org.freedesktop.DBus.Error.ServiceUnknown: The name
 >> com.intel.tss2.Tabrmd was not provided by any .service files
 >> ERROR: Failed to initialize TABRMD TCTI context: 0xa0008
 >> 
 >> The solution is to set the TCTI name to device, either through the
 >> TPM2TOOLS_TCTI_NAME environment variable or the --tcti option.

 > Well, this is very clearly explained in the man page for tpm2_pcrlist. ;-)

Correct, but it is still not obvious.


 > And it turns out that the use of tpm2-abrmd is in fact totally unneeded,
 > even to get proper concurrent access, when using the in-kernel manager
 > exposed as /dev/tmprmN (so says the man page). So, in this case, a
 > userland daemon is not even needed to begin with.

Correct, but the in-kernel manager is relatively new (4.12). There is
various tradeoffs between the in-kernel and user space managers. There
was a presentation about it at Plumbers 2017:

https://blog.linuxplumbersconf.org/2017/ocw//system/presentations/4818/original/TPM2-kernel-evnet-app_tricca-sakkinen.pdf


 > But since any person not completely insane would still want it, then it
 > should stay a select. The very minor minority who really, like
 > really-really, do not want tpm2-abrmd can still remove it with a
 > post-build script.

Sorry, why do you think I am insane for not wanting tpm2-abrmd?

Cleaning up with a post-build script is pretty horrible, especially as
tpm2-abrmd pulls in dbus.


 >> This is imho exactly the kind of use cases imply has been made for.

 > I still think its semantics are much more fuzzy than select or depends-on,
 > and it can cause confusion.

Yes, that is the entire point, E.G. a "weak" select. It behaves the same
as 'default y if foo'.

 >> Is that any different than changing toolchain options or toggling
 >> BR2_PACKAGE_BUSYBOX_SHOW_OTHERS?

 > Sorry, I don't follow you. When you toggle those options, they always do
 > the same thing.

 > But with imply, if you do something like:

 >     make distclean
 >     make menuconfig
 > --> enable tpm2-tools

 > Then tpm2-abrmd is enabled.

 > But if you now go with:

 >     make distclean
 >     make defconfig
 >     make menuconfig
 > --> enable tpm2-tools

 > Then tpm2-abrmd is not enabled, because it was already disabled in the
 > .config.

 > So, this is more confusing than anything else to me, because "sometimes
 > it works, sometimes it does not".

 > Whereas, the toolchain options or the busybox-show-others will always do
 > the same thing.

The point is that they influence other "unrelated" options. Another
example is the places where we have multiple backends where we do
'select foo if !bar'. Here as well behaviour when you then go an enable
bar depends on if you have already exited menuconfig or not (E.G. foo
will stay enabled or not).


 >> Sure, but we can atleast ensure that the tpm2-tools utilites do not fail
 >> out of the box because of a missing obscure dependency and that fairly
 >> common use cases are possible.

 > If you go that route, then you must ensure that the kernel has TPM
 > support configured in. That is, for the kernel we build; we're leaving
 > out in the cold those who build their kernel out of Buildroot...

And that we indeed do when it is possible / the needed kernel options
aren't obvious.

In this case the kernel has several different tpm drivers, so we cannot
really do it.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH v2 6/7] tpm2-tools: do not enforce dependency on tpm2-abrmd

[Arnout Vandecappelle]

On 17/01/2019 16:58, Yann E. MORIN wrote:
> But with imply, if you do something like:
> 
>     make distclean
>     make menuconfig
>         --> enable tpm2-tools
> 
> Then tpm2-abrmd is enabled.
> 
> But if you now go with:
> 
>     make distclean
>     make defconfig
>     make menuconfig
>         --> enable tpm2-tools
> 
> Then tpm2-abrmd is not enabled, because it was already disabled in the
> .config.

 This, for me, is the crux of the matter. I agree with Yann that this is
confusing. Especially because 'make some-defconfig; make menuconfig' is the
usual workflow. So the value of this imply is almost nothing in practice.

 So let me take this occasion to review the cases of imply that we already have
(obviously they're not yet written with the imply keyword).

BR2_ARC_ATOMIC_EXT
BR2_TARGET_ROOTFS_JFFS2_NOCLEANMARKER
BR2_PACKAGE_LUA_32BITS
BR2_PACKAGE_OPUS_FIXED_POINT
BR2_TOOLCHAIN_EXTERNAL_HAS_SSP
BR2_TOOLCHAIN_EXTERNAL_INET_RPC

 These are not confusing IMO because they only become visible after selecting
some other option that is not enabled by default.


BR2_PACKAGE_IFUPDOWN_SCRIPTS

 This one is somewhat less confusing because in the 'make defconfig; make
menuconfig' scenario, the option will already be enabled. However, if you later
on switch to a custom skeleton, the ifupdown-scripts will stay enabled. Still,
that is very similar to the situation for packages that got select'ed: once you
remove the option that caused that package to be enabled, it will stay enabled
even after you disable the option that triggered it.


 In conclusion, we currently already have some confusion caused by 'make
defconfig; make menuconfig' situations, but currently they only go in one
direction: something that was enabled will stay enabled even if you don't need
it any more. Using 'imply' in the way proposed by Peter would introduce a
different kind of confusion: options that don't get enabled though they should be.

 I would say, the design of defaults in Kconfig is simply wrong. To make it work
well, there should be tracking of whether a value was set automatically or by
the user. But let's not go there :-)

 So, that doesn't mean that imply should be banned entirely. It could still be
useful in some cases, like the ifupdown scripts.

 For the situations like the one in this patch, I would say that we could relax
a little the 'avoid extra per-package configuration options'. In fact, extra
configuration options in Config.in don't cost that much. They don't really make
the menus larger because they're only visible when the package is selected. And
the .mk handling is the same as for an automatic optional dependency. That said,
in this specific case of tpm2-tools, I have the feeling that an additional
option is not appropriate. Since the two packages are right next to each other,
that is almost the same as having the suboption. So I would go for the help text
instead.


 Regards,
 Arnout

[Buildroot] [PATCH v2 1/7] tpm2-tss: do not enforce -fstack-protector-all

[Peter Korsgaard]

>>>>> "Peter" == Peter Korsgaard <peter@korsgaard.com> writes:

 > Stack protection is now controlled buildroot wide with the BR2_SSP_*
 > options, so disable the explicit -fstack-protector-all so the SSP logic in
 > the toolchain wrapper is used instead.

 > Signed-off-by: Peter Korsgaard <peter@korsgaard.com>

Committed to 2018.11.x, thanks.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH v2 2/7] tpm2-tss: fix build with BR2_FORTIFY_SOURCE_1

[Peter Korsgaard]

>>>>> "Peter" == Peter Korsgaard <peter@korsgaard.com> writes:

 > The configure script passes -U FORTIFY_SOURCE -D FORTIFY_SOURCE=2 by
 > default, which conflicts with BR2_FORTIFY_SOURCE_1 as -Werror is used:

 > <cross>-gcc ..  -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 .. -D_FORTIFY_SOURCE=1
 > <command-line>:0:0: error: "_FORTIFY_SOURCE" redefined [-Werror]

 > Disable this so the FORTIFY_SOURCE flags in TARGET_CFLAGS (if any) is used
 > instead.

 > Signed-off-by: Peter Korsgaard <peter@korsgaard.com>

Committed, thanks.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH v2 3/7] tpm2-tools: always disable hardening options

[Peter Korsgaard]

>>>>> "Peter" == Peter Korsgaard <peter@korsgaard.com> writes:

 > Building with --enable-hardening (the default), forces -fstack-protector-all
 > / FORTIFY_SOURCE=2.  These options are now controlled Buildroot wide with
 > the BR2_SSP_* / BR2_FORTIFY_SOURCE_* options.  Disable hardening so the
 > ssp/fortify settings in the toolchain wrapper / CFLAGS is used instead.

 > Signed-off-by: Peter Korsgaard <peter@korsgaard.com>

Committed to 2018.11.x, thanks.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH v2 4/7] tpm2-abrmd: do not enforce -fstack-protector-all

[Peter Korsgaard]

>>>>> "Peter" == Peter Korsgaard <peter@korsgaard.com> writes:

 > Stack protection is now controlled Buildroot wide with the BR2_SSP_*
 > options, so disable the explicit -fstack-protector-all so the SSP logic in
 > the toolchain wrapper is used instead.

 > Signed-off-by: Peter Korsgaard <peter@korsgaard.com>

Committed to 2018.11.x after adjusting the variable names for v1.3.0,
thanks.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH v2 5/7] tpm2-abrmd: fix build with BR2_FORTIFY_SOURCE_1

[Peter Korsgaard]

>>>>> "Peter" == Peter Korsgaard <peter@korsgaard.com> writes:

 > The configure script passes -U FORTIFY_SOURCE -D FORTIFY_SOURCE=2 by
 > default, which conflicts with BR2_FORTIFY_SOURCE_1 as -Werror is used:

 > <cross>-gcc ..  -U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=2 .. -D_FORTIFY_SOURCE=1
 > <command-line>:0:0: error: "_FORTIFY_SOURCE" redefined [-Werror]

 > Disable this so the FORTIFY_SOURCE flags in TARGET_CFLAGS (if any) is used
 > instead.

 > Signed-off-by: Peter Korsgaard <peter@korsgaard.com>

Committed to 2018.11.x after adjusting the variable names for v1.3.0,
thanks.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH v2 7/7] tpm2-abrmd: S80tpm2-abrmd: create pid file at startup

[Peter Korsgaard]

>>>>> "Peter" == Peter Korsgaard <peter@korsgaard.com> writes:

 > The start-stop-daemon invocation to start abrmd was missing the -m (make
 > pidfile) option, causing stop to fail.

 > Signed-off-by: Peter Korsgaard <peter@korsgaard.com>

Committed to 2018.11.x, thanks.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH v2 6/7] tpm2-tools: do not enforce dependency on tpm2-abrmd

[Peter Korsgaard]

>>>>> "Arnout" == Arnout Vandecappelle <arnout@mind.be> writes:

Hi,

 >  For the situations like the one in this patch, I would say that we could relax
 > a little the 'avoid extra per-package configuration options'. In fact, extra
 > configuration options in Config.in don't cost that much. They don't really make
 > the menus larger because they're only visible when the package is selected. And
 > the .mk handling is the same as for an automatic optional dependency. That said,
 > in this specific case of tpm2-tools, I have the feeling that an additional
 > option is not appropriate. Since the two packages are right next to each other,
 > that is almost the same as having the suboption. So I would go for the help text
 > instead.

Ok, I'll send an updated patch that drops the select and adds a note in
the help text instead.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH v2 6/7] tpm2-tools: do not enforce dependency on tpm2-abrmd

[Yann E. MORIN]

Peter, All,

On 2019-01-17 20:01 +0100, Peter Korsgaard spake thusly:
> >>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:
[--SNIP--]
>  > But since any person not completely insane would still want it, then it
> Sorry, why do you think I am insane for not wanting tpm2-abrmd?

I do apologise if you took offense, my words were careless, and I should
have known better to not write that.

Regards,
Yann E. MORIN.

-- 
.-----------------.--------------------.------------------.--------------------.
|  Yann E. MORIN  | Real-Time Embedded | /"\ ASCII RIBBON | Erics' conspiracy: |
| +33 662 376 056 | Software  Designer | \ / CAMPAIGN     |  ___               |
| +33 561 099 427 `------------.-------:  X  AGAINST      |  \e/  There is no  |
| http://ymorin.is-a-geek.org/ | _/*\_ | / \ HTML MAIL    |   v   conspiracy.  |
'------------------------------^-------^------------------^--------------------'

[Buildroot] [PATCH v2 6/7] tpm2-tools: do not enforce dependency on tpm2-abrmd

[Peter Korsgaard]

>>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:

 > Peter, All,
 > On 2019-01-17 20:01 +0100, Peter Korsgaard spake thusly:
 >> >>>>> "Yann" == Yann E MORIN <yann.morin.1998@free.fr> writes:
 > [--SNIP--]
 >> > But since any person not completely insane would still want it, then it
 >> Sorry, why do you think I am insane for not wanting tpm2-abrmd?

 > I do apologise if you took offense, my words were careless, and I should
 > have known better to not write that.

No problem!

-- 
Bye, Peter Korsgaard