There is a potential race condition in amdtee_close_session that may
cause use-after-free in amdtee_open_session. For instance, if a session
has refcount == 1, and one thread tries to free this session via:
kref_put(&sess->refcount, destroy_session);
the reference count will get decremented, and the next step would be to
call destroy_session(). However, if in another thread,
amdtee_open_session() is called before destroy_session() has completed
execution, alloc_session() may return 'sess' that will be freed up
later in destroy_session() leading to use-after-free in
amdtee_open_session.
To fix this issue, treat decrement of sess->refcount and invocation of
destroy_session() as a single critical section, so that it is executed
atomically.
Fixes: 757cc3e9ff1d ("tee: add AMD-TEE driver")
Cc: stable(a)vger.kernel.org
Signed-off-by: Rijo Thomas <Rijo-john.Thomas(a)amd.com>
---
drivers/tee/amdtee/core.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/tee/amdtee/core.c b/drivers/tee/amdtee/core.c
index 372d64756ed6..04cee03bec9d 100644
--- a/drivers/tee/amdtee/core.c
+++ b/drivers/tee/amdtee/core.c
@@ -217,14 +217,13 @@ static int copy_ta_binary(struct tee_context *ctx, void *ptr, void **ta,
return rc;
}
+/* mutex must be held by caller */
static void destroy_session(struct kref *ref)
{
struct amdtee_session *sess = container_of(ref, struct amdtee_session,
refcount);
- mutex_lock(&session_list_mutex);
list_del(&sess->list_node);
- mutex_unlock(&session_list_mutex);
kfree(sess);
}
@@ -272,7 +271,9 @@ int amdtee_open_session(struct tee_context *ctx,
if (arg->ret != TEEC_SUCCESS) {
pr_err("open_session failed %d\n", arg->ret);
handle_unload_ta(ta_handle);
+ mutex_lock(&session_list_mutex);
kref_put(&sess->refcount, destroy_session);
+ mutex_unlock(&session_list_mutex);
goto out;
}
@@ -290,7 +291,9 @@ int amdtee_open_session(struct tee_context *ctx,
pr_err("reached maximum session count %d\n", TEE_NUM_SESSIONS);
handle_close_session(ta_handle, session_info);
handle_unload_ta(ta_handle);
+ mutex_lock(&session_list_mutex);
kref_put(&sess->refcount, destroy_session);
+ mutex_unlock(&session_list_mutex);
rc = -ENOMEM;
goto out;
}
@@ -331,7 +334,9 @@ int amdtee_close_session(struct tee_context *ctx, u32 session)
handle_close_session(ta_handle, session_info);
handle_unload_ta(ta_handle);
+ mutex_lock(&session_list_mutex);
kref_put(&sess->refcount, destroy_session);
+ mutex_unlock(&session_list_mutex);
return 0;
}
--
2.25.1
+cc OP-TEE ML
On Tue, 26 Sept 2023 at 13:47, Rijo Thomas <Rijo-john.Thomas(a)amd.com> wrote:
>
> On 9/26/2023 1:19 PM, Sumit Garg wrote:
> > On Tue, 26 Sept 2023 at 12:53, Rijo Thomas <Rijo-john.Thomas(a)amd.com> wrote:
> >>
> >> On 9/26/2023 12:14 PM, Sumit Garg wrote:
> >>> +cc Alex
> >>>
> >>> On Tue, 26 Sept 2023 at 08:16, Jens Wiklander <jens.wiklander(a)linaro.org> wrote:
> >>>>
> >>>> Hi,
> >>>>
> >>>> [+cc Arnd]
> >>>>
> >>>> On Tue, Sep 26, 2023 at 8:00 AM Sumit Garg <sumit.garg(a)linaro.org> wrote:
> >>>>>
> >>>>> +cc Jens
> >>>>>
> >>>>>> In a virtual environment, an application running in guest VM may want
> >>>>>> to delegate security sensitive tasks to a Trusted Application (TA)
> >>>>>> running within a Trusted Execution Environment (TEE). A TEE is a trusted
> >>>>>> OS running in some secure environment, for example, TrustZone on ARM
> >>>>>> CPUs, or a separate secure co-processor etc.
> >>>>>
> >>>>> I have been exploring this area quite recently with an effort to have a common VIRIO interface which can support different trusted OS implementations. I guess you intend to test it with AMD-TEE, right? Any plans to test it with OP-TEE? As currently we have these two supported upstream.
> >>>>>
> >> Yes, we have tested with AMD-TEE. We have not yet tested with OP-TEE. Sure, we will try it out.
> >
> > Glad to hear that. I can help get it tested with OP-TEE as well.
> >
>
> We will test it out internally. Shall let you know in case we need help.
>
> >>
> >>>>> Do you currently have any virtio frontend/backend implementations for this?
> >>>>>
> >>
> >> Yes, we have. Frontend is a Linux virtio-TEE driver, and backend is virtio-TEE device emulated in QEMU.
> >> We used the Xen hypervisor.
> >
> > Can you share corresponding references? I can give it a try using Qemu with KVM.
> >
>
> We will share it in next couple of weeks. We have not yet hosted the code for external consumption.
>
> >>
> >>>>>>
> >>>>>> A virtual TEE device emulates a TEE within a guest VM. Such a virtual
> >>>>>> TEE device supports multiple operations such as:
> >>>>>>
> >>>>>> VIRTIO_TEE_CMD_OPEN_DEVICE – Open a communication channel with virtio
> >>>>>> TEE device.
> >>>>>> VIRTIO_TEE_CMD_CLOSE_DEVICE – Close communication channel with virtio
> >>>>>> TEE device.
> >>>>>> VIRTIO_TEE_CMD_GET_VERSION – Get version of virtio TEE.
> >>>>>> VIRTIO_TEE_CMD_OPEN_SESSION – Open a session to communicate with
> >>>>>> trusted application running in TEE.
> >>>>>> VIRTIO_TEE_CMD_CLOSE_SESSION – Close a session to end communication
> >>>>>> with trusted application running in TEE.
> >>>>>> VIRTIO_TEE_CMD_INVOKE_FUNC – Invoke a command or function in trusted
> >>>>>> application running in TEE.
> >>>>>> VIRTIO_TEE_CMD_CANCEL_REQ – Cancel an ongoing command within TEE.
> >>>>>>
> >>>>>
> >>>>> How about shared memory support? We would like to register guest pages with the trusted OS.
> >> We have a command VIRTIO_TEE_CMD_REGISTER_MEM for registering shared memory buffer with Trusted OS.
> >
> > I suppose the commit message has to be appended then. Do you have the
> > draft virtio-tee device specification ready for review? I would be
> > interested to review that.
> >
>
> Yes, the command is missed out in the commit message.
With the commit message updated to include support for shared memory,
feel free to add:
Acked-by: Sumit Garg <sumit.garg(a)linaro.org>
>
> We are in the process of preparing virtio-tee device specification. We will be sending it out to this
> list.
I would also suggest you to CC: op-tee(a)lists.trustedfirmware.org for review.
>
> >>
> >> In this command, the guest pages are copied into a shadow buffer in the host OS. And this shadow
> >> buffer is mapped with Trusted OS. So, buffer-copy is involved.
> >>
> >> One limitation, that we had was that the guest pages were non-contiguous. So, the number of physical
> >> pages that had to be mapped with Trusted OS was exceeding 64 entries when we were testing out the
> >> registering of guest pages. AMD-TEE Trusted OS can map a physically non-contiguous buffer, but the
> >> number of sg entries for such a buffer must be less than 64. So, we resorted to using a shadow buffer
> >> that is allocated within host, and gets mapped with Trusted OS.
> >
> > I don't think OP-TEE OS has such a limitation on non-contiguous pages.
> > So I would suggest you to keep VIRTIO_TEE_CMD_REGISTER_MEM as part of
> > the ABI. It can be an optional feature for a particular trusted OS
> > implementation to support.
> >
>
> Currently, the reg_mem (register memory) control is dictated by a flag in virtio-tee qemu code. This flag
> for our testing was hard-coded as false. We will enhance our code, so that it is configurable. The value
> of reg_mem shall be set to true/false depending upon whether the underlying TEE driver reports TEE_GEN_CAP_REG_MEM.
Sounds good to me.
-Sumit
>
> Thanks,
> Rijo
>
> > -Sumit
> >
> >>
> >> Thanks,
> >> Rijo
> >>
> >>>>
> >>>> Coincidently Arnd and I (among others) discussed this in person last
> >>>> week and the conclusion was that only temporary shared memory is
> >>>> possible with virtio. So the shared memory has to be set up and torn
> >>>> down by the host during each operation, typically open-session or
> >>>> invoke-func.
> >>>
> >>> Agree as I was part of those discussions. But I would like to
> >>> understand the reasoning behind it. Is there any restriction by VIRTIO
> >>> specification that we can't register guest page PAs to a device (TEE
> >>> in our case) to allow for zero copy transfers?
> >>>
> >>> Alex mentioned some references to virtio GPU device. I suppose I need
> >>> to dive into its implementation to see if there are any similarities
> >>> to our use-case.
> >>>
> >>>> That might not be optimal if trying to maximize
> >>>> performance, but it is portable.
> >>>
> >>> IMO, the ABI should be flexible enough to support a TEE with optimum
> >>> performance.
> >>>
> >>> -Sumit
> >>>
> >>>>
> >>>> Cheers,
> >>>> Jens
> >>>>
> >>>>>
> >>>>> -Sumit
> >>>>>
> >>>>>> We would like to reserve device ID 46 for Virtio-TEE device.
> >>>>>>
> >>>>>> Signed-off-by: Jeshwanth Kumar <jeshwanthkumar.nk(a)amd.com>
> >>>>>> ---
> >>>>>> content.tex | 2 ++
> >>>>>> 1 file changed, 2 insertions(+)
> >>>>>>
> >>>>>> diff --git a/content.tex b/content.tex
> >>>>>> index 0a62dce..644aa4a 100644
> >>>>>> --- a/content.tex
> >>>>>> +++ b/content.tex
> >>>>>> @@ -739,6 +739,8 @@ \chapter{Device Types}\label{sec:Device Types}
> >>>>>> \hline
> >>>>>> 45 & SPI master \\
> >>>>>> \hline
> >>>>>> +46 & TEE device \\
> >>>>>> +\hline
> >>>>>> \end{tabular}
> >>>>>>
> >>>>>> Some of the devices above are unspecified by this document,
Hi
Today Tuesday, September 26 it's time for another LOC monthly meeting.
Sorry for the short notice. For
time and connection details see the calendar at
https://www.trustedfirmware.org/meetings/
I have a few items for the agenda:
- Firmware handoff in OP-TEE https://github.com/OP-TEE/optee_os/pull/6308
- We've started to upstream a few PRs that will require bumping the
major version to 4
Any other topics?
Thanks,
Jens
Hi,
For legislative purposes, one of our Trusted Applications needs to show a
checksum to the end-user on our secure screen, to allow verification.
Albeit a bit unconventional use of it, we thought that the TA's own
checksum would be ideal for that, but we're having difficulties figuring
how to access that, if at all possible.
So I have to turn to you guys here: Is there any way for a TA to access its
own checksum? And if yes, could somebody please give me some pointers on
how to do this?
With kind regards,
Robert.
--
DISCLAIMER
De informatie, verzonden in of met dit e-mailbericht, is
vertrouwelijk en uitsluitend voor de geadresseerde(n) bestemd. Het gebruik
van de informatie in dit bericht, de openbaarmaking, vermenigvuldiging,
verspreiding en|of verstrekking daarvan aan derden is niet toegestaan.
Gebruik van deze informatie door anderen dan geadresseerde(n) is strikt
verboden. Aan deze informatie kunnen geen rechten worden ontleend. U wordt
verzocht bij onjuiste adressering de afzender direct te informeren door het
bericht te retourneren en het bericht uit uw computersysteem te verwijderen.
Hello arm-soc maintainers,
Please pull this small fix that removes a few unused function declarations
in the TEE subsystem.
Thanks,
Jens
The following changes since commit 2dde18cd1d8fac735875f2e4987f11817cc0bc2c:
Linux 6.5 (2023-08-27 14:49:51 -0700)
are available in the Git repository at:
https://git.linaro.org/people/jens.wiklander/linux-tee.git/ tags/optee-for-for-v6.6
for you to fetch changes up to 069969d6c5264d2348fd6cf0cedc00fd87ff3cee:
tee: Remove unused declarations (2023-09-13 08:16:24 +0200)
----------------------------------------------------------------
Remove a few unused declarations in TEE subsystem
----------------------------------------------------------------
Yue Haibing (1):
tee: Remove unused declarations
drivers/tee/optee/optee_private.h | 2 --
drivers/tee/tee_private.h | 2 --
2 files changed, 4 deletions(-)
Hi All,
Note you may have received another instance of this note but when I
attempted to send to all TF ML's simultaneously it seemed to fail, so
sending to each one at a time. Sorry about that. :/
We've created a Discord Server for real time chats/sharing. This solution
comes at no cost to the project, is set up with channels for each project,
includes a #general channel, and supports direct 1-1 chats between members,
all with the goal of improving collaboration between trustedfirmware.org
developers.
We encourage all to join! :) Instructions for joining can be found on
the TF.org
FAQ page <https://www.trustedfirmware.org/faq/>.
See you all there and please don't hesitate to reach out if you have any
questions!
Don Harbin
TrustedFirmware Community Manager
don.harbin(a)linaro.org
Hi All,
To all TF maillists in case all aren't yet aware.
We've created a Discord Server for real time chats/sharing. This solution
comes at no cost to the project, is set up with channels for each project,
includes a #general channel, and supports direct 1-1 chats between members,
all with the goal of improving collaboration between trustedfirmware.org
developers.
I've attached a recent screenshot from the #general channel as a sample of
the interface and usages.
We encourage all to join! :) Instructions for joining can be found on
the TF.org
FAQ page <https://www.trustedfirmware.org/faq/>.
See you all there and please don't hesitate to reach out if you have any
questions!
Don Harbin
TrustedFirmware Community Manager
don.harbin(a)linaro.org
Hello all,
We are started using Op-tee in our project.
Since we are new to Op-tee, could someone please confirm whether anyone has already tried below in their project or is it possible to use along with OpenSSH/OpenSSL ?
What we try to accomplish is: application/sshd -> openssl (libcrypto/provider) -> Optee (client/TA) -> (HW or SW cipher algorithm) for data encryption/decryption.
Thanks,
Hareesh
