core: storage: add some overflow checks

Adds overflow checks to the secure storage code.

Signed-off-by: Bastien Simondi <bsimondi@netflix.com>
[jf: Fix test in syscall_storage_obj_seek() case TEE_D

core: storage: add some overflow checks

Adds overflow checks to the secure storage code.

Signed-off-by: Bastien Simondi <bsimondi@netflix.com>
[jf: Fix test in syscall_storage_obj_seek() case TEE_DATA_SEEK_END]
[jf: Get rid of { } block, initialize new local variables]
[jf: Do not fail on (data && !len) in syscall_storage_obj_create()]
Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
Reviewed-by: Joakim Bech <joakim.bech@linaro.org>

show more ...

core: check allocated size of temporary secure memory

When servicing syscall_invoke_ta_command(), the invoked TA could modify
the .size field. Make sure the allocated buffer is not overwritten on
re

core: check allocated size of temporary secure memory

When servicing syscall_invoke_ta_command(), the invoked TA could modify
the .size field. Make sure the allocated buffer is not overwritten on
return.

Signed-off-by: Bastien Simondi <bsimondi@netflix.com>
[jf: fix multi-line comment, replace '= { 0 };' with '= { };']
[jf: add commit description]
Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
Reviewed-by: Joakim Bech <joakim.bech@linaro.org>

show more ...

core: FS: wipe sensitive data after use

The secure storage code makes use of various cryptographic data (keys
and IVs). Make sure the buffers are wiped after use to minimize the
risks that sensitive

core: FS: wipe sensitive data after use

The secure storage code makes use of various cryptographic data (keys
and IVs). Make sure the buffers are wiped after use to minimize the
risks that sensitive data may be leaked to an attacker who would have
gained some access to the secure memory.

Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
Reported-by: Bastien Simondi <bsimondi@netflix.com>
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
Reviewed-by: Joakim Bech <joakim.bech@linaro.org>

show more ...

core: scrub user-tainted memory returned by alloc_temp_sec_mem()

This is a security fix for TA-to-TA calls.

In syscall_open_ta_session() and syscall_invoke_ta_command(), caller TA
can reference som

core: scrub user-tainted memory returned by alloc_temp_sec_mem()

This is a security fix for TA-to-TA calls.

In syscall_open_ta_session() and syscall_invoke_ta_command(), caller TA
can reference some private memory, in which case the kernel makes a
temporary copy. Unfortunately, memory allocated through
alloc_temp_sec_mem() is not cleared when returned. One could leverage
this to copy arbitrary data into this secure memory pool or to snoop
former data from a previous call done by another TA (e.g., using
TEE_PARAM_TYPE_MEMREF_OUTPUT allows to map the data while not overwriting
it, hence accessing to what is already there).

This patch introduces mobj_free_wipe() to clear and free an mobj.

Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
Reported-by: Bastien Simondi <bsimondi@netflix.com> [1.5]
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
Reviewed-by: Joakim Bech <joakim.bech@linaro.org>

show more ...

core: scrub user-tainted kernel heap memory before freeing it

Some syscalls can be used to poison kernel heap memory. Data copied from
userland is not wiped when the syscall returns. For instance, w

core: scrub user-tainted kernel heap memory before freeing it

Some syscalls can be used to poison kernel heap memory. Data copied from
userland is not wiped when the syscall returns. For instance, when doing
syscall_log() one can copy arbitrary data of variable length onto kernel
memory. When free() is called, the block is returned to the memory pool,
tainted with that userland data. This might be used in combination with
some other vulnerability to produce an exploit.

This patch uses free_wipe() to clear the buffers that have been used to
store user-provided data before returning them to the heap.

Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
Reported-by: Bastien Simondi <bsimondi@netflix.com> [1.4]
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
Reviewed-by: Joakim Bech <joakim.bech@linaro.org>
Acked-by: Etienne Carriere <etienne.carriere@linaro.org>

show more ...

core: add get_tag() to struct user_ta_store_ops

Adds get_tag() method to struct user_ta_store_ops.

Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
Signed-off-by: Jens Wiklander <jens.wi

core: add get_tag() to struct user_ta_store_ops

Adds get_tag() method to struct user_ta_store_ops.

Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

show more ...

core: add tee_tadb_get_tag()

Adds the function tee_tadb_get_tag() which returns a tag that uniquely
identifies a TA.

Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
Signed-off-by: Jens

core: add tee_tadb_get_tag()

Adds the function tee_tadb_get_tag() which returns a tag that uniquely
identifies a TA.

Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

show more ...

core: remove flags argument from tee_pager_alloc()

Removes the flags argument from tee_pager_alloc() since it's only used
with TEE_MATTR_LOCKED. The exception is the bignum pool, but since it
still

core: remove flags argument from tee_pager_alloc()

Removes the flags argument from tee_pager_alloc() since it's only used
with TEE_MATTR_LOCKED. The exception is the bignum pool, but since it
still releases all locked pages each time the pool becomes unused it's
efficient usage of memory.

Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

show more ...

core: keep alive TA context can be created after TA has panicked

When a keep alive TA instance panics, it continues to exist and
blocks all further use of the TA until the next reboot of the system.

core: keep alive TA context can be created after TA has panicked

When a keep alive TA instance panics, it continues to exist and
blocks all further use of the TA until the next reboot of the system.
Moreover, when a new session is trying to be created for
the panicked TA (while another session to that TA is still opened),
the system hangs.

This change releases panicked TA context and clears all references to
the released context when the TA panics regardless the TA properties.
This allows keep alive TA instances to be created back after they have
panicked without needing to reboot OP-TEE core.

Sessions on panicked TAs have to be closed by the client by calling
the proper API when session client is scheduled back.

Signed-off-by: Ovidiu Mihalachi <ovidiu_mihalachi@mentor.com>
Reviewed-by: Etienne Carriere <etienne.carriere@linaro.org>
Acked-by: Jens Wiklander <jens.wiklander@linaro.org>

show more ...

core: derive RPMB key using huk_subkey_derive()

tee_rpmb_key_gen() uses huk_subkey_derive() to derive the RPMB instead
of MAC:ing etc directly.

Note that this is only backwards compatible if
CFG_CO

core: derive RPMB key using huk_subkey_derive()

tee_rpmb_key_gen() uses huk_subkey_derive() to derive the RPMB instead
of MAC:ing etc directly.

Note that this is only backwards compatible if
CFG_CORE_HUK_SUBKEY_COMPAT=y.

Reviewed-by: Joakim Bech <joakim.bech@linaro.org>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

show more ...

core: derive SSK using huk_subkey_derive()

tee_fs_init_key_manager() uses huk_subkey_derive() to derive the SSK
instead of MAC:ing etc directly.

Note that this is only backwards compatible if
CFG_C

core: derive SSK using huk_subkey_derive()

tee_fs_init_key_manager() uses huk_subkey_derive() to derive the SSK
instead of MAC:ing etc directly.

Note that this is only backwards compatible if
CFG_CORE_HUK_SUBKEY_COMPAT=y.

Reviewed-by: Joakim Bech <joakim.bech@linaro.org>
Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>

show more ...

core: storage: set data length after truncation

After truncating a persistent object, update dataSize in the
corresponding TEE_ObjectInfo structure.

Signed-off-by: Christopher Tam <godtamit@google.

core: storage: set data length after truncation

After truncating a persistent object, update dataSize in the
corresponding TEE_ObjectInfo structure.

Signed-off-by: Christopher Tam <godtamit@google.com>
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
Tested-by: Jerome Forissier <jerome.forissier@linaro.org> (QEMU, GP)

show more ...

core: Initialize the chip_id array when generating the SSK

In tee_fs_init_key_manager(), Secure Storage Key (SSK) is computed as
follow:

SSK = HMAC(HUK, message)
message := concatenate(chip

core: Initialize the chip_id array when generating the SSK

In tee_fs_init_key_manager(), Secure Storage Key (SSK) is computed as
follow:

SSK = HMAC(HUK, message)
message := concatenate(chip_id, static string)

chip_id is a 32-byte array but some tee_otp_get_die_id() implementation
may provide a smaller chip ID. Initialize the chip_id array to make
sure the remaining bytes do not contain garbage data. Without this
initialization, SSK may be inconsistent across power cycles generating
failures when reading back data from the secure storage.

Signed-off-by: Alexandre Jutras <alexandre.jutras@linaro.org>
Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>

show more ...

core: cleanup generic traces

Remove useless newline character in few generic debug traces.

Remove argument __func__ from a FMSG trace since already
output by macro FMSG().

Remove error trace from

core: cleanup generic traces

Remove useless newline character in few generic debug traces.

Remove argument __func__ from a FMSG trace since already
output by macro FMSG().

Remove error trace from syscall_storage_obj_read() that, prior
this change, output failing error code from storage read()
handler. This is useless and not done for other storage handlers
return code.

Signed-off-by: Etienne Carriere <etienne.carriere@linaro.org>
Reviewed-by: Jerome Forissier <jerome.forissier@linaro.org>

show more ...

libutils: remove buf_compare_ct()

Now that we have consttime_memcmp(), buf_compare_ct() is redundant.
Every time buf_compare_ct() is used, consttime_memcmp() may be used
instead.

This commit remove

libutils: remove buf_compare_ct()

Now that we have consttime_memcmp(), buf_compare_ct() is redundant.
Every time buf_compare_ct() is used, consttime_memcmp() may be used
instead.

This commit removes buf_compare_ct(). A compatibility wrapper is kept
in <string_ext.h> to avoid knowingly breaking the build of any TA that
may use it.

Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>

show more ...

core: RPMB FS: check for potential overflows

This commit deals with a number of potential integer overflows in the
RPMB FS code.

rpmb_fs_init() requests device information from the REE. The RPMB si

core: RPMB FS: check for potential overflows

This commit deals with a number of potential integer overflows in the
RPMB FS code.

rpmb_fs_init() requests device information from the REE. The RPMB size
is returned in struct rpmb_dev_info (field rpmb_size_mult) and is used
in a multiplication that could overflow. Use MUL_OVERFLOW() to deal with
this case.

Some overflow checks are also added in the read and write paths.

Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
Reported-by: Bastien Simondi <bsimondi@netflix.com> [2.12]
Reviewed-by: Joakim Bech <joakim.bech@linaro.org>

show more ...

core: syscall_authenc_init(): check nonce accessibility

syscall_authenc_init() does not check that the given nonce address is
within TA accessible memory. Fix that.

Signed-off-by: Jerome Forissier

core: syscall_authenc_init(): check nonce accessibility

syscall_authenc_init() does not check that the given nonce address is
within TA accessible memory. Fix that.

Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
Reported-by: Bastien Simondi <bsimondi@netflix.com> [2.10]
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
Reviewed-by: Joakim Bech <joakim.bech@linaro.org>

show more ...

core: crypto: add overflow check when copying attributes

In copy_in_attrs(), attr_count * sizeof(struct utee_attribute) could
overflow if a very large attr_count is given. Use MUL_OVERFLOW() to
prop

core: crypto: add overflow check when copying attributes

In copy_in_attrs(), attr_count * sizeof(struct utee_attribute) could
overflow if a very large attr_count is given. Use MUL_OVERFLOW() to
properly deal with this case.

Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
Reported-by: Bastien Simondi <bsimondi@netflix.com> [2.9]
Reviewed-by: Joakim Bech <joakim.bech@linaro.org>

show more ...

core: do not use virtual addresses as session identifier

Session context virtual address is returned to the REE in
entry_open_session(); it is then used back in entry_close_session() and
entry_invok

core: do not use virtual addresses as session identifier

Session context virtual address is returned to the REE in
entry_open_session(); it is then used back in entry_close_session() and
entry_invoke_command(). Sharing virtual addresses with the REE leads to
virtual memory addresses disclosure that could be leveraged to defeat
ASLR (if/when implemented) and/or mount an attack.

Similarly, syscall_open_ta_session() returns a session ID directly
derived from the session virtual address to the caller TA.

This commit introduces a 32-bit identifier field in struct tee_ta_session.
The ID is generated when the session is created, starting from the id of
the last session in the queue, and counting up until a number that is not
used in the session queue is found.

Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
Reported-by: Bastien Simondi <bsimondi@netflix.com> [2.1]
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
Reviewed-by: Joakim Bech <joakim.bech@linaro.org>

show more ...

core: check the value of tee_otp_get_die_id()

Just like the get_prop_tee_dev_id() in tee_svc.c, it returns
TEE_ERROR_BAD_STATE, when tee_otp_get_die_id() reports someting bad.
Put the same check in

core: check the value of tee_otp_get_die_id()

Just like the get_prop_tee_dev_id() in tee_svc.c, it returns
TEE_ERROR_BAD_STATE, when tee_otp_get_die_id() reports someting bad.
Put the same check in tee_fs_init_key_manager() as well.

Fixes: https://github.com/OP-TEE/optee_os/issues/2762
Signed-off-by: Oliver Chiang <rockerfeynman@gmail.com>
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
[jf: use URL in Fixes: tag]
Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>

show more ...

core: syscall_storage_obj_create(): fix a memory leak

Free the o->attr in the error handling part.

Fixes: https://github.com/OP-TEE/optee_os/issues/2738
Signed-off-by: Oliver Chiang <rockerfeynman@

core: syscall_storage_obj_create(): fix a memory leak

Free the o->attr in the error handling part.

Fixes: https://github.com/OP-TEE/optee_os/issues/2738
Signed-off-by: Oliver Chiang <rockerfeynman@gmail.com>
[jf: do not set o->attr = 0; move tee_obj_free(o) under if (o) { ... }]
[jf: add spaces to subject; use URL in Fixes: tag]
Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
Tested-by: Jerome Forissier <jerome.forissier@linaro.org> (QEMU)

show more ...

core: svc_cryp: fix truncated buffer length

Fixes truncated buffer length in multiple crypto syscalls. The buffer
length is truncated on 32-bit systems because a size_t can't hold a
uint64_t which i

core: svc_cryp: fix truncated buffer length

Fixes truncated buffer length in multiple crypto syscalls. The buffer
length is truncated on 32-bit systems because a size_t can't hold a
uint64_t which is use to carry the buffer length.

Fixes: "Truncated buffer length in crypto system calls (x4)" as reported
by Riscure.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Tested-by: Joakim Bech <joakim.bech@linaro.org> (QEMU v7, v8)
Reviewed-by: Joakim Bech <joakim.bech@linaro.org>
Reported-by: Riscure <inforequest@riscure.com>
Reported-by: Alyssa Milburn <a.a.milburn@vu.nl>
Acked-by: Etienne Carriere <etienne.carriere@linaro.org>

show more ...

core: svc: always check ta parameters

Always check TA parameters from a user TA. This prevents a user TA from
passing invalid pointers to a pseudo TA.

Fixes: OP-TEE-2018-0007: "Buffer checks missin

core: svc: always check ta parameters

Always check TA parameters from a user TA. This prevents a user TA from
passing invalid pointers to a pseudo TA.

Fixes: OP-TEE-2018-0007: "Buffer checks missing when calling pseudo
TAs".

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Tested-by: Joakim Bech <joakim.bech@linaro.org> (QEMU v7, v8)
Reviewed-by: Joakim Bech <joakim.bech@linaro.org>
Reported-by: Riscure <inforequest@riscure.com>
Reported-by: Alyssa Milburn <a.a.milburn@vu.nl>
Acked-by: Etienne Carriere <etienne.carriere@linaro.org>

show more ...

svc: Initialize tmp_va_buf to prevent a TOCTOU attack

tmp_va_buf will be used if caller parameters points to private TA
memory. However, after doing the syscall to invoke the command it could
be tha

svc: Initialize tmp_va_buf to prevent a TOCTOU attack

tmp_va_buf will be used if caller parameters points to private TA
memory. However, after doing the syscall to invoke the command it could
be that REE has changed caller parameters to point to regular shared
memory and that could potentially open for tmp_va_buf leaking old
information on the stack.

Mitigate this by simplify tee_svc_update_out_param() by only taking
tmp_buf_va[n] into account to tell if a temporary buffer is used or not.

Note that tee_svc_copy_to_user() will make sure that only data writeable
by the user TA can be updated.

Fixes: "Double fetch can be used to copy from uninitialized pointer" as
reported by Riscure.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Tested-by: Joakim Bech <joakim.bech@linaro.org> (QEMU v7, v8)
Reviewed-by: Joakim Bech <joakim.bech@linaro.org>
Reported-by: Riscure <inforequest@riscure.com>
Reported-by: Alyssa Milburn <a.a.milburn@vu.nl>
Acked-by: Etienne Carriere <etienne.carriere@linaro.org>

show more ...

svc: fix NULL pointer dereference during storage enumeration

In syscall_storage_next_enum(..) when 'tee_obj o' isn't successfully
initialized, then 'o->pobj->fops' is a NULL pointer and therefore we

svc: fix NULL pointer dereference during storage enumeration

In syscall_storage_next_enum(..) when 'tee_obj o' isn't successfully
initialized, then 'o->pobj->fops' is a NULL pointer and therefore we
need to check for that before trying to dereference it in the clean-up
part of the function.

Fixes: "Null pointer dereference in storage system call" as reported by
Riscure.

Signed-off-by: Joakim Bech <joakim.bech@linaro.org>
Tested-by: Joakim Bech <joakim.bech@linaro.org> (QEMU v7, v8)
Reviewed-by: Jens Wiklander <jens.wiklander@linaro.org>
Reported-by: Riscure <inforequest@riscure.com>
Reported-by: Alyssa Milburn <a.a.milburn@vu.nl>
Acked-by: Etienne Carriere <etienne.carriere@linaro.org>

show more ...