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 ...

svc: check for overflow when allocating a BigNum buffer

To avoid overflow errors and copy more data than being allocated we must
check for overflow when allocating a buffer for the bignum-buffer whi

svc: check for overflow when allocating a BigNum buffer

To avoid overflow errors and copy more data than being allocated we must
check for overflow when allocating a buffer for the bignum-buffer which
is 8 times larger than the binary buffer.

Fixes: "Integer overflow in crypto 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 ...

svc: avoid TOCTOU issue in syscall_hash_final

When checking that the supplied buffer is big enough to fit the computed
digest one should use the local copy 'hlen' instead of 'hash_len' to
prevent th

svc: avoid TOCTOU issue in syscall_hash_final

When checking that the supplied buffer is big enough to fit the computed
digest one should use the local copy 'hlen' instead of 'hash_len' to
prevent that a malicious attacker in REE have changed the size of
'hash_len' after it has been copied to the local buffer.

(TOCTOU: Time Of Check To Time of Use)

Fixes: "Double-fetch of length in syscall_hash_final (x2)" 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 ...

svc: check for allocation overflow in crypto calls part 2

Without checking for overflow there is a risk of allocating a buffer
with size smaller than anticipated and as a consequence of that it migh

svc: check for allocation overflow in crypto calls part 2

Without checking for overflow there is a risk of allocating a buffer
with size smaller than anticipated and as a consequence of that it might
lead to a heap based overflow with attacker controlled data written
outside the boundaries of the buffer.

Fixes: OP-TEE-2018-0011: "Integer overflow in crypto system calls (x2)"

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 ...

svc: check for allocation overflow in crypto calls

Without checking for overflow there is a risk of allocating a buffer
with size smaller than anticipated and as a consequence of that it might
lead

svc: check for allocation overflow in crypto calls

Without checking for overflow there is a risk of allocating a buffer
with size smaller than anticipated and as a consequence of that it might
lead to a heap based overflow with attacker controlled data written
outside the boundaries of the buffer.

Fixes: OP-TEE-2018-0010: "Integer overflow in crypto system calls (x2)"

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 ...

svc: check for allocation overflow in syscall_cryp_obj_populate

Without checking for overflow there is a risk of allocating a buffer
with size smaller than anticipated and as a consequence of that i

svc: check for allocation overflow in syscall_cryp_obj_populate

Without checking for overflow there is a risk of allocating a buffer
with size smaller than anticipated and as a consequence of that it might
lead to a heap based overflow with attacker controlled data written
outside the boundaries of the buffer.

Fixes: OP-TEE-2018-0009: "Integer overflow in crypto system calls"

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 ...

fs: prevent out of place write when no data

Fixes: "Uninitialized return value returned if len equals 0" as reported
by Riscure.

Signed-off-by: Jens Wiklander <jens.wiklander@linaro.org>
Tested-by:

fs: prevent out of place write when no data

Fixes: "Uninitialized return value returned if len equals 0" 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 ...

rpmb: check return value from essiv call

An error in the function essiv, as for example memory allocation failure
could result in an uninitialized IV, which means that the IV used for
en/decryption

rpmb: check return value from essiv call

An error in the function essiv, as for example memory allocation failure
could result in an uninitialized IV, which means that the IV used for
en/decryption would consist of data previously stored at this memory
location. This could eventually corrupt the filesystem.

Fixes: "Return value of cryptographic function is unchecked" 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 ...

core: syscall_storage_obj_rename(): fix handling of .rename() return status

Any error returned by fops->rename() should be reflected by
syscall_storage_obj_rename(). There is no reason why errors ot

core: syscall_storage_obj_rename(): fix handling of .rename() return status

Any error returned by fops->rename() should be reflected by
syscall_storage_obj_rename(). There is no reason why errors other than
TEE_ERROR_GENERIC should be ignored.

Fixes the following test case: create two persistent objects (o1 and o2),
close o1, rename o2 to the name of o1. TEE_RenamePersistentObject() should
return TEE_ERROR_ACCESS_CONFLICT, but TEE_SUCCESS is returned instead.

Fixes: https://github.com/OP-TEE/optee_os/issues/2707
Signed-off-by: Jerome Forissier <jerome.forissier@linaro.org>
Reported-by: Chao Liu <chao.liu@amlogic.com>
Reviewed-by: Joakim Bech <joakim.bech@linaro.org>
Acked-by: Jens Wiklander <jens.wiklander@linaro.org>

show more ...