Skip to content

Commit

Permalink
tee-supplicant: -d: return after TEE device is opened
Browse files Browse the repository at this point in the history
This commit addresses a race condition when a kernel module using OP-TEE
is loaded immediately after tee-supplicant is started. To understand the
problem, consider that with a shell background task there is no guarantee
that the service is available to the kernel when the command returns.
So the following:

  tee-supplicant &
  modprobe <some_module>

...may fail when <some_module> invokes the kernel TEE client API (note
that kernel users do NOT wait for the supplicant to be available [1],
contrary to user space clients [1]).

This scenario was addressed by commit 66cdd5d ("tee-supplicant: add
daemon mode (-d)"), although the commit description did not explicitly
mention it. With '-d ' the supplicant command would open the device
before returning. Unfortunately, this was inadvertently broken by commit
523d50b ("tee-supplicant: daemonize before opening a supplicant
device").

Restore the previous behavior while still keeping the open() call in the
child process, by introducing some synchronization between the parent
and the child. A pipe is created and the parent issues a blocking read.
After successfully opening the device the child writes data to the pipe,
thus releasing the parent. If the child crashes or exits before writing
the parent is released with 0 bytes read and exits with an error status.

The daemon() call is replaced by make_daemon() which is an open-coded
version of the daemon() funcion as described in the Linux man page,
modified to deal with the IPC.

Link: [1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/tee/tee_core.c?h=v5.19#n1128
Link: [2] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/drivers/tee/tee_core.c?h=v5.19#n117
Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
  • Loading branch information
jforissier committed Aug 18, 2022
1 parent d59ed2d commit 492410d
Showing 1 changed file with 77 additions and 8 deletions.
85 changes: 77 additions & 8 deletions tee-supplicant/src/tee_supplicant.c
Original file line number Diff line number Diff line change
Expand Up @@ -491,8 +491,8 @@ static int usage(int status)
{
fprintf(stderr, "Usage: tee-supplicant [options] [<device-name>]\n");
fprintf(stderr, "\t-h, --help: this help\n");
fprintf(stderr, "\t-d, --daemonize: run as a daemon (fork after successful "
"initialization)\n");
fprintf(stderr, "\t-d, --daemonize: run as a daemon (fork and return "
"after child has opened the TEE device or on error)\n");
fprintf(stderr, "\t-f, --fs-parent-path: secure fs parent path [%s]\n",
supplicant_params.fs_parent_path);
fprintf(stderr, "\t-t, --ta-dir: TAs dirname under %s [%s]\n", TEEC_LOAD_PATH,
Expand Down Expand Up @@ -736,9 +736,61 @@ static void set_ta_path(void)
exit(EXIT_FAILURE);
}

/*
* Similar to the standard libc function daemon(0, 0) but the parent process
* issues a blocking read on pipefd[0] before exiting.
* Returns 0 on success, <0 on error.
*/
static int make_daemon(int pipefd[2])
{
int fd = 0;
char c = 0;
int n = 0;

switch (fork()) {
case -1:
return -1;
case 0:
/* In child */
close(pipefd[0]);
break;
default:
/* In parent */
close(pipefd[1]);
n = read(pipefd[0], &c, 1);
close(pipefd[0]);
if (!n) {
/*
* Nothing has been read: child has closed without
* writing (either exited on error or crashed)
*/
return -1;
}
/* Child is done with the opening of the TEE device */
_exit(EXIT_SUCCESS);
}

if (setsid() < 0)
return -2;

if (chdir("/") < 0)
return -3;

fd = open("/dev/null", O_RDWR);
if (fd < 0)
return -4;
dup2(fd, 0);
dup2(fd, 1);
dup2(fd, 2);
close(fd);

return 0;
}

int main(int argc, char *argv[])
{
struct thread_arg arg = { .fd = -1 };
int pipefd[2] = { 0, };
bool daemonize = false;
char *dev = NULL;
int e = 0;
Expand Down Expand Up @@ -800,12 +852,25 @@ int main(int argc, char *argv[])
}
}

if (daemonize && daemon(0, 0) < 0) {
EMSG("daemon(): %s", strerror(errno));

set_ta_path();

if (plugin_load_all() != 0) {
EMSG("failed to load plugins");
exit(EXIT_FAILURE);
}

set_ta_path();
if (daemonize) {
if (pipe(pipefd) < 0) {
EMSG("pipe(): %s", strerror(errno));
exit(EXIT_FAILURE);
}
e = make_daemon(pipefd);
if (e < 0) {
EMSG("make_daemon(): %d", e);
exit(EXIT_FAILURE);
}
}

if (dev) {
arg.fd = open_dev(dev, &arg.gen_caps);
Expand All @@ -821,9 +886,13 @@ int main(int argc, char *argv[])
}
}

if (plugin_load_all() != 0) {
EMSG("failed to load plugins");
exit(EXIT_FAILURE);
if (daemonize) {
/* Release parent */
if (write(pipefd[1], "", 1) != 1) {
EMSG("write(): %s", strerror(errno));
exit(EXIT_FAILURE);
}
close(pipefd[1]);
}

while (!arg.abort) {
Expand Down

0 comments on commit 492410d

Please sign in to comment.