Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

tee-supplicant: Enable command line support for RPMB_EMU #355

Closed
wants to merge 1 commit into from
Closed

tee-supplicant: Enable command line support for RPMB_EMU #355

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions tee-supplicant/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ project (tee-supplicant C)
# Configuration flags always included
################################################################################
option (CFG_TA_TEST_PATH "Enable tee-supplicant to load from test/debug path" OFF)
option (RPMB_EMU "Enable tee-supplicant to emulate RPMB" ON)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For backward compat, could config switch RPMB_EMU be preserved and set whether or not RPMB emulation is default enabled?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it really help? What would happen if set to OFF? I'd rather simplify things as much as possible.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean preserving the config switch but just default it is OFF? If so, fine with me.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No I mean remove this config entirely, because otherwise it would be misleading when set to OFF.

option (CFG_TA_GPROF_SUPPORT "Enable tee-supplicant support for TAs instrumented with gprof" ON)
option (CFG_FTRACE_SUPPORT "Enable tee-supplicant support for TAs instrumented with ftrace" ON)
option (CFG_TEE_SUPP_PLUGINS "Enable tee-supplicant plugin support" ON)
Expand Down Expand Up @@ -74,11 +73,6 @@ if (CFG_TA_TEST_PATH)
PRIVATE -DCFG_TA_TEST_PATH=${CFG_TA_TEST_PATH})
endif()

if (RPMB_EMU)
target_compile_definitions (${PROJECT_NAME}
PRIVATE -DRPMB_EMU=1)
endif()

if (CFG_TA_GPROF_SUPPORT)
target_compile_definitions (${PROJECT_NAME}
PRIVATE -DCFG_TA_GPROF_SUPPORT)
Expand Down
9 changes: 1 addition & 8 deletions tee-supplicant/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,6 @@ include ../config.mk

OUT_DIR := $(OO)/tee-supplicant

# Emulate RPMB ioctl's by default
RPMB_EMU ?= 1

.PHONY: all tee-supplicant clean

all: tee-supplicant
Expand All @@ -24,9 +21,8 @@ ifeq ($(CFG_GP_SOCKETS),y)
TEES_SRCS += tee_socket.c
endif

ifeq ($(RPMB_EMU),1)
TEES_SRCS += sha2.c hmac_sha2.c
endif

ifneq (,$(filter y,$(CFG_TA_GPROF_SUPPORT) $(CFG_FTRACE_SUPPORT)))
TEES_SRCS += prof.c
endif
Expand All @@ -51,9 +47,6 @@ TEES_CFLAGS := $(addprefix -I, $(TEES_INCLUDES)) $(CFLAGS) \
ifeq ($(CFG_GP_SOCKETS),y)
TEES_CFLAGS += -DCFG_GP_SOCKETS=1
endif
ifeq ($(RPMB_EMU),1)
TEES_CFLAGS += -DRPMB_EMU=1
endif
ifeq ($(CFG_TA_TEST_PATH),y)
TEES_CFLAGS += -DCFG_TA_TEST_PATH=1
endif
Expand Down
88 changes: 52 additions & 36 deletions tee-supplicant/src/rpmb.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,12 +26,14 @@
*/

#include <dirent.h>
#include <errno.h>
#include <fcntl.h>
#include <linux/types.h>
#include <linux/mmc/ioctl.h>
#include <netinet/in.h>
#include <pthread.h>
#include <rpmb.h>
#include <stdarg.h>
#include <stdbool.h>
#include <stdlib.h>
#include <string.h>
Expand All @@ -43,12 +45,7 @@
#include <tee_supplicant.h>
#include <unistd.h>

#ifdef RPMB_EMU
#include <stdarg.h>
#include "hmac_sha2.h"
#else
#include <errno.h>
#endif

/*
* Request and response definitions must be in sync with the secure side
Expand Down Expand Up @@ -139,17 +136,16 @@ static pthread_mutex_t rpmb_mutex = PTHREAD_MUTEX_INITIALIZER;
/* Maximum number of commands used in a multiple ioc command request */
#define RPMB_MAX_IOC_MULTI_CMDS 3

#ifndef RPMB_EMU

#define IOCTL(fd, request, ...) \
({ \
int ret; \
ret = ioctl((fd), (request), ##__VA_ARGS__); \
if (ret < 0) \
EMSG("ioctl ret=%d errno=%d", ret, errno); \
ret; \
})
typedef struct {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prefer without a typedef definition, just struct rpmb_ops { ... };.

int (*ioctl)(int d, long unsigned int request, ...);
int (*get_fd)(uint16_t dev_id);
TEEC_Result (*read_cid)(uint16_t dev_id, uint8_t *cid);
TEEC_Result (*read_size_mult)(uint16_t dev_id, uint8_t *value);
TEEC_Result (*read_rel_wr_sec_c)(uint16_t dev_id, uint8_t *value);
bool (*remap_dev_id)(uint16_t dev_id, uint16_t *ndev_id);
} rpmb_ops_t;

static rpmb_ops_t rpmb_ops;

/* Open and/or return file descriptor to RPMB partition of device dev_id */
static int mmc_rpmb_fd(uint16_t dev_id)
Expand Down Expand Up @@ -389,10 +385,6 @@ static bool remap_rpmb_dev_id(uint16_t dev_id, uint16_t *ndev_id)
return found;
}

#else /* RPMB_EMU */

#define IOCTL(fd, request, ...) ioctl_emu((fd), (request), ##__VA_ARGS__)

/* Emulated rel_wr_sec_c value (reliable write size, *256 bytes) */
#define EMU_RPMB_REL_WR_SEC_C 1
/* Emulated rpmb_size_mult value (RPMB size, *128 kB) */
Expand All @@ -414,9 +406,7 @@ struct rpmb_emu {
uint16_t address;
} last_op;
};
static struct rpmb_emu rpmb_emu = {
.size = EMU_RPMB_SIZE_BYTES
};
static struct rpmb_emu *rpmb_emu;

static struct rpmb_emu *mem_for_fd(int fd)
{
Expand All @@ -429,7 +419,7 @@ static struct rpmb_emu *mem_for_fd(int fd)
return NULL;
}

return &rpmb_emu;
return rpmb_emu;
}

#if (DEBUGLEVEL >= TRACE_FLOW)
Expand Down Expand Up @@ -609,7 +599,7 @@ static void ioctl_emu_read_ctr(struct rpmb_emu *mem,
frm->op_result = compute_hmac(mem, frm, 1);
}

static uint32_t read_cid(uint16_t dev_id, uint8_t *cid)
static uint32_t read_cid_emu(uint16_t dev_id, uint8_t *cid)
{
/* Taken from an actual eMMC chip */
static const uint8_t test_cid[] = {
Expand Down Expand Up @@ -747,38 +737,36 @@ static int ioctl_emu(int fd, unsigned long request, ...)
return res;
}

static int mmc_rpmb_fd(uint16_t dev_id)
static int mmc_rpmb_fd_emu(uint16_t dev_id)
{
(void)dev_id;

/* Any value != -1 will do in test mode */
return 0;
}

static TEEC_Result read_size_mult(uint16_t dev_id, uint8_t *value)
static TEEC_Result read_size_mult_emu(uint16_t dev_id, uint8_t *value)
{
(void)dev_id;

*value = EMU_RPMB_SIZE_MULT;
return TEEC_SUCCESS;
}

static TEEC_Result read_rel_wr_sec_c(uint16_t dev_id, uint8_t *value)
static TEEC_Result read_rel_wr_sec_c_emu(uint16_t dev_id, uint8_t *value)
{
(void)dev_id;

*value = EMU_RPMB_REL_WR_SEC_C;
return TEEC_SUCCESS;
}

static bool remap_rpmb_dev_id(uint16_t dev_id, uint16_t *ndev_id)
static bool remap_rpmb_dev_id_emu(uint16_t dev_id, uint16_t *ndev_id)
{
*ndev_id = dev_id;
return true;
}

#endif /* RPMB_EMU */

static inline void set_mmc_io_cmd(struct mmc_ioc_cmd *cmd, unsigned int blocks,
__u32 opcode, int write_flag)
{
Expand Down Expand Up @@ -890,7 +878,7 @@ static uint32_t rpmb_data_req(int fd, struct rpmb_data_frame *req_frm,
goto out;
}

st = IOCTL(fd, MMC_IOC_MULTI_CMD, mcmd);
st = rpmb_ops.ioctl(fd, MMC_IOC_MULTI_CMD, mcmd);
if (st < 0)
res = TEEC_ERROR_GENERIC;

Expand All @@ -906,16 +894,16 @@ static uint32_t rpmb_get_dev_info(uint16_t dev_id, struct rpmb_dev_info *info)
uint8_t rpmb_size_mult = 0;
uint8_t rel_wr_sec_c = 0;

res = read_cid(dev_id, info->cid);
res = rpmb_ops.read_cid(dev_id, info->cid);
if (res != TEEC_SUCCESS)
return res;

res = read_size_mult(dev_id, &rpmb_size_mult);
res = rpmb_ops.read_size_mult(dev_id, &rpmb_size_mult);
if (res != TEEC_SUCCESS)
return res;
info->rpmb_size_mult = rpmb_size_mult;

res = read_rel_wr_sec_c(dev_id, &rel_wr_sec_c);
res = rpmb_ops.read_rel_wr_sec_c(dev_id, &rel_wr_sec_c);
if (res != TEEC_SUCCESS)
return res;
info->rel_wr_sec_c = rel_wr_sec_c;
Expand Down Expand Up @@ -943,14 +931,14 @@ static uint32_t rpmb_process_request_unlocked(void *req, size_t req_size,
if (req_size < sizeof(*sreq))
return TEEC_ERROR_BAD_PARAMETERS;

if (!remap_rpmb_dev_id(sreq->dev_id, &dev_id))
if (!rpmb_ops.remap_dev_id(sreq->dev_id, &dev_id))
return TEEC_ERROR_ITEM_NOT_FOUND;

switch (sreq->cmd) {
case RPMB_CMD_DATA_REQ:
req_nfrm = (req_size - sizeof(struct rpmb_req)) / 512;
rsp_nfrm = rsp_size / 512;
fd = mmc_rpmb_fd(dev_id);
fd = rpmb_ops.get_fd(dev_id);
if (fd < 0)
return TEEC_ERROR_BAD_PARAMETERS;
res = rpmb_data_req(fd, RPMB_REQ_DATA(req), req_nfrm, rsp,
Expand Down Expand Up @@ -987,3 +975,31 @@ uint32_t rpmb_process_request(void *req, size_t req_size, void *rsp,

return res;
}


int rpmb_init()
{
if (!supplicant_params.rpmb_emu_disable) {
rpmb_emu = calloc(1, sizeof(*rpmb_emu));
if (!rpmb_emu) {
EMSG("Failed to allocate buffer for RPMBi emulation");
return -ENOMEM;
}
rpmb_emu->size = EMU_RPMB_SIZE_BYTES;
rpmb_ops.ioctl = ioctl_emu;
rpmb_ops.get_fd = mmc_rpmb_fd_emu;
rpmb_ops.read_cid = read_cid_emu;
rpmb_ops.read_size_mult = read_size_mult_emu;
rpmb_ops.read_rel_wr_sec_c = read_rel_wr_sec_c_emu;
rpmb_ops.remap_dev_id = remap_rpmb_dev_id_emu;
} else {
rpmb_ops.ioctl = ioctl;
rpmb_ops.get_fd = mmc_rpmb_fd;
rpmb_ops.read_cid = read_cid;
rpmb_ops.read_size_mult = read_size_mult;
rpmb_ops.read_rel_wr_sec_c = read_rel_wr_sec_c;
rpmb_ops.remap_dev_id = remap_rpmb_dev_id;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC, but I also didn't address this in my version, emulation comes with a rather large static buffer. Maybe that should now be allocated dynamically and only if emulation is actually used.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

emulation

The 'emulation' itself seems useful (at least for our case). It's used to test the RPMB related functionalities before switching to real RPMB, which needs to program the one-time permanent RPMB keys.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, but you also don't ship debug symbols or allocate debug trace buffers statically when you operate in production mode. I'm just asking for reducing the overhead by a potentially unused mode during runtime.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it. I misunderstood the top comment. Will update it and post a new version.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated (in latest 21bad5b).


return 0;
}
2 changes: 2 additions & 0 deletions tee-supplicant/src/rpmb.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,4 +34,6 @@
uint32_t rpmb_process_request(void *req, size_t req_size, void *rsp,
size_t rsp_size);

int rpmb_init(void);

#endif /* RPMB_H */
9 changes: 8 additions & 1 deletion tee-supplicant/src/tee_supplicant.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -493,6 +493,7 @@ static int usage(int status)
fprintf(stderr, "\t-h, --help: this help\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-e, --rpmb-emu-disable: RPMB emunation disable\n");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems inverted: Emulation is the corner case, not the common one. So we likely rather want -e having the semantic of --rpmb-emulation.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I would prefer the '--rpmb-emulation' idea as well. The problem is with the Linux distros we worked with requested to preserve the current default behavior, which means that "./tee-supplicant" should behave like RPMB EMU enabled. Thus adding the optional argument '--rpmb-emu-disable' to disable it.

Or any suggestions?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those distros could be given such a switch to keep the old behavior. In newer major distro releases, this can then be adjusted to something more useful than emulation only or emulation by default.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But I can't imagine any stable distro picking up this change anyway. It's clearly aiming at new major releases (eg. Debian trixie).

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually we're working with Canonical support to add the real RPMB support (with this patch) into their existing LTS release. They're holding it 'pending the adoption of the pull request'.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi - I'm working with @lsun100 to see if there's a way to enable the non-emu version in Ubuntu, as a post-release update. With respect to whether or not -e enables or disables emulation, I wonder if we could have both --rpmb-emu-disable and --rpmb-emu-enable arguments that allow the user to express their intent, with a configurable compile-time default. Then, theoretically, we could add --rpmb-emu-disable support leaving the default as "enabled", avoiding a change in behavior to existing users. The default could later change in a future feature release, but users who are passing one of these two flags could continue to have it honored.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dannf , how about @jan-kiszka 's proposal to use "--rpmb-emulation" only. That's to say, use real RPMB by default and specify "--rpmb-emulation" when needed for emulation. In ubuntu 'debian/tee-supplicant.service' file, we could this argument like "ExecStart=/usr/sbin/tee-supplicant --rpmb-emulation"?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a possibility that we have users using the tee-supplicant binary directly, or have implemented their own override service. Pushing out an update that requires a new argument to restore existing behavior would break those users.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But emulation is a testing toy, not the real purpose of tee-supplicant. I would suggest to give those users the chance to restore the behavior at build-time but avoid bothering the rest.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jan-kiszka leaving it as a build-time option may very well be what is best for the upstream project - I'm not qualified to comment on that. My commentary is really just about the impact the chosen implementation would have on the feasibility of including those changes in a post-release update to an existing Ubuntu release. These objectives may end up being incompatible, in which case Ubuntu users will likely need to wait for a future release for a non-emulated support.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typo - s/emunation/emulation/

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 @@ -808,14 +809,15 @@ int main(int argc, char *argv[])
/* long name | has argument | flag | short value */
{ "help", no_argument, 0, 'h' },
{ "daemonize", no_argument, 0, 'd' },
{ "rpmb-emu-disable", no_argument, 0, 'e' },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add also --rpmb-emu-enable / -E?

{ "fs-parent-path", required_argument, 0, 'f' },
{ "ta-dir", required_argument, 0, 't' },
{ "plugin-path", required_argument, 0, 'p' },
{ "rpmb-cid", required_argument, 0, 'r' },
{ 0, 0, 0, 0 }
};

while ((opt = getopt_long(argc, argv, "hdf:t:p:r:",
while ((opt = getopt_long(argc, argv, "hde:f:t:p:r:",
long_options, &long_index )) != -1) {
switch (opt) {
case 'h' :
Expand All @@ -824,6 +826,9 @@ int main(int argc, char *argv[])
case 'd':
daemonize = true;
break;
case 'e':
supplicant_params.rpmb_emu_disable = true;
break;
case 'f':
supplicant_params.fs_parent_path = optarg;
break;
Expand Down Expand Up @@ -895,6 +900,8 @@ int main(int argc, char *argv[])
close(pipefd[1]);
}

rpmb_init();

while (!arg.abort) {
if (!process_one_request(&arg))
arg.abort = true;
Expand Down
1 change: 1 addition & 0 deletions tee-supplicant/src/tee_supplicant.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@ struct tee_supplicant_params {
const char *plugin_load_path;
const char *fs_parent_path;
const char *rpmb_cid;
bool rpmb_emu_disable;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better be called in a positive statement: bool rpmb_emu_enable

};

extern struct tee_supplicant_params supplicant_params;
Expand Down
4 changes: 0 additions & 4 deletions tee-supplicant/tee_supplicant_android.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,11 +30,7 @@ LOCAL_SRC_FILES += src/tee_socket.c
LOCAL_CFLAGS += -DCFG_GP_SOCKETS=1
endif

RPMB_EMU ?= 1
ifeq ($(RPMB_EMU),1)
LOCAL_SRC_FILES += src/sha2.c src/hmac_sha2.c
LOCAL_CFLAGS += -DRPMB_EMU=1
endif

ifneq (,$(filter y,$(CFG_TA_GPROF_SUPPORT) $(CFG_FTRACE_SUPPORT)))
LOCAL_SRC_FILES += src/prof.c
Expand Down