tee-supplicant: send READY=1 notification to systemd #383

embetrix · 2024-05-30T11:39:44Z

This is very useful when tee-supplicant is started from systemd and can used with Type=notify to signal readiness

flags.mk

tee-supplicant/src/tee_supplicant.c

jenswi-linaro · 2024-05-30T12:21:39Z

tee-supplicant/src/tee_supplicant.c

@@ -835,6 +839,7 @@ int main(int argc, char *argv[])
 		/* long name      | has argument  | flag | short value */
 		{ "help",            no_argument,       0, 'h' },
 		{ "daemonize",       no_argument,       0, 'd' },
+		{ "sdnotify",        no_argument,       0, 'n' },


-n usually means dry-run in other utilities. How about -s instead?

jenswi-linaro · 2024-05-30T12:27:02Z

tee-supplicant/src/tee_supplicant.c

@@ -923,6 +931,28 @@ int main(int argc, char *argv[])
 		}
 	}

+	if ((sdnotify) && (!daemonize)) {
+		/* we are set here notify systemd */
+		int(*__sd_notify__)();


The __ are ugly, I suppose you've added them to avoid conflicts with sd_notify() in <systemd/sd-daemon.h>. How about int (*my_sd_notify)(int unset_environment, const char *state)?
Please initialize all local variables.

jenswi-linaro · 2024-05-30T12:27:25Z

tee-supplicant/src/tee_supplicant.c

+		int(*__sd_notify__)();
+		void *systemd = dlopen("libsystemd.so", RTLD_LAZY);
+		if (systemd) {
+			*(int**)(&__sd_notify__) = dlsym(systemd, "sd_notify");


Why do you need all this casting?

jenswi-linaro · 2024-05-30T12:27:38Z

tee-supplicant/src/tee_supplicant.c

@@ -923,6 +931,28 @@ int main(int argc, char *argv[])
 		}
 	}

+	if ((sdnotify) && (!daemonize)) {


The extra () are not needed.

jenswi-linaro · 2024-05-30T12:27:52Z

tee-supplicant/src/tee_supplicant.c

+			*(int**)(&__sd_notify__) = dlsym(systemd, "sd_notify");
+			if (__sd_notify__) {
+					int ret = __sd_notify__(0, "READY=1");
+					if (ret <= 0) {


The extra {} are not needed.

embetrix · 2024-05-30T14:53:17Z

@jforissier : I just figured out on the target testing that yocto doesn't ship libsystemd.so

but only versioned so.

# ls -l /lib/libsystemd.so*
lrwxrwxrwx 1 root root     20 Jul  9  2020 /lib/libsystemd.so.0 -> libsystemd.so.0.33.0
-rwxr-xr-x 1 root root 989248 Jul  9  2020 /lib/libsystemd.so.0.33.0

the dlopen won't work unless the symlink /lib/libsystemd.so is present

jforissier · 2024-05-30T15:16:08Z

@embetrix dlopen("libsystemd.so.0", RTLD_LAZY); should work fine. Using libsystemd.so is actually not a good idea because you don't know which major version it is. It could lead to compatibility issues.

embetrix · 2024-05-30T15:21:53Z

@embetrix dlopen("libsystemd.so.0", RTLD_LAZY); should work fine. Using libsystemd.so is actually not a good idea because you don't know which major version it is. It could lead to compatibility issues.

done

jenswi-linaro · 2024-05-30T15:20:48Z

tee-supplicant/src/tee_supplicant.c

+					if (ret <= 0)
+						fprintf(stderr, "sd_notify failed: %d\n", ret);
+			}
+			else


Please fold up else to make it } else

tee-supplicant/src/tee_supplicant.c

jenswi-linaro · 2024-05-30T15:23:43Z

tee-supplicant/src/tee_supplicant.c

+	if (sdnotify && !daemonize) {
+		int ret = 0;
+		void *sd_handle = NULL;
+ 		int (*sd_notify)(int unset_environment, const char *state) =  NULL;


This is bound to cause a compile error if <systemd/sd-daemon.h> is ever included, that's why I suggested my_sd_notify or such.

mikkorapeli-linaro · 2024-06-03T11:53:00Z

tee-supplicant/src/tee_supplicant.c

+	if (sd_handle) {
+		tee_sd_notify = dlsym(sd_handle, "sd_notify");
+		if (tee_sd_notify) {
+				e = tee_sd_notify(0, "READY=1");


While at it, could you add a "tee_sd_notify(0, "WATCHDOG=1"); into the main loops which call process_one_request()?

What could be the maximum time that a process_one_request() call takes? We could then double this and add to systemd service file to detect if tee-supplicant hangs. I've seen hangs related ftpm kernel module unloading which could affect tee-supplicant too, for example.

How long does it take to copy a file (of unknown size) from the file system into memory, worst case?

Ok, the watchdog target is application dependent but the sd_notify() call to kick the watchdog could be generic in the loop.

Users could set the application specific watchdog timeout in the systemd service file for tee-supplicant. Default could be that it's not set at all.

The ioctl() call is blocking until a request comes, with no timeout. So we would at least need to add a timeout before adding support for such a watchdog. An alternative would be to ping from a separate thread but that's obviously a weaker health guarantee.

The watchdog call from tee-supplicant to systemd is just a health signal. It can block for ever. The timeout value is set in systemd service file, which can be unset by default, or a really large value. Dending on use cases and TAs, then this value can be reduced.

But it's ok to skip this for now too.

mikkorapeli-linaro · 2024-06-03T11:54:03Z

tee-supplicant/src/tee_supplicant.c

+					fprintf(stderr, "sd_notify failed: %d\n", e);
+		} else
+			fprintf(stderr, "Couldn't find sd_notify symbol: %s\n", dlerror());
+		dlclose(sd_handle);


If watchdog calls are done, I'd leave this open until exiting mainloops

also note that libsystemd.so is nowadays built with -z nodelete, i.e. we do not support being unloaded, and the dlclose() is hence without effect

mikkorapeli-linaro · 2024-06-10T07:36:29Z

Tested on Linaro Trusted Substrate firmware https://gitlab.com/Linaro/trustedsubstrate/meta-ts/ and TRS https://trs.readthedocs.io/en/latest/ yocto based distro with systemd initramfs using service file, running on ARM64 Rockpi4b:

[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

udev rule:

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"

boot log in initramfs which uses optee early fTPM TA to create an encrypted rootfs:

[0;38;5;245mteepriv0: Processing udev action (SEQNUM=4838, ACTION=add)[0m
[0;38;5;245mteepriv0: Got 'add' action on syspath '/sys/devices/virtual/tee/teepriv0'.[0m
[0;38;5;245mUnit tee-supplicant@teepriv0.service has alias tee-supplicant@.service.[0m
[0;38;5;245mdev-teepriv0.device: Changed dead -> plugged[0m
[0;38;5;245msys-devices-virtual-tee-teepriv0.device: Changed dead -> plugged[0m
[0;38;5;245mtee-supplicant@teepriv0.service: Trying to enqueue job tee-supplicant@teepriv0.service/start/fail[0m
[0;38;5;245mtee-supplicant@teepriv0.service: Installed new job tee-supplicant@teepriv0.service/start as 126[0m
[0;38;5;245msystem-tee\x2dsupplicant.slice: Installed new job system-tee\x2dsupplicant.slice/start as 127[0m
[0;38;5;245mtee-supplicant@teepriv0.service: Enqueued job tee-supplicant@teepriv0.service/start as 126[0m
[0;38;5;245msystem-tee\x2dsupplicant.slice: Failed to set 'memory.zswap.max' attribute on '/system.slice/system-tee\x2dsupplicant.slice' to 'max': No such file or directory[0m
[0;38;5;245msystem-tee\x2dsupplicant.slice changed dead -> active[0m
[0;38;5;245msystem-tee\x2dsupplicant.slice: Job 127 system-tee\x2dsupplicant.slice/start finished, result=done[0m
[[0;32m  OK  [0m] Created slice [0;1;39mSlice /system/tee-supplicant[0m.
[0;38;5;245mtee-supplicant@teepriv0.service: Will spawn child (service_enter_start): /usr/sbin/tee-supplicant[0m
[0;38;5;245mtee-supplicant@teepriv0.service: Failed to set 'memory.zswap.max' attribute on '/system.slice/system-tee\x2dsupplicant.slice/tee-supplicant@teepriv0.service' to 'max': No such file or directory[0m
[0;38;5;245mtee-supplicant@teepriv0.service: Passing 0 fds to service[0m
[0;38;5;245mtee-supplicant@teepriv0.service: About to execute: /usr/sbin/tee-supplicant "\$OPTARGS"[0m
...
[0;38;5;245mtee-supplicant@teepriv0.service: Changed dead -> start[0m
[0;38;5;245mFound cgroup2 on /sys/fs/cgroup/, full unified hierarchy[0m
         Starting [0;1;39mTEE Supplicant on teepriv0[0m...
...
[0;1;38;5;185mtee-supplicant@teepriv0.service: Referenced but unset environment variable evaluates to an empty string: OPTARGS[0m
...
[0;38;5;245mtee-supplicant@teepriv0.service: Got notification message from PID 235 (READY=1)[0m
[0;38;5;245mtee-supplicant@teepriv0.service: Changed start -> running[0m
[0;38;5;245mtee-supplicant@teepriv0.service: Job 126 tee-supplicant@teepriv0.service/start finished, result=done[0m
[[0;32m  OK  [0m] Started [0;1;39mTEE Supplicant on teepriv0[0m.
[0;38;5;245mSent message type=signal sender=org.freedesktop.systemd1 destination=n/a path=/org/freedesktop/systemd1/unit/tee_2dsupplicant_40teepriv0_2eservice interface=org.freedesktop.DBus.Properties member=PropertiesChanged cookie=6 reply_cookie=0 signature=sa{sv}as error-name=n/a error-message=n/a[0m
[0;38;5;245mSent message type=signal sender=org.freedesktop.systemd1 destination=n/a path=/org/freedesktop/systemd1/unit/tee_2dsupplicant_40teepriv0_2eservice interface=org.freedesktop.DBus.Properties member=PropertiesChanged cookie=7 reply_cookie=0 signature=sa{sv}as error-name=n/a error-message=n/a[0m
[0;38;5;245mSent message type=signal sender=org.freedesktop.systemd1 destination=n/a path=/org/freedesktop/systemd1 interface=org.freedesktop.systemd1.Manager member=JobRemoved cookie=8 reply_cookie=0 signature=uoss error-name=n/a error-message=n/a[0m
[0;38;5;245mFailed to communicate with plymouth: Connection refused[0m
[0;38;5;245mSent message type=signal sender=org.freedesktop.systemd1 destination=n/a path=/org/freedesktop/systemd1/unit/tee_2dsupplicant_40teepriv0_2eservice interface=org.freedesktop.DBus.Properties member=PropertiesChanged cookie=9 reply_cookie=0 signature=sa{sv}as error-name=n/a error-message=n/a[0m
[0;38;5;245mSent message type=signal sender=org.freedesktop.systemd1 destination=n/a path=/org/freedesktop/systemd1/unit/tee_2dsupplicant_40teepriv0_2eservice interface=org.freedesktop.DBus.Properties member=PropertiesChanged cookie=10 reply_cookie=0 signature=sa{sv}as error-name=n/a error-message=n/a[0m

So this sd_notify() patch works nicely. Thanks!

poettering · 2024-06-10T08:06:07Z

tee-supplicant/src/tee_supplicant.c

@@ -923,6 +926,20 @@ int main(int argc, char *argv[])
 		}
 	}

+	/* we are set here notify systemd */
+	sd_handle = dlopen("libsystemd.so.0", RTLD_LAZY);


sorry for parachuting in here, I was pointed to this PR from here:

https://lists.freedesktop.org/archives/systemd-devel/2024-June/050385.html

I just wanted to mention that for a trivial call like sd_notify() you might just opt directly implementing this, rather than doing dlopen()/dlsym(). i.e. something like this:

https://www.freedesktop.org/software/systemd/man/devel/sd_notify.html#Standalone%20Implementations

poettering · 2024-06-10T08:08:41Z

tee-supplicant/src/tee_supplicant.c

+		if (tee_sd_notify) {
+				e = tee_sd_notify(0, "READY=1");
+				if (e <= 0)
+					fprintf(stderr, "sd_notify failed: %d\n", e);


please do not consider e == 0 an error, it's really not. It just means no message sent because $NOTIFY_SOCKET wasn't set, i.e. because it was invoked in a non-systemd context, which is entirely fine however.

tee-supplicant/CMakeLists.txt

tee-supplicant/src/tee_sd_notify.c

tee-supplicant/src/tee_sd_notify.h

tee-supplicant/src/tee_sd_notify.c

mikkorapeli-linaro · 2024-06-11T05:07:18Z

Tested latest version of patch with TS and TRS in https://ledge.validation.linaro.org/scheduler/job/88692 and it works. tee-supplicant startup state is correctly reported to systemd.

jforissier

See comments below.
Please use prefix in commit subject: "tee-supplicant: send READY=1 notification to systemd"
Please add your Signed-off-by:

tee-supplicant/src/sd_notify.h

tee-supplicant/src/tee_supplicant.c

embetrix · 2024-06-11T08:18:40Z

See comments below.

Please use prefix in commit subject: "tee-supplicant: send READY=1 notification to systemd"
fixed

Please add your :
added Signed-off-by

jforissier

Please wrap the commit description at 72 characters and add:

Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

@poettering thanks for your recommendations

This option is very useful when tee-supplicant is started from systemd and can used with Type=notify to signal readiness Signed-off-by: Ayoub Zaki <ayoub.zaki@embetrix.com> Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

embetrix · 2024-06-11T08:54:57Z

Please wrap the commit description at 72 characters and add:

Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

@poettering thanks for your recommendations

done

jforissier · 2024-06-12T12:17:14Z

Any more feedback? @mikkorapeli-linaro would you mind giving an Ack and/or Tested-by perhaps?

mikkorapeli-linaro · 2024-06-13T01:09:58Z

Any more feedback? @mikkorapeli-linaro would you mind giving an Ack and/or Tested-by perhaps?

Yes, latest patch version also

Tested-by: Mikko Rapeli mikko.rapeli@linaro.org

TS firmware with optee 4.1 and RPMB kernel patches from Jens, and TRS uki kernel and initramfs with systemd which starts tee-supplicant using this patch to create fTPM encrypted rootfs, on rockpi4b. The few remaining failures in TS/TRS test suite are not related to this patch.

Test run:
https://ledge.validation.linaro.org/scheduler/job/88800

embetrix force-pushed the feature/systemd-notify branch 2 times, most recently from c7649f8 to 0b76849 Compare May 30, 2024 11:52

jforissier reviewed May 30, 2024

View reviewed changes

flags.mk Outdated Show resolved Hide resolved

tee-supplicant/src/tee_supplicant.c Outdated Show resolved Hide resolved

tee-supplicant/src/tee_supplicant.c Outdated Show resolved Hide resolved

embetrix force-pushed the feature/systemd-notify branch from 0b76849 to 3b5d908 Compare May 30, 2024 12:28

jenswi-linaro reviewed May 30, 2024

View reviewed changes

embetrix force-pushed the feature/systemd-notify branch 11 times, most recently from 0c60cfb to 920d783 Compare May 30, 2024 13:44

embetrix force-pushed the feature/systemd-notify branch from 920d783 to 41ef939 Compare May 30, 2024 15:21

jenswi-linaro reviewed May 30, 2024

View reviewed changes

embetrix force-pushed the feature/systemd-notify branch 3 times, most recently from 65d76d3 to d130624 Compare May 31, 2024 11:54

mikkorapeli-linaro mentioned this pull request Jun 3, 2024

tee-supplicant readiness from systemd #382

Open

mikkorapeli-linaro reviewed Jun 3, 2024

View reviewed changes

poettering reviewed Jun 10, 2024

View reviewed changes

embetrix force-pushed the feature/systemd-notify branch 4 times, most recently from 7044c13 to 50010dd Compare June 10, 2024 12:49

jforissier reviewed Jun 10, 2024

View reviewed changes

tee-supplicant/CMakeLists.txt Outdated Show resolved Hide resolved

tee-supplicant/src/tee_sd_notify.c Outdated Show resolved Hide resolved

tee-supplicant/src/tee_sd_notify.h Outdated Show resolved Hide resolved

tee-supplicant/src/tee_sd_notify.c Outdated Show resolved Hide resolved

embetrix force-pushed the feature/systemd-notify branch 4 times, most recently from 2cddc19 to 37a6124 Compare June 10, 2024 13:49

jforissier reviewed Jun 11, 2024

View reviewed changes

tee-supplicant/src/sd_notify.h Outdated Show resolved Hide resolved

tee-supplicant/src/tee_supplicant.c Outdated Show resolved Hide resolved

embetrix force-pushed the feature/systemd-notify branch 5 times, most recently from 0cece47 to fc9f273 Compare June 11, 2024 08:18

embetrix changed the title ~~Add option to send READY=1 notification to systemd after tee-supplicant is setup~~ tee-supplicant: send READY=1 notification to systemd Jun 11, 2024

jforissier approved these changes Jun 11, 2024

View reviewed changes

tee-supplicant: send READY=1 notification to systemd

e3ebccc

This option is very useful when tee-supplicant is started from systemd and can used with Type=notify to signal readiness Signed-off-by: Ayoub Zaki <ayoub.zaki@embetrix.com> Acked-by: Jerome Forissier <jerome.forissier@linaro.org>

embetrix force-pushed the feature/systemd-notify branch from fc9f273 to e3ebccc Compare June 11, 2024 08:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

tee-supplicant: send READY=1 notification to systemd #383

tee-supplicant: send READY=1 notification to systemd #383

embetrix commented May 30, 2024 •

edited

jenswi-linaro May 30, 2024

jenswi-linaro May 30, 2024 •

edited

jenswi-linaro May 30, 2024

jenswi-linaro May 30, 2024

jenswi-linaro May 30, 2024

embetrix commented May 30, 2024 •

edited

jforissier commented May 30, 2024

embetrix commented May 30, 2024

jenswi-linaro May 30, 2024

jenswi-linaro May 30, 2024

mikkorapeli-linaro Jun 3, 2024

jenswi-linaro Jun 3, 2024

mikkorapeli-linaro Jun 3, 2024

jforissier Jun 3, 2024

mikkorapeli-linaro Jun 10, 2024

mikkorapeli-linaro Jun 3, 2024

poettering Jun 10, 2024

mikkorapeli-linaro commented Jun 10, 2024

poettering Jun 10, 2024

poettering Jun 10, 2024

mikkorapeli-linaro commented Jun 11, 2024

jforissier left a comment

embetrix commented Jun 11, 2024

jforissier left a comment

embetrix commented Jun 11, 2024

jforissier commented Jun 12, 2024

mikkorapeli-linaro commented Jun 13, 2024

tee-supplicant: send READY=1 notification to systemd #383

Are you sure you want to change the base?

tee-supplicant: send READY=1 notification to systemd #383

Conversation

embetrix commented May 30, 2024 • edited

Choose a reason for hiding this comment

jenswi-linaro May 30, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

embetrix commented May 30, 2024 • edited

jforissier commented May 30, 2024

embetrix commented May 30, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikkorapeli-linaro commented Jun 10, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikkorapeli-linaro commented Jun 11, 2024

jforissier left a comment

Choose a reason for hiding this comment

embetrix commented Jun 11, 2024

jforissier left a comment

Choose a reason for hiding this comment

embetrix commented Jun 11, 2024

jforissier commented Jun 12, 2024

mikkorapeli-linaro commented Jun 13, 2024

embetrix commented May 30, 2024 •

edited

jenswi-linaro May 30, 2024 •

edited

embetrix commented May 30, 2024 •

edited