| Message ID | 20241017065907.172504-1-mikko.rapeli@linaro.org |
|---|---|
| State | New |
| Headers | show |
| Series | [v3,1/2] optee-client: use udev rule and systemd service from upstream | expand |
Hi Mikko,
We have multilib testing that is failing now because the systemd folder is being installed in `libdir` instead of the non-arch-specific `systemd_system_unitdir = "${nonarch_base_libdir}/systemd/system"` as is codified in `bitbake.conf`.
ERROR: optee-client-4.4.0.imx-r0 do_package: QA Issue: optee-client: Files/directories were installed but not shipped in any package:
/usr/lib64/systemd
/usr/lib64/systemd/system
/usr/lib64/systemd/system/tee-supplicant@.service
I'm not sure how this should be fixed, as it doesn't appear that CMAKE can handle more than one libdir. Can the install of the service file be hard-coded to use /usr/lib? Or does this need to be fixed in the recipe?
Tom
-----Original Message-----
From: Mikko Rapeli <mikko.rapeli@linaro.org>
Sent: Thursday, October 17, 2024 1:59 AM
To: meta-arm@lists.yoctoproject.org
Cc: Mikko Rapeli <mikko.rapeli@linaro.org>; Tom Hochstein <tom.hochstein@nxp.com>; Sahil Malhotra <sahil.malhotra@nxp.com>
Subject: [PATCH v3 1/2] optee-client: use udev rule and systemd service from upstream
Use backported upstream patch for udev rule and systemd service file.
sysvinit script is still used from meta-arm. Don't install systemd
service without systemd distro feature, other way round for
sysvinit script.
tee-supplicant started by systemd service runs as non-root teesuppl
user with teepriv group. sysvinit still runs as root since busybox
start-stop-daemon doesn't support -g group parameter and -u teesuppl
doesn't seem to change the effective user.
udev rules allow non-root /dev/tee* access from tee and
/dev/teepriv* access from teepriv groups.
Tested sysvinit changes with:
$ kas build ci/qemuarm64-secureboot.yml:ci/poky.yml:ci/testimage.yml
and systemd changes with:
$ kas build ci/qemuarm64-secureboot.yml:ci/poky.yml:ci/testimage.yml:ci/uefi-secureboot.yml
Cc: tom.hochstein@nxp.com
Cc: sahil.malhotra@nxp.com
Signed-off-by: Mikko Rapeli <mikko.rapeli@linaro.org>
---
.../recipes-security/optee/optee-client.inc | 30 +--
...dd-udev-rule-and-systemd-service-fil.patch | 186 ++++++++++++++++++
.../optee/optee-client/optee-udev.rules | 6 -
.../optee-client/tee-supplicant@.service | 13 --
.../optee/optee-client_4.3.0.bb | 2 +
5 files changed, 205 insertions(+), 32 deletions(-)
create mode 100644 meta-arm/recipes-security/optee/optee-client/0001-tee-supplicant-add-udev-rule-and-systemd-service-fil.patch
delete mode 100644 meta-arm/recipes-security/optee/optee-client/optee-udev.rules
delete mode 100644 meta-arm/recipes-security/optee/optee-client/tee-supplicant@.service
diff --git a/meta-arm/recipes-security/optee/optee-client.inc b/meta-arm/recipes-security/optee/optee-client.inc
index f387c805..fc48c302 100644
--- a/meta-arm/recipes-security/optee/optee-client.inc
+++ b/meta-arm/recipes-security/optee/optee-client.inc
@@ -9,9 +9,7 @@ inherit systemd update-rc.d cmake useradd
SRC_URI = " \
git://github.com/OP-TEE/optee_client.git;branch=master;protocol=https \
- file://tee-supplicant@.service \
file://tee-supplicant.sh \
- file://optee-udev.rules \
"
UPSTREAM_CHECK_GITTAGREGEX = "^(?P<pver>\d+(\.\d+)+)$"
@@ -20,20 +18,21 @@ S = "${WORKDIR}/git"
EXTRA_OECMAKE = " \
-DBUILD_SHARED_LIBS=ON \
- -DCFG_TEE_FS_PARENT_PATH='${localstatedir}/lib/tee' \
"
EXTRA_OECMAKE:append:toolchain-clang = " -DCFG_WERROR=0"
do_install:append() {
- install -D -p -m0644 ${UNPACKDIR}/tee-supplicant@.service ${D}${systemd_system_unitdir}/tee-supplicant@.service
- install -D -p -m0755 ${UNPACKDIR}/tee-supplicant.sh ${D}${sysconfdir}/init.d/tee-supplicant
- install -d ${D}${sysconfdir}/udev/rules.d
- install -m 0644 ${UNPACKDIR}/optee-udev.rules ${D}${sysconfdir}/udev/rules.d/optee.rules
-
- sed -i -e s:@sysconfdir@:${sysconfdir}:g \
- -e s:@sbindir@:${sbindir}:g \
- ${D}${systemd_system_unitdir}/tee-supplicant@.service \
- ${D}${sysconfdir}/init.d/tee-supplicant
+ # installed by default
+ if ! ${@bb.utils.contains('DISTRO_FEATURES', 'systemd', 'true', 'false', d)}; then
+ rm -rf ${D}${libdir}/systemd
+ fi
+ if ${@bb.utils.contains('DISTRO_FEATURES', 'sysvinit', 'true', 'false', d)}; then
+ install -D -p -m0755 ${UNPACKDIR}/tee-supplicant.sh ${D}${sysconfdir}/init.d/tee-supplicant
+ sed -i -e s:@sysconfdir@:${sysconfdir}:g \
+ -e s:@sbindir@:${sbindir}:g \
+ ${D}${sysconfdir}/init.d/tee-supplicant
+ fi
+ install -o teesuppl -g teesuppl -m 0700 -d ${D}${localstatedir}/lib/tee
}
SYSTEMD_SERVICE:${PN} = "tee-supplicant@.service"
@@ -42,5 +41,10 @@ INITSCRIPT_PACKAGES = "${PN}"
INITSCRIPT_NAME:${PN} = "tee-supplicant"
INITSCRIPT_PARAMS:${PN} = "start 10 1 2 3 4 5 . stop 90 0 6 ."
+# Users and groups:
+# tee group to access /dev/tee*
+# teepriv group to acess /dev/teepriv*, only tee-supplicant
+# teesuppl user and group teesuppl to run tee-supplicant
USERADD_PACKAGES = "${PN}"
-GROUPADD_PARAM:${PN} = "--system teeclnt"
+GROUPADD_PARAM:${PN} = "--system tee; --system teepriv; --system teesuppl"
+USERADD_PARAM:${PN} = "--system -g teesuppl --groups teepriv --home-dir ${localstatedir}/lib/tee -M --shell /sbin/nologin teesuppl;"
diff --git a/meta-arm/recipes-security/optee/optee-client/0001-tee-supplicant-add-udev-rule-and-systemd-service-fil.patch b/meta-arm/recipes-security/optee/optee-client/0001-tee-supplicant-add-udev-rule-and-systemd-service-fil.patch
new file mode 100644
index 00000000..18c0d950
--- /dev/null
+++ b/meta-arm/recipes-security/optee/optee-client/0001-tee-supplicant-add-udev-rule-and-systemd-service-fil.patch
@@ -0,0 +1,186 @@
+From bf0d02758696ee7a9f7af9e95f85f5c238d0e109 Mon Sep 17 00:00:00 2001
+From: Mikko Rapeli <mikko.rapeli@linaro.org>
+Date: Wed, 2 Oct 2024 15:24:21 +0100
+Subject: [PATCH] tee-supplicant: add udev rule and systemd service file
+
+tee-supplicant startup with systemd init based
+is non-trivial. Add sample udev rule and systemd
+service files here so that distros can co-operate maintaining
+them.
+
+Files are from meta-arm https://git.yoctoproject.org/meta-arm
+at commit 7cce43e632daa8650f683ac726f9124681b302a4 with license
+MIT and authors:
+
+Peter Griffin <peter.griffin@linaro.org>
+Joshua Watt <JPEWhacker@gmail.com>
+Javier Tia <javier.tia@linaro.org>
+Mikko Rapeli <mikko.rapeli@linaro.org>
+
+With permission from the authors, files can be relicensed to
+BSD-2-Clause like rest of optee client repo.
+
+The config files expect to find tee and teepriv system groups
+and teesuppl user and group (part of teepriv group) for running
+tee-supplicant. Additionally state directory /var/lib/tee
+must be owned by teesuppl user and group with no rights
+to other users. The groups and user can be changed via
+CMake variables:
+
+CFG_TEE_GROUP
+CFG_TEEPRIV_GROUP
+CFG_TEE_SUPPL_USER
+CFG_TEE_SUPPL_GROUP
+
+Change storage path from /data to /var/lib and
+use standard CMake variables also for constructing install
+paths which can be override to change the defaults:
+
+CMAKE_INSTALL_PREFIX, e.g. /
+CMAKE_INSTALL_LIBDIR, e.g. /usr/lib
+CMAKE_INSTALL_LOCALSTATEDIR /var
+
+Once these are setup, udev will start tee-supplicant in initramfs
+or rootfs with teesuppl user and group when /dev/teepriv
+device appears. The systemd service starts before tpm2.target
+(new in systemd 256) which starts early in initramfs and in main rootfs.
+This covers firmware TPM TA usecases for main rootfs encryption. When
+stopping tee-supplicant, the ftpm kernel modules are removed and only
+then the main process stopped to avoid fTPM breakage. These workarounds
+may be removed once RPMB kernel and optee patches without tee-supplicant
+are merged (Linux kernel >= 6.12-rc1, optee_os latest master or >= 4.4).
+
+Tested on yocto meta-arm setup which runs fTPM and optee-test/xtest
+under qemuarm64:
+
+$ git clone https://git.yoctoproject.org/meta-arm
+$ cd meta-arm
+$ SSTATE_DIR=$HOME/sstate DL_DIR=$HOME/download kas build \
+ci/qemuarm64-secureboot.yml:ci/poky-altcfg.yml:ci/testimage.yml
+
+Compiled image can be manually started to qemu serial console with:
+
+$ SSTATE_DIR=$HOME/sstate DL_DIR=$HOME/download kas shell \
+ci/qemuarm64-secureboot.yml:ci/poky-altcfg.yml:ci/testimage.yml
+$ runqemu slirp nographic
+
+meta-arm maintainers run these tests as part of their CI.
+
+Note that if the tee-supplicant state directory /var/lib/tee
+can not be accessed due permissions or other problems, then
+tee-supplicant startup with systemd still works. Only optee-test/xtest
+will be failing and fTPM kernel drivers fail to load with error
+messages.
+
+Cc: Peter Griffin <peter.griffin@linaro.org>
+Cc: Joshua Watt <JPEWhacker@gmail.com>
+Cc: Javier Tia <javier.tia@linaro.org>
+Acked-by: Jerome Forissier <jerome.forissier@linaro.org>
+Signed-off-by: Mikko Rapeli <mikko.rapeli@linaro.org>
+---
+ config.mk | 2 +-
+ libteec/CMakeLists.txt | 2 +-
+ tee-supplicant/CMakeLists.txt | 13 +++++++++++--
+ tee-supplicant/optee-udev.rules.in | 7 +++++++
+ tee-supplicant/tee-supplicant@.service.in | 17 +++++++++++++++++
+ 5 files changed, 37 insertions(+), 4 deletions(-)
+ create mode 100644 tee-supplicant/optee-udev.rules.in
+ create mode 100644 tee-supplicant/tee-supplicant@.service.in
+
+Upstream-Status: Backport
+
+diff --git a/config.mk b/config.mk
+index eae481f..3def087 100644
+--- a/config.mk
++++ b/config.mk
+@@ -23,7 +23,7 @@ CFG_TEE_SUPP_LOG_LEVEL?=1
+ # This folder can be created with the required permission in an init
+ # script during boot, else it will be created by the tee-supplicant on
+ # first REE FS access.
+-CFG_TEE_FS_PARENT_PATH ?= /data/tee
++CFG_TEE_FS_PARENT_PATH ?= /var/lib/tee
+
+ # CFG_TEE_CLIENT_LOG_FILE
+ # The location of the client log file when logging to file is enabled.
+diff --git a/libteec/CMakeLists.txt b/libteec/CMakeLists.txt
+index c742d31..c857369 100644
+--- a/libteec/CMakeLists.txt
++++ b/libteec/CMakeLists.txt
+@@ -14,7 +14,7 @@ endif()
+ # Configuration flags always included
+ ################################################################################
+ set(CFG_TEE_CLIENT_LOG_LEVEL "1" CACHE STRING "libteec log level")
+-set(CFG_TEE_CLIENT_LOG_FILE "/data/tee/teec.log" CACHE STRING "Location of libteec log")
++set(CFG_TEE_CLIENT_LOG_FILE "${CMAKE_INSTALL_LOCALSTATEDIR}/lib/tee/teec.log" CACHE STRING "Location of libteec log")
+
+ ################################################################################
+ # Source files
+diff --git a/tee-supplicant/CMakeLists.txt b/tee-supplicant/CMakeLists.txt
+index 54a34c7..8df9bef 100644
+--- a/tee-supplicant/CMakeLists.txt
++++ b/tee-supplicant/CMakeLists.txt
+@@ -11,10 +11,15 @@ option(CFG_TEE_SUPP_PLUGINS "Enable tee-supplicant plugin support" ON)
+ set(CFG_TEE_SUPP_LOG_LEVEL "1" CACHE STRING "tee-supplicant log level")
+ # FIXME: Question is, is this really needed? Should just use defaults from # GNUInstallDirs?
+ set(CFG_TEE_CLIENT_LOAD_PATH "/lib" CACHE STRING "Colon-separated list of paths where to look for TAs (see also --ta-dir)")
+-set(CFG_TEE_FS_PARENT_PATH "/data/tee" CACHE STRING "Location of TEE filesystem (secure storage)")
++set(CFG_TEE_FS_PARENT_PATH "${CMAKE_INSTALL_LOCALSTATEDIR}/lib/tee" CACHE STRING "Location of TEE filesystem (secure storage)")
+ # FIXME: Why do we have if defined(CFG_GP_SOCKETS) && CFG_GP_SOCKETS == 1 in the c-file?
+ set(CFG_GP_SOCKETS "1" CACHE STRING "Enable GlobalPlatform Socket API support")
+-set(CFG_TEE_PLUGIN_LOAD_PATH "/usr/lib/tee-supplicant/plugins/" CACHE STRING "tee-supplicant's plugins path")
++set(CFG_TEE_PLUGIN_LOAD_PATH "${CMAKE_INSTALL_PREFIX}/${CMAKE_INSTALL_LIBDIR}/${PROJECT_NAME}/plugins/" CACHE STRING "tee-supplicant's plugins path")
++
++set(CFG_TEE_GROUP "tee" CACHE STRING "Group which has access to /dev/tee* devices")
++set(CFG_TEEPRIV_GROUP "teepriv" CACHE STRING "Group which has access to /dev/teepriv* devices")
++set(CFG_TEE_SUPPL_USER "teesuppl" CACHE STRING "User account which tee-supplicant is started with")
++set(CFG_TEE_SUPPL_GROUP "teesuppl" CACHE STRING "Group account which tee-supplicant is started with")
+
+ if(CFG_TEE_SUPP_PLUGINS)
+ set(CMAKE_INSTALL_RPATH "${CFG_TEE_PLUGIN_LOAD_PATH}")
+@@ -113,3 +118,7 @@ endif()
+ # Install targets
+ ################################################################################
+ install(TARGETS ${PROJECT_NAME} RUNTIME DESTINATION ${CMAKE_INSTALL_SBINDIR})
++configure_file(tee-supplicant@.service.in tee-supplicant@.service @ONLY)
++install(FILES ${CMAKE_BINARY_DIR}/${PROJECT_NAME}/tee-supplicant@.service DESTINATION ${CMAKE_INSTALL_LIBDIR}/systemd/system)
++configure_file(optee-udev.rules.in optee-udev.rules @ONLY)
++install(FILES ${CMAKE_BINARY_DIR}/${PROJECT_NAME}/optee-udev.rules DESTINATION ${CMAKE_INSTALL_SYSCONFDIR}/udev/rules.d)
+diff --git a/tee-supplicant/optee-udev.rules.in b/tee-supplicant/optee-udev.rules.in
+new file mode 100644
+index 0000000..275e833
+--- /dev/null
++++ b/tee-supplicant/optee-udev.rules.in
+@@ -0,0 +1,7 @@
++# SPDX-License-Identifier: BSD-2-Clause
++KERNEL=="tee[0-9]*", MODE="0660", OWNER="root", GROUP="@CFG_TEE_GROUP@", TAG+="systemd"
++
++# If a /dev/teepriv[0-9]* device is detected, start an instance of
++# tee-supplicant.service with the device name as parameter
++KERNEL=="teepriv[0-9]*", MODE="0660", OWNER="root", GROUP="@CFG_TEEPRIV_GROUP@", \
++ TAG+="systemd", ENV{SYSTEMD_WANTS}+="tee-supplicant@%k.service"
+diff --git a/tee-supplicant/tee-supplicant@.service.in b/tee-supplicant/tee-supplicant@.service.in
+new file mode 100644
+index 0000000..e53a935
+--- /dev/null
++++ b/tee-supplicant/tee-supplicant@.service.in
+@@ -0,0 +1,17 @@
++# SPDX-License-Identifier: BSD-2-Clause
++[Unit]
++Description=TEE Supplicant on %i
++DefaultDependencies=no
++After=dev-%i.device
++Wants=dev-%i.device
++Conflicts=shutdown.target
++Before=tpm2.target sysinit.target shutdown.target
++
++[Service]
++Type=notify
++User=@CFG_TEE_SUPPL_USER@
++Group=@CFG_TEE_SUPPL_GROUP@
++EnvironmentFile=-@CMAKE_INSTALL_SYSCONFDIR@/default/tee-supplicant
++ExecStart=@CMAKE_INSTALL_PREFIX@/@CMAKE_INSTALL_SBINDIR@/tee-supplicant $OPTARGS
++# Workaround for fTPM TA: stop kernel module before tee-supplicant
++ExecStop=-/bin/sh -c "/sbin/modprobe -v -r tpm_ftpm_tee ; /bin/kill $MAINPID"
+--
+2.34.1
+
diff --git a/meta-arm/recipes-security/optee/optee-client/optee-udev.rules b/meta-arm/recipes-security/optee/optee-client/optee-udev.rules
deleted file mode 100644
index 075f469c..00000000
--- a/meta-arm/recipes-security/optee/optee-client/optee-udev.rules
+++ /dev/null
@@ -1,6 +0,0 @@
-KERNEL=="tee[0-9]*", MODE="0660", OWNER="root", GROUP="teeclnt", TAG+="systemd"
-
-# If a /dev/teepriv[0-9]* device is detected, start an instance of
-# tee-supplicant.service with the device name as parameter
-KERNEL=="teepriv[0-9]*", MODE="0660", OWNER="root", GROUP="teeclnt", \
- TAG+="systemd", ENV{SYSTEMD_WANTS}+="tee-supplicant@%k.service"
diff --git a/meta-arm/recipes-security/optee/optee-client/tee-supplicant@.service b/meta-arm/recipes-security/optee/optee-client/tee-supplicant@.service
deleted file mode 100644
index e3039fde..00000000
--- a/meta-arm/recipes-security/optee/optee-client/tee-supplicant@.service
+++ /dev/null
@@ -1,13 +0,0 @@
-[Unit]
-Description=TEE Supplicant on %i
-DefaultDependencies=no
-After=dev-%i.device
-Wants=dev-%i.device
-Conflicts=shutdown.target
-Before=tpm2.target sysinit.target shutdown.target
-
-[Service]
-Type=notify
-EnvironmentFile=-@sysconfdir@/default/tee-supplicant
-ExecStart=@sbindir@/tee-supplicant $OPTARGS
-ExecStop=-/bin/sh -c "/sbin/modprobe -v -r tpm_ftpm_tee ; /bin/kill $MAINPID"
diff --git a/meta-arm/recipes-security/optee/optee-client_4.3.0.bb b/meta-arm/recipes-security/optee/optee-client_4.3.0.bb
index 4a088004..edab4583 100644
--- a/meta-arm/recipes-security/optee/optee-client_4.3.0.bb
+++ b/meta-arm/recipes-security/optee/optee-client_4.3.0.bb
@@ -2,6 +2,8 @@ require recipes-security/optee/optee-client.inc
SRCREV = "a5b1ffcd26e328af0bbf18ab448a38ecd558e05c"
+SRC_URI += "file://0001-tee-supplicant-add-udev-rule-and-systemd-service-fil.patch"
+
inherit pkgconfig
DEPENDS += "util-linux"
EXTRA_OEMAKE += "PKG_CONFIG=pkg-config"
--
2.34.1
Hi, On Wed, Oct 23, 2024 at 02:22:30PM +0000, Tom Hochstein (OSS) wrote: > Hi Mikko, > > We have multilib testing that is failing now because the systemd folder is being installed in `libdir` instead of the non-arch-specific `systemd_system_unitdir = "${nonarch_base_libdir}/systemd/system"` as is codified in `bitbake.conf`. > > ERROR: optee-client-4.4.0.imx-r0 do_package: QA Issue: optee-client: Files/directories were installed but not shipped in any package: > /usr/lib64/systemd > /usr/lib64/systemd/system > /usr/lib64/systemd/system/tee-supplicant@.service > > I'm not sure how this should be fixed, as it doesn't appear that CMAKE can handle more than one libdir. Can the install of the service file be hard-coded to use /usr/lib? Or does this need to be fixed in the recipe? As mentioned in https://github.com/OP-TEE/optee_client/issues/393 your use case is "sysvinit" without "usrmerge" in distro features. "sysvinit" conflicts with "systemd" init. These recipe changes only install the systemd service file if "systemd" is enabled in DISTRO_FEATURES. Your usecase should be covered by that. Cheers, -Mikko
Hi,
On Wed, Oct 23, 2024 at 02:34:15PM +0000, Tom Hochstein wrote:
> Sorry I wasn't clear. The new build break is for when we do use systemd. We use sysvinit for QorIQ, but systemd for i.MX.
Ok got it now.
${nonarch_base_libdir} isn't in the CMake toolchain file. We could
move ${libdir}/systemd unconditionlly to ${nonarch_base_libdir}/systemd
in the do_install task. Could you test and submit a patch like below?
--- a/meta-arm/recipes-security/optee/optee-client.inc
+++ b/meta-arm/recipes-security/optee/optee-client.inc
@@ -23,8 +23,9 @@ EXTRA_OECMAKE:append:toolchain-clang = " -DCFG_WERROR=0"
do_install:append() {
# installed by default
+ mv ${D}${libdir}/systemd ${D}${noarch_base_libdir}/systemd
if ! ${@bb.utils.contains('DISTRO_FEATURES', 'systemd', 'true', 'false', d)}; then
- rm -rf ${D}${libdir}/systemd
+ rm -rf ${D}${noarch_base_libdir}/systemd
fi
if ${@bb.utils.contains('DISTRO_FEATURES', 'sysvinit', 'true', 'false', d)}; then
install -D -p -m0755 ${UNPACKDIR}/tee-supplicant.sh ${D}${sysconfdir}/init.d/tee-supplicant
Cheers,
-Mikko
On 10/23/2024 9:54 AM, Mikko Rapeli wrote: > Ok got it now. > > ${nonarch_base_libdir} isn't in the CMake toolchain file. We could > move ${libdir}/systemd unconditionlly to ${nonarch_base_libdir}/systemd > in the do_install task. Could you test and submit a patch like below? > > --- a/meta-arm/recipes-security/optee/optee-client.inc > +++ b/meta-arm/recipes-security/optee/optee-client.inc > @@ -23,8 +23,9 @@ EXTRA_OECMAKE:append:toolchain-clang = " -DCFG_WERROR=0" > > do_install:append() { > # installed by default > + mv ${D}${libdir}/systemd ${D}${noarch_base_libdir}/systemd > if ! ${@bb.utils.contains('DISTRO_FEATURES', 'systemd', 'true', 'false', d)}; then > - rm -rf ${D}${libdir}/systemd > + rm -rf ${D}${noarch_base_libdir}/systemd > fi > if ${@bb.utils.contains('DISTRO_FEATURES', 'sysvinit', 'true', 'false', d)}; then > install -D -p -m0755 ${UNPACKDIR}/tee-supplicant.sh ${D}${sysconfdir}/init.d/tee-supplicant > > Cheers, > > -Mikko This works fine in our testing. Sorry for the late response. Thanks again! Tom
Hi,
(Sorry for the delayed reply.)
Another way to address this would be to use PkgConfig from CMakeLists.txt (see [1]). This might be considered a nicer solution as the yocto recipe would not have to correct the install location, and thus there would be a single source of truth. (Kudos for Ross for the suggestion.)
Something like (code is not tested):
Include(FindPkgConfig)
If (PKG_CONFIG_FOUND)
# TODO: there are other variables. Doublecheck if systemdsystemconfdir is the right one to be used. On X86 Ubuntu this returns /etc/systemd/system. Yocto might define distro specific values.
pkg_get_variable(UNIT_DIR systemd systemdsystemconfdir)
# TODO: check the value to see if pkgconfig successfully found the value. Behavior is not documented, probably if (UNIT_DIR) would work.
# TODO: UNIT_DIR is a string list, use only the first or last value? Rewrite the line below accordingly
set(CMAKE_INSTALL_SYSTEMDSYSCONFDIR “${ UNIT_DIR }” CACHE PATH “Target directory for system config files”)
endif()
# By default, use LIBDIR
# TODO: CMAKE_INSTALL_SYSCONFDIR might be a better default base dir.
set(CMAKE_INSTALL_ SYSTEMDSYSCONFDIR “${ CMAKE_INSTALL_LIBDIR}}/systemd/system” CACHE PATH “Target directory for system config files”)
install(FILES ${CMAKE_BINARY_DIR}/${PROJECT_NAME}/tee-supplicant@.service DESTINATION ${ CMAKE_INSTALL_SYSTEMDSYSCONFDIR})
The above code would default to the current install location under LIBDIR, and would use PkgConfig if available. Setting CMAKE_INSTALL_ SYSTEMDSYSCONFDIR from the command line allows manual override.
1: https://cmake.org/cmake/help/latest/module/FindPkgConfig.html
/George
On 2024-10-29, 18:03, "meta-arm@lists.yoctoproject.org" <meta-arm@lists.yoctoproject.org> wrote:
On 10/23/2024 9:54 AM, Mikko Rapeli wrote:
> Ok got it now.
>
> ${nonarch_base_libdir} isn't in the CMake toolchain file. We could
> move ${libdir}/systemd unconditionlly to ${nonarch_base_libdir}/systemd
> in the do_install task. Could you test and submit a patch like below?
>
> --- a/meta-arm/recipes-security/optee/optee-client.inc
> +++ b/meta-arm/recipes-security/optee/optee-client.inc
> @@ -23,8 +23,9 @@ EXTRA_OECMAKE:append:toolchain-clang = " -DCFG_WERROR=0"
>
> do_install:append() {
> # installed by default
> + mv ${D}${libdir}/systemd ${D}${noarch_base_libdir}/systemd
> if ! ${@bb.utils.contains('DISTRO_FEATURES', 'systemd', 'true', 'false', d)}; then
> - rm -rf ${D}${libdir}/systemd
> + rm -rf ${D}${noarch_base_libdir}/systemd
> fi
> if ${@bb.utils.contains('DISTRO_FEATURES', 'sysvinit', 'true', 'false', d)}; then
> install -D -p -m0755 ${UNPACKDIR}/tee-supplicant.sh ${D}${sysconfdir}/init.d/tee-supplicant
>
> Cheers,
>
> -Mikko
This works fine in our testing. Sorry for the late response.
Thanks again!
Tom
Hi, On Wed, Oct 30, 2024 at 08:02:19AM +0000, Gyorgy Szing wrote: > Hi, > > (Sorry for the delayed reply.) > > Another way to address this would be to use PkgConfig from CMakeLists.txt (see [1]). This might be considered a nicer solution as the yocto recipe would not have to correct the install location, and thus there would be a single source of truth. (Kudos for Ross for the suggestion.) > Something like (code is not tested): > > Include(FindPkgConfig) > If (PKG_CONFIG_FOUND) > # TODO: there are other variables. Doublecheck if systemdsystemconfdir is the right one to be used. On X86 Ubuntu this returns /etc/systemd/system. Yocto might define distro specific values. > pkg_get_variable(UNIT_DIR systemd systemdsystemconfdir) > # TODO: check the value to see if pkgconfig successfully found the value. Behavior is not documented, probably if (UNIT_DIR) would work. > # TODO: UNIT_DIR is a string list, use only the first or last value? Rewrite the line below accordingly > set(CMAKE_INSTALL_SYSTEMDSYSCONFDIR “${ UNIT_DIR }” CACHE PATH “Target directory for system config files”) > endif() > # By default, use LIBDIR > # TODO: CMAKE_INSTALL_SYSCONFDIR might be a better default base dir. > set(CMAKE_INSTALL_ SYSTEMDSYSCONFDIR “${ CMAKE_INSTALL_LIBDIR}}/systemd/system” CACHE PATH “Target directory for system config files”) > > install(FILES ${CMAKE_BINARY_DIR}/${PROJECT_NAME}/tee-supplicant@.service DESTINATION ${ CMAKE_INSTALL_SYSTEMDSYSCONFDIR}) > > > The above code would default to the current install location under LIBDIR, and would use PkgConfig if available. Setting CMAKE_INSTALL_ SYSTEMDSYSCONFDIR from the command line allows manual override. > > 1: https://cmake.org/cmake/help/latest/module/FindPkgConfig.html This would only work if optee-client has "systemd" build time dependency in DEPENDS. I don't think we should add this dependency. The sd_notify() support for example dlopen()'s systemd libs at runtime to avoid making systemd a hard build time dependency. I don't think we should add this dependency due to systemd service file install paths. Cheers, -Mikko > /George > > On 2024-10-29, 18:03, "meta-arm@lists.yoctoproject.org" <meta-arm@lists.yoctoproject.org> wrote: > > On 10/23/2024 9:54 AM, Mikko Rapeli wrote: > > Ok got it now. > > > > ${nonarch_base_libdir} isn't in the CMake toolchain file. We could > > move ${libdir}/systemd unconditionlly to ${nonarch_base_libdir}/systemd > > in the do_install task. Could you test and submit a patch like below? > > > > --- a/meta-arm/recipes-security/optee/optee-client.inc > > +++ b/meta-arm/recipes-security/optee/optee-client.inc > > @@ -23,8 +23,9 @@ EXTRA_OECMAKE:append:toolchain-clang = " -DCFG_WERROR=0" > > > > do_install:append() { > > # installed by default > > + mv ${D}${libdir}/systemd ${D}${noarch_base_libdir}/systemd > > if ! ${@bb.utils.contains('DISTRO_FEATURES', 'systemd', 'true', 'false', d)}; then > > - rm -rf ${D}${libdir}/systemd > > + rm -rf ${D}${noarch_base_libdir}/systemd > > fi > > if ${@bb.utils.contains('DISTRO_FEATURES', 'sysvinit', 'true', 'false', d)}; then > > install -D -p -m0755 ${UNPACKDIR}/tee-supplicant.sh ${D}${sysconfdir}/init.d/tee-supplicant > > > > Cheers, > > > > -Mikko > > This works fine in our testing. Sorry for the late response. > > Thanks again! > > Tom
> Hi, > > On Wed, Oct 30, 2024 at 08:02:19AM +0000, Gyorgy Szing wrote: > > Hi, > > > > (Sorry for the delayed reply.) > > > > Another way to address this would be to use PkgConfig from CMakeLists.txt (see [1]). This might be considered a nicer solution as the yocto recipe would not have to correct the install location, and thus there would be a single source of truth. (Kudos for Ross for the suggestion.) > > Something like (code is not tested): > > > > Include(FindPkgConfig) > > If (PKG_CONFIG_FOUND) > > # TODO: there are other variables. Doublecheck if systemdsystemconfdir is the right one to be used. On X86 Ubuntu this returns /etc/systemd/system. Yocto might define distro specific values. > > pkg_get_variable(UNIT_DIR systemd systemdsystemconfdir) > > # TODO: check the value to see if pkgconfig successfully found the value. Behavior is not documented, probably if (UNIT_DIR) would work. > > # TODO: UNIT_DIR is a string list, use only the first or last value? Rewrite the line below accordingly > > set(CMAKE_INSTALL_SYSTEMDSYSCONFDIR “${ UNIT_DIR }” CACHE PATH “Target directory for system config files”) > > endif() > > # By default, use LIBDIR > > # TODO: CMAKE_INSTALL_SYSCONFDIR might be a better default base dir. > > set(CMAKE_INSTALL_ SYSTEMDSYSCONFDIR “${ CMAKE_INSTALL_LIBDIR}}/systemd/system” CACHE PATH “Target directory for system config files”) > > > > install(FILES ${CMAKE_BINARY_DIR}/${PROJECT_NAME}/tee-supplicant@.service DESTINATION ${ CMAKE_INSTALL_SYSTEMDSYSCONFDIR}) > > > > > > The above code would default to the current install location under LIBDIR, and would use PkgConfig if available. Setting CMAKE_INSTALL_ SYSTEMDSYSCONFDIR from the command line allows manual override. > > > > 1: https://cmake.org/cmake/help/latest/module/FindPkgConfig.html > > This would only work if optee-client has "systemd" build time dependency in DEPENDS. > I don't think we should add this dependency. The sd_notify() support for example > dlopen()'s systemd libs at runtime to avoid making systemd a hard build time > dependency. I don't think we should add this dependency due to systemd > service file install paths. DISTRO_FEATURES seem to reflect if systemd is deployed and this allows the recipe to behave accordingly. Not sure if this is the case for all distros, but poky and meta-arm already depend on DISTRO_FEATURES based systemd configuration and thus might be an acceptable risk. AFAIK systemd was added part of uefi-secure boot enablement. If this feature is enabled the native sysroot of the optee-client recipe has systemd already because of some uefi-secureboot specific transitive dependency. Might not be bulletproof but if systemd is needed, things seem to be ok as is. On the other hand, adding systemd conditionally to DEPENDS might not make things much worse since it is included already. Please check the attached patch file. I cherry-picked you commit “optee-client: use udev rule and systemd service from upstream“ to my topic branch and added the attached patch on top (among other patches which are hopefully have no effect on optee-client). I did some manual testing by hacking DISTRO_FEATURES and running bitbake-getvar, plus testing fvp_base and quemuarm64-secureboot configurations. It’s not 100% tested as e.g. I did not check how the cmake modification behaves if pkg-config is enabled, but the executable is not available or is available but either system or udev package is missing. The cmake code is there to handle these cases though. Anyways these scenarios seem to be out of scope for yocto. I added a dependency on udev to the optee-client recipe, to make udevs pkg config data available. This might be considered similar “unwanted bloat” you mentioned related to systemd. I tried to make this conditional but could not find a safe way. According to [1] there is a variable called USE_DEVFS which tells if a device manager is available, but the variable is not visible to the recipe. AFAIK the image class would need to be inherited to get access to it, but that does not seem to be a good idea. The op-tee client patch: - adds build options to control if the system service file shall be installed or not. The default value is on, to keep backwards compatibility. - another option is added to enable using pkg-config. This is by default off, and the op-tee client recipe enables it for yocto. If enabled both system and/or udev specific directories are discovered with pkg-config. - the intention was to implement option and pkg-config processing in a flexible way. There are hard-coded default values which keep existing behavior and will be used if pkg-config is disabled or failing. Note: I missed the systemd dependency issue and conditional appending to DEPENDS is not implemented. 1: https://docs.yoctoproject.org/dev/dev-manual/device-manager.html /George > Cheers, > > -Mikko > > > /George > > > > On 2024-10-29, 18:03, "meta-arm@lists.yoctoproject.org" <meta-arm@lists.yoctoproject.org> wrote: > > > > On 10/23/2024 9:54 AM, Mikko Rapeli wrote: > > > Ok got it now. > > > > > > ${nonarch_base_libdir} isn't in the CMake toolchain file. We could > > > move ${libdir}/systemd unconditionlly to ${nonarch_base_libdir}/systemd > > > in the do_install task. Could you test and submit a patch like below? > > > > > > --- a/meta-arm/recipes-security/optee/optee-client.inc > > > +++ b/meta-arm/recipes-security/optee/optee-client.inc > > > @@ -23,8 +23,9 @@ EXTRA_OECMAKE:append:toolchain-clang = " -DCFG_WERROR=0" > > > > > > do_install:append() { > > > # installed by default > > > + mv ${D}${libdir}/systemd ${D}${noarch_base_libdir}/systemd > > > if ! ${@bb.utils.contains('DISTRO_FEATURES', 'systemd', 'true', 'false', d)}; then > > > - rm -rf ${D}${libdir}/systemd > > > + rm -rf ${D}${noarch_base_libdir}/systemd > > > fi > > > if ${@bb.utils.contains('DISTRO_FEATURES', 'sysvinit', 'true', 'false', d)}; then > > > install -D -p -m0755 ${UNPACKDIR}/tee-supplicant.sh ${D}${sysconfdir}/init.d/tee-supplicant > > > > > > Cheers, > > > > > > -Mikko > > > > This works fine in our testing. Sorry for the late response. > > > > Thanks again! > > > > Tom >
Hi, On Tue, Nov 26, 2024 at 09:05:15PM +0000, Gyorgy Szing wrote: > > Hi, > > > > On Wed, Oct 30, 2024 at 08:02:19AM +0000, Gyorgy Szing wrote: > > > Hi, > > > > > > (Sorry for the delayed reply.) > > > > > > Another way to address this would be to use PkgConfig from CMakeLists.txt (see [1]). This might be considered a nicer solution as the yocto recipe would not have to correct the install location, and thus there would be a single source of truth. (Kudos for Ross for the suggestion.) > > > Something like (code is not tested): > > > > > > Include(FindPkgConfig) > > > If (PKG_CONFIG_FOUND) > > > # TODO: there are other variables. Doublecheck if systemdsystemconfdir is the right one to be used. On X86 Ubuntu this returns /etc/systemd/system. Yocto might define distro specific values. > > > pkg_get_variable(UNIT_DIR systemd systemdsystemconfdir) > > > # TODO: check the value to see if pkgconfig successfully found the value. Behavior is not documented, probably if (UNIT_DIR) would work. > > > # TODO: UNIT_DIR is a string list, use only the first or last value? Rewrite the line below accordingly > > > set(CMAKE_INSTALL_SYSTEMDSYSCONFDIR “${ UNIT_DIR }” CACHE PATH “Target directory for system config files”) > > > endif() > > > # By default, use LIBDIR > > > # TODO: CMAKE_INSTALL_SYSCONFDIR might be a better default base dir. > > > set(CMAKE_INSTALL_ SYSTEMDSYSCONFDIR “${ CMAKE_INSTALL_LIBDIR}}/systemd/system” CACHE PATH “Target directory for system config files”) > > > > > > install(FILES ${CMAKE_BINARY_DIR}/${PROJECT_NAME}/tee-supplicant@.service DESTINATION ${ CMAKE_INSTALL_SYSTEMDSYSCONFDIR}) > > > > > > > > > The above code would default to the current install location under LIBDIR, and would use PkgConfig if available. Setting CMAKE_INSTALL_ SYSTEMDSYSCONFDIR from the command line allows manual override. > > > > > > 1: https://cmake.org/cmake/help/latest/module/FindPkgConfig.html > > > > This would only work if optee-client has "systemd" build time dependency in DEPENDS. > > I don't think we should add this dependency. The sd_notify() support for example > > dlopen()'s systemd libs at runtime to avoid making systemd a hard build time > > dependency. I don't think we should add this dependency due to systemd > > service file install paths. > > DISTRO_FEATURES seem to reflect if systemd is deployed and this allows the recipe to behave accordingly. Not sure if this is the case for all distros, but poky and meta-arm already depend on DISTRO_FEATURES based systemd configuration and thus might be an acceptable risk. > AFAIK systemd was added part of uefi-secure boot enablement. If this feature is enabled the native sysroot of the optee-client recipe has systemd already because of some uefi-secureboot specific transitive dependency. Might not be bulletproof but if systemd is needed, things seem to be ok as is. On the other hand, adding systemd conditionally to DEPENDS might not make things much worse since it is included already. Even when "systemd" is the selected init and in DISTRO_FEATURES, it is not a build dependency and in DEPENDS of optee-client and thus systemd pkg-config files are not available. Yocto recipes and open source SW which ships systemd service files and even notify systemd at startup using sd_notify() are not meant to have systemd in their build dependencies. This makes too many things depend on systemd which results in longer build times. Thus I don't want to add systemd to DEPENDS of optee-client. > Please check the attached patch file. I cherry-picked you commit “optee-client: use udev rule and systemd service from upstream“ to my topic branch and added the attached patch on top (among other patches which are hopefully have no effect on optee-client). > I did some manual testing by hacking DISTRO_FEATURES and running bitbake-getvar, plus testing fvp_base and quemuarm64-secureboot configurations. It’s not 100% tested as e.g. I did not check how the cmake modification behaves if pkg-config is enabled, but the executable is not available or is available but either system or udev package is missing. The cmake code is there to handle these cases though. Anyways these scenarios seem to be out of scope for yocto. I think this patch is complex enough that it needs upstreaming to optee-client. I don't agree with using pkg-config settings from systemd to figure out systemd service file install paths because that requires adding systemd to build dependencies. > I added a dependency on udev to the optee-client recipe, to make udevs pkg config data available. This might be considered similar “unwanted bloat” you mentioned related to systemd. I tried to make this conditional but could not find a safe way. According to [1] there is a variable called USE_DEVFS which tells if a device manager is available, but the variable is not visible to the recipe. AFAIK the image class would need to be inherited to get access to it, but that does not seem to be a good idea. As with systemd, I don't think udev should be in build time dependencies of recipes which just ship udev rules. In the same way, udev is not in the build dependencies by default and I don't think it's a good idea to add it there. > The op-tee client patch: > - adds build options to control if the system service file shall be installed or not. The default value is on, to keep backwards compatibility. > - another option is added to enable using pkg-config. This is by default off, and the op-tee client recipe enables it for yocto. If enabled both system and/or udev specific directories are discovered with pkg-config. > - the intention was to implement option and pkg-config processing in a flexible way. There are hard-coded default values which keep existing behavior and will be used if pkg-config is disabled or failing. > > Note: I missed the systemd dependency issue and conditional appending to DEPENDS is not implemented. > > 1: https://docs.yoctoproject.org/dev/dev-manual/device-manager.html Since we disagree on the solution, I'll stop proposing these changes. It's up to maintainer of optee-client recipe, Jon and Ross, to decide what to do with newer optee-client releases which do install a default udev rule and systemd service which use different users and groups than the meta-arm side optee-client recipe. I don't think shipping two optee udev rules is good either and thus would have kept that rule only in optee-client, possibly a separate binary package. Cheers, -Mikko > /George > > > Cheers, > > > > -Mikko > > > > > /George > > > > > > On 2024-10-29, 18:03, "meta-arm@lists.yoctoproject.org" <meta-arm@lists.yoctoproject.org> wrote: > > > > > > On 10/23/2024 9:54 AM, Mikko Rapeli wrote: > > > > Ok got it now. > > > > > > > > ${nonarch_base_libdir} isn't in the CMake toolchain file. We could > > > > move ${libdir}/systemd unconditionlly to ${nonarch_base_libdir}/systemd > > > > in the do_install task. Could you test and submit a patch like below? > > > > > > > > --- a/meta-arm/recipes-security/optee/optee-client.inc > > > > +++ b/meta-arm/recipes-security/optee/optee-client.inc > > > > @@ -23,8 +23,9 @@ EXTRA_OECMAKE:append:toolchain-clang = " -DCFG_WERROR=0" > > > > > > > > do_install:append() { > > > > # installed by default > > > > + mv ${D}${libdir}/systemd ${D}${noarch_base_libdir}/systemd > > > > if ! ${@bb.utils.contains('DISTRO_FEATURES', 'systemd', 'true', 'false', d)}; then > > > > - rm -rf ${D}${libdir}/systemd > > > > + rm -rf ${D}${noarch_base_libdir}/systemd > > > > fi > > > > if ${@bb.utils.contains('DISTRO_FEATURES', 'sysvinit', 'true', 'false', d)}; then > > > > install -D -p -m0755 ${UNPACKDIR}/tee-supplicant.sh ${D}${sysconfdir}/init.d/tee-supplicant > > > > > > > > Cheers, > > > > > > > > -Mikko > > > > > > This works fine in our testing. Sorry for the late response. > > > > > > Thanks again! > > > > > > Tom > > >
On 26 Nov 2024, at 14:55, Mikko Rapeli via lists.yoctoproject.org <mikko.rapeli=linaro.org@lists.yoctoproject.org> wrote: > This would only work if optee-client has "systemd" build time dependency in DEPENDS. > I don't think we should add this dependency. The sd_notify() support for example > dlopen()'s systemd libs at runtime to avoid making systemd a hard build time > dependency. I don't think we should add this dependency due to systemd > service file install paths. If this was all my code, I’d be flexible: add a cmake option so the caller can do -DSYSTEMD_UNIT_DIR=…, but if that isn’t set then ask pkg-config for the systemdsystemconfdir. Then in our recipes as we know that path is fixed, we can simply pass -DSYSTEMD_UNIT_DIR=${systemd_system_unitdir} when configuring and we don’t need to add a DEPENDS on systemd. You can always pass that option to cmake, and inheriting the systemd class will result in the units being deleted if systemd isn’t actually enabled in DISTRO_FEATURES. Ross
diff --git a/meta-arm/recipes-security/optee/optee-client.inc b/meta-arm/recipes-security/optee/optee-client.inc index f387c805..fc48c302 100644 --- a/meta-arm/recipes-security/optee/optee-client.inc +++ b/meta-arm/recipes-security/optee/optee-client.inc @@ -9,9 +9,7 @@ inherit systemd update-rc.d cmake useradd SRC_URI = " \ git://github.com/OP-TEE/optee_client.git;branch=master;protocol=https \ - file://tee-supplicant@.service \ file://tee-supplicant.sh \ - file://optee-udev.rules \ " UPSTREAM_CHECK_GITTAGREGEX = "^(?P<pver>\d+(\.\d+)+)$" @@ -20,20 +18,21 @@ S = "${WORKDIR}/git" EXTRA_OECMAKE = " \ -DBUILD_SHARED_LIBS=ON \ - -DCFG_TEE_FS_PARENT_PATH='${localstatedir}/lib/tee' \ " EXTRA_OECMAKE:append:toolchain-clang = " -DCFG_WERROR=0" do_install:append() { - install -D -p -m0644 ${UNPACKDIR}/tee-supplicant@.service ${D}${systemd_system_unitdir}/tee-supplicant@.service - install -D -p -m0755 ${UNPACKDIR}/tee-supplicant.sh ${D}${sysconfdir}/init.d/tee-supplicant - install -d ${D}${sysconfdir}/udev/rules.d - install -m 0644 ${UNPACKDIR}/optee-udev.rules ${D}${sysconfdir}/udev/rules.d/optee.rules - - sed -i -e s:@sysconfdir@:${sysconfdir}:g \ - -e s:@sbindir@:${sbindir}:g \ - ${D}${systemd_system_unitdir}/tee-supplicant@.service \ - ${D}${sysconfdir}/init.d/tee-supplicant + # installed by default + if ! ${@bb.utils.contains('DISTRO_FEATURES', 'systemd', 'true', 'false', d)}; then + rm -rf ${D}${libdir}/systemd + fi + if ${@bb.utils.contains('DISTRO_FEATURES', 'sysvinit', 'true', 'false', d)}; then + install -D -p -m0755 ${UNPACKDIR}/tee-supplicant.sh ${D}${sysconfdir}/init.d/tee-supplicant + sed -i -e s:@sysconfdir@:${sysconfdir}:g \ + -e s:@sbindir@:${sbindir}:g \ + ${D}${sysconfdir}/init.d/tee-supplicant + fi + install -o teesuppl -g teesuppl -m 0700 -d ${D}${localstatedir}/lib/tee } SYSTEMD_SERVICE:${PN} = "tee-supplicant@.service" @@ -42,5 +41,10 @@ INITSCRIPT_PACKAGES = "${PN}" INITSCRIPT_NAME:${PN} = "tee-supplicant" INITSCRIPT_PARAMS:${PN} = "start 10 1 2 3 4 5 . stop 90 0 6 ." +# Users and groups: +# tee group to access /dev/tee* +# teepriv group to acess /dev/teepriv*, only tee-supplicant +# teesuppl user and group teesuppl to run tee-supplicant USERADD_PACKAGES = "${PN}" -GROUPADD_PARAM:${PN} = "--system teeclnt" +GROUPADD_PARAM:${PN} = "--system tee; --system teepriv; --system teesuppl" +USERADD_PARAM:${PN} = "--system -g teesuppl --groups teepriv --home-dir ${localstatedir}/lib/tee -M --shell /sbin/nologin teesuppl;" diff --git a/meta-arm/recipes-security/optee/optee-client/0001-tee-supplicant-add-udev-rule-and-systemd-service-fil.patch b/meta-arm/recipes-security/optee/optee-client/0001-tee-supplicant-add-udev-rule-and-systemd-service-fil.patch new file mode 100644 index 00000000..18c0d950 --- /dev/null +++ b/meta-arm/recipes-security/optee/optee-client/0001-tee-supplicant-add-udev-rule-and-systemd-service-fil.patch @@ -0,0 +1,186 @@ +From bf0d02758696ee7a9f7af9e95f85f5c238d0e109 Mon Sep 17 00:00:00 2001 +From: Mikko Rapeli <mikko.rapeli@linaro.org> +Date: Wed, 2 Oct 2024 15:24:21 +0100 +Subject: [PATCH] tee-supplicant: add udev rule and systemd service file + +tee-supplicant startup with systemd init based +is non-trivial. Add sample udev rule and systemd +service files here so that distros can co-operate maintaining +them. + +Files are from meta-arm https://git.yoctoproject.org/meta-arm +at commit 7cce43e632daa8650f683ac726f9124681b302a4 with license +MIT and authors: + +Peter Griffin <peter.griffin@linaro.org> +Joshua Watt <JPEWhacker@gmail.com> +Javier Tia <javier.tia@linaro.org> +Mikko Rapeli <mikko.rapeli@linaro.org> + +With permission from the authors, files can be relicensed to +BSD-2-Clause like rest of optee client repo. + +The config files expect to find tee and teepriv system groups +and teesuppl user and group (part of teepriv group) for running +tee-supplicant. Additionally state directory /var/lib/tee +must be owned by teesuppl user and group with no rights +to other users. The groups and user can be changed via +CMake variables: + +CFG_TEE_GROUP +CFG_TEEPRIV_GROUP +CFG_TEE_SUPPL_USER +CFG_TEE_SUPPL_GROUP + +Change storage path from /data to /var/lib and +use standard CMake variables also for constructing install +paths which can be override to change the defaults: + +CMAKE_INSTALL_PREFIX, e.g. / +CMAKE_INSTALL_LIBDIR, e.g. /usr/lib +CMAKE_INSTALL_LOCALSTATEDIR /var + +Once these are setup, udev will start tee-supplicant in initramfs +or rootfs with teesuppl user and group when /dev/teepriv +device appears. The systemd service starts before tpm2.target +(new in systemd 256) which starts early in initramfs and in main rootfs. +This covers firmware TPM TA usecases for main rootfs encryption. When +stopping tee-supplicant, the ftpm kernel modules are removed and only +then the main process stopped to avoid fTPM breakage. These workarounds +may be removed once RPMB kernel and optee patches without tee-supplicant +are merged (Linux kernel >= 6.12-rc1, optee_os latest master or >= 4.4). + +Tested on yocto meta-arm setup which runs fTPM and optee-test/xtest +under qemuarm64: + +$ git clone https://git.yoctoproject.org/meta-arm +$ cd meta-arm +$ SSTATE_DIR=$HOME/sstate DL_DIR=$HOME/download kas build \ +ci/qemuarm64-secureboot.yml:ci/poky-altcfg.yml:ci/testimage.yml + +Compiled image can be manually started to qemu serial console with: + +$ SSTATE_DIR=$HOME/sstate DL_DIR=$HOME/download kas shell \ +ci/qemuarm64-secureboot.yml:ci/poky-altcfg.yml:ci/testimage.yml +$ runqemu slirp nographic + +meta-arm maintainers run these tests as part of their CI. + +Note that if the tee-supplicant state directory /var/lib/tee +can not be accessed due permissions or other problems, then +tee-supplicant startup with systemd still works. Only optee-test/xtest +will be failing and fTPM kernel drivers fail to load with error +messages. + +Cc: Peter Griffin <peter.griffin@linaro.org> +Cc: Joshua Watt <JPEWhacker@gmail.com> +Cc: Javier Tia <javier.tia@linaro.org> +Acked-by: Jerome Forissier <jerome.forissier@linaro.org> +Signed-off-by: Mikko Rapeli <mikko.rapeli@linaro.org> +--- + config.mk | 2 +- + libteec/CMakeLists.txt | 2 +- + tee-supplicant/CMakeLists.txt | 13 +++++++++++-- + tee-supplicant/optee-udev.rules.in | 7 +++++++ + tee-supplicant/tee-supplicant@.service.in | 17 +++++++++++++++++ + 5 files changed, 37 insertions(+), 4 deletions(-) + create mode 100644 tee-supplicant/optee-udev.rules.in + create mode 100644 tee-supplicant/tee-supplicant@.service.in + +Upstream-Status: Backport + +diff --git a/config.mk b/config.mk +index eae481f..3def087 100644 +--- a/config.mk ++++ b/config.mk +@@ -23,7 +23,7 @@ CFG_TEE_SUPP_LOG_LEVEL?=1 + # This folder can be created with the required permission in an init + # script during boot, else it will be created by the tee-supplicant on + # first REE FS access. +-CFG_TEE_FS_PARENT_PATH ?= /data/tee ++CFG_TEE_FS_PARENT_PATH ?= /var/lib/tee + + # CFG_TEE_CLIENT_LOG_FILE + # The location of the client log file when logging to file is enabled. +diff --git a/libteec/CMakeLists.txt b/libteec/CMakeLists.txt +index c742d31..c857369 100644 +--- a/libteec/CMakeLists.txt ++++ b/libteec/CMakeLists.txt +@@ -14,7 +14,7 @@ endif() + # Configuration flags always included + ################################################################################ + set(CFG_TEE_CLIENT_LOG_LEVEL "1" CACHE STRING "libteec log level") +-set(CFG_TEE_CLIENT_LOG_FILE "/data/tee/teec.log" CACHE STRING "Location of libteec log") ++set(CFG_TEE_CLIENT_LOG_FILE "${CMAKE_INSTALL_LOCALSTATEDIR}/lib/tee/teec.log" CACHE STRING "Location of libteec log") + + ################################################################################ + # Source files +diff --git a/tee-supplicant/CMakeLists.txt b/tee-supplicant/CMakeLists.txt +index 54a34c7..8df9bef 100644 +--- a/tee-supplicant/CMakeLists.txt ++++ b/tee-supplicant/CMakeLists.txt +@@ -11,10 +11,15 @@ option(CFG_TEE_SUPP_PLUGINS "Enable tee-supplicant plugin support" ON) + set(CFG_TEE_SUPP_LOG_LEVEL "1" CACHE STRING "tee-supplicant log level") + # FIXME: Question is, is this really needed? Should just use defaults from # GNUInstallDirs? + set(CFG_TEE_CLIENT_LOAD_PATH "/lib" CACHE STRING "Colon-separated list of paths where to look for TAs (see also --ta-dir)") +-set(CFG_TEE_FS_PARENT_PATH "/data/tee" CACHE STRING "Location of TEE filesystem (secure storage)") ++set(CFG_TEE_FS_PARENT_PATH "${CMAKE_INSTALL_LOCALSTATEDIR}/lib/tee" CACHE STRING "Location of TEE filesystem (secure storage)") + # FIXME: Why do we have if defined(CFG_GP_SOCKETS) && CFG_GP_SOCKETS == 1 in the c-file? + set(CFG_GP_SOCKETS "1" CACHE STRING "Enable GlobalPlatform Socket API support") +-set(CFG_TEE_PLUGIN_LOAD_PATH "/usr/lib/tee-supplicant/plugins/" CACHE STRING "tee-supplicant's plugins path") ++set(CFG_TEE_PLUGIN_LOAD_PATH "${CMAKE_INSTALL_PREFIX}/${CMAKE_INSTALL_LIBDIR}/${PROJECT_NAME}/plugins/" CACHE STRING "tee-supplicant's plugins path") ++ ++set(CFG_TEE_GROUP "tee" CACHE STRING "Group which has access to /dev/tee* devices") ++set(CFG_TEEPRIV_GROUP "teepriv" CACHE STRING "Group which has access to /dev/teepriv* devices") ++set(CFG_TEE_SUPPL_USER "teesuppl" CACHE STRING "User account which tee-supplicant is started with") ++set(CFG_TEE_SUPPL_GROUP "teesuppl" CACHE STRING "Group account which tee-supplicant is started with") + + if(CFG_TEE_SUPP_PLUGINS) + set(CMAKE_INSTALL_RPATH "${CFG_TEE_PLUGIN_LOAD_PATH}") +@@ -113,3 +118,7 @@ endif() + # Install targets + ################################################################################ + install(TARGETS ${PROJECT_NAME} RUNTIME DESTINATION ${CMAKE_INSTALL_SBINDIR}) ++configure_file(tee-supplicant@.service.in tee-supplicant@.service @ONLY) ++install(FILES ${CMAKE_BINARY_DIR}/${PROJECT_NAME}/tee-supplicant@.service DESTINATION ${CMAKE_INSTALL_LIBDIR}/systemd/system) ++configure_file(optee-udev.rules.in optee-udev.rules @ONLY) ++install(FILES ${CMAKE_BINARY_DIR}/${PROJECT_NAME}/optee-udev.rules DESTINATION ${CMAKE_INSTALL_SYSCONFDIR}/udev/rules.d) +diff --git a/tee-supplicant/optee-udev.rules.in b/tee-supplicant/optee-udev.rules.in +new file mode 100644 +index 0000000..275e833 +--- /dev/null ++++ b/tee-supplicant/optee-udev.rules.in +@@ -0,0 +1,7 @@ ++# SPDX-License-Identifier: BSD-2-Clause ++KERNEL=="tee[0-9]*", MODE="0660", OWNER="root", GROUP="@CFG_TEE_GROUP@", TAG+="systemd" ++ ++# If a /dev/teepriv[0-9]* device is detected, start an instance of ++# tee-supplicant.service with the device name as parameter ++KERNEL=="teepriv[0-9]*", MODE="0660", OWNER="root", GROUP="@CFG_TEEPRIV_GROUP@", \ ++ TAG+="systemd", ENV{SYSTEMD_WANTS}+="tee-supplicant@%k.service" +diff --git a/tee-supplicant/tee-supplicant@.service.in b/tee-supplicant/tee-supplicant@.service.in +new file mode 100644 +index 0000000..e53a935 +--- /dev/null ++++ b/tee-supplicant/tee-supplicant@.service.in +@@ -0,0 +1,17 @@ ++# SPDX-License-Identifier: BSD-2-Clause ++[Unit] ++Description=TEE Supplicant on %i ++DefaultDependencies=no ++After=dev-%i.device ++Wants=dev-%i.device ++Conflicts=shutdown.target ++Before=tpm2.target sysinit.target shutdown.target ++ ++[Service] ++Type=notify ++User=@CFG_TEE_SUPPL_USER@ ++Group=@CFG_TEE_SUPPL_GROUP@ ++EnvironmentFile=-@CMAKE_INSTALL_SYSCONFDIR@/default/tee-supplicant ++ExecStart=@CMAKE_INSTALL_PREFIX@/@CMAKE_INSTALL_SBINDIR@/tee-supplicant $OPTARGS ++# Workaround for fTPM TA: stop kernel module before tee-supplicant ++ExecStop=-/bin/sh -c "/sbin/modprobe -v -r tpm_ftpm_tee ; /bin/kill $MAINPID" +-- +2.34.1 + diff --git a/meta-arm/recipes-security/optee/optee-client/optee-udev.rules b/meta-arm/recipes-security/optee/optee-client/optee-udev.rules deleted file mode 100644 index 075f469c..00000000 --- a/meta-arm/recipes-security/optee/optee-client/optee-udev.rules +++ /dev/null @@ -1,6 +0,0 @@ -KERNEL=="tee[0-9]*", MODE="0660", OWNER="root", GROUP="teeclnt", TAG+="systemd" - -# If a /dev/teepriv[0-9]* device is detected, start an instance of -# tee-supplicant.service with the device name as parameter -KERNEL=="teepriv[0-9]*", MODE="0660", OWNER="root", GROUP="teeclnt", \ - TAG+="systemd", ENV{SYSTEMD_WANTS}+="tee-supplicant@%k.service" diff --git a/meta-arm/recipes-security/optee/optee-client/tee-supplicant@.service b/meta-arm/recipes-security/optee/optee-client/tee-supplicant@.service deleted file mode 100644 index e3039fde..00000000 --- a/meta-arm/recipes-security/optee/optee-client/tee-supplicant@.service +++ /dev/null @@ -1,13 +0,0 @@ -[Unit] -Description=TEE Supplicant on %i -DefaultDependencies=no -After=dev-%i.device -Wants=dev-%i.device -Conflicts=shutdown.target -Before=tpm2.target sysinit.target shutdown.target - -[Service] -Type=notify -EnvironmentFile=-@sysconfdir@/default/tee-supplicant -ExecStart=@sbindir@/tee-supplicant $OPTARGS -ExecStop=-/bin/sh -c "/sbin/modprobe -v -r tpm_ftpm_tee ; /bin/kill $MAINPID" diff --git a/meta-arm/recipes-security/optee/optee-client_4.3.0.bb b/meta-arm/recipes-security/optee/optee-client_4.3.0.bb index 4a088004..edab4583 100644 --- a/meta-arm/recipes-security/optee/optee-client_4.3.0.bb +++ b/meta-arm/recipes-security/optee/optee-client_4.3.0.bb @@ -2,6 +2,8 @@ require recipes-security/optee/optee-client.inc SRCREV = "a5b1ffcd26e328af0bbf18ab448a38ecd558e05c" +SRC_URI += "file://0001-tee-supplicant-add-udev-rule-and-systemd-service-fil.patch" + inherit pkgconfig DEPENDS += "util-linux" EXTRA_OEMAKE += "PKG_CONFIG=pkg-config"
Use backported upstream patch for udev rule and systemd service file. sysvinit script is still used from meta-arm. Don't install systemd service without systemd distro feature, other way round for sysvinit script. tee-supplicant started by systemd service runs as non-root teesuppl user with teepriv group. sysvinit still runs as root since busybox start-stop-daemon doesn't support -g group parameter and -u teesuppl doesn't seem to change the effective user. udev rules allow non-root /dev/tee* access from tee and /dev/teepriv* access from teepriv groups. Tested sysvinit changes with: $ kas build ci/qemuarm64-secureboot.yml:ci/poky.yml:ci/testimage.yml and systemd changes with: $ kas build ci/qemuarm64-secureboot.yml:ci/poky.yml:ci/testimage.yml:ci/uefi-secureboot.yml Cc: tom.hochstein@nxp.com Cc: sahil.malhotra@nxp.com Signed-off-by: Mikko Rapeli <mikko.rapeli@linaro.org> --- .../recipes-security/optee/optee-client.inc | 30 +-- ...dd-udev-rule-and-systemd-service-fil.patch | 186 ++++++++++++++++++ .../optee/optee-client/optee-udev.rules | 6 - .../optee-client/tee-supplicant@.service | 13 -- .../optee/optee-client_4.3.0.bb | 2 + 5 files changed, 205 insertions(+), 32 deletions(-) create mode 100644 meta-arm/recipes-security/optee/optee-client/0001-tee-supplicant-add-udev-rule-and-systemd-service-fil.patch delete mode 100644 meta-arm/recipes-security/optee/optee-client/optee-udev.rules delete mode 100644 meta-arm/recipes-security/optee/optee-client/tee-supplicant@.service