tee: fix illegal pointer dereference in tee_shm_put()

[PATCH] tee: fix illegal pointer dereference in tee_shm_put()

Posted by yangzhao 1 month, 1 week ago

In tee_shm_put(), there is not only the NULL pointer dereference,
but also the illegal pointer dereference.

shutdown() --->
    __optee_disable_shm_cache -->
        shm = reg_pair_to_ptr(...);
        tee_shm_free(shm); -->
            tee_shm_put(shm); //crash: shm->ctx maybe NULL pointer or illegal pointer

Check whether the pointer is NULL and whether the pointer address is valid.

This issue occurs when rich world uses the 6.x version of the kernel and
optee secure world uses a lower version (such as version 3.2), and it is
highly likely to trigger a kernel panic when conducting hibernate tests.

Fixes: e4a718a3a47e ("tee: fix NULL pointer dereference in tee_shm_put")
Signed-off-by: yangzhao <yangzhao@kylinos.cn>
---
 drivers/tee/tee_shm.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
index 4a47de4bb2e5..de01d16409c1 100644
--- a/drivers/tee/tee_shm.c
+++ b/drivers/tee/tee_shm.c
@@ -722,7 +722,14 @@ void tee_shm_put(struct tee_shm *shm)
 	struct tee_device *teedev;
 	bool do_release = false;
 
-	if (!shm || !shm->ctx || !shm->ctx->teedev)
+	/* checking pointer */
+	if (IS_ERR_OR_NULL(shm) || !virt_addr_valid(shm))
+		return;
+
+	if (IS_ERR_OR_NULL(shm->ctx) || !virt_addr_valid(shm->ctx))
+		return;
+
+	if (IS_ERR_OR_NULL(shm->ctx->teedev) || !virt_addr_valid(shm->ctx->teedev))
 		return;
 
 	teedev = shm->ctx->teedev;
-- 
2.25.1

Re: [PATCH] tee: fix illegal pointer dereference in tee_shm_put()

Posted by Jens Wiklander 1 month, 1 week ago

Hi,

On Thu, Nov 6, 2025 at 11:20 AM yangzhao <yangzhao@kylinos.cn> wrote:
>
> In tee_shm_put(), there is not only the NULL pointer dereference,
> but also the illegal pointer dereference.
>
> shutdown() --->
>     __optee_disable_shm_cache -->
>         shm = reg_pair_to_ptr(...);
>         tee_shm_free(shm); -->
>             tee_shm_put(shm); //crash: shm->ctx maybe NULL pointer or illegal pointer
>
> Check whether the pointer is NULL and whether the pointer address is valid.
>
> This issue occurs when rich world uses the 6.x version of the kernel and
> optee secure world uses a lower version (such as version 3.2), and it is
> highly likely to trigger a kernel panic when conducting hibernate tests.

It sounds like the root of the problem is in the TEE rather than in
the kernel. How about fixing the TEE to avoid supplying garbage
pointers?

>
> Fixes: e4a718a3a47e ("tee: fix NULL pointer dereference in tee_shm_put")
> Signed-off-by: yangzhao <yangzhao@kylinos.cn>
> ---
>  drivers/tee/tee_shm.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
> index 4a47de4bb2e5..de01d16409c1 100644
> --- a/drivers/tee/tee_shm.c
> +++ b/drivers/tee/tee_shm.c
> @@ -722,7 +722,14 @@ void tee_shm_put(struct tee_shm *shm)
>         struct tee_device *teedev;
>         bool do_release = false;
>
> -       if (!shm || !shm->ctx || !shm->ctx->teedev)
> +       /* checking pointer */
> +       if (IS_ERR_OR_NULL(shm) || !virt_addr_valid(shm))

The IS_ERR_OR_NULL() check might make sense, but the virt_addr_valid()
does not. virt_addr_valid() might catch a few garbage pointers, but
the real problem is that someone is supplying garbage pointers.

> +               return;
> +
> +       if (IS_ERR_OR_NULL(shm->ctx) || !virt_addr_valid(shm->ctx))
> +               return;
> +
> +       if (IS_ERR_OR_NULL(shm->ctx->teedev) || !virt_addr_valid(shm->ctx->teedev))
>                 return;

shm->ctx or shm->ctx->teedev should never be an ERR pointer. The
virt_addr_valid() test doesn't make sense.

Cheers,
Jens

>
>         teedev = shm->ctx->teedev;
> --
> 2.25.1
>

Re: [PATCH] tee: fix illegal pointer dereference in tee_shm_put()

Posted by yangzhao 1 month, 1 week ago

Hi，

This issue is indeed caused by the mismatch between the TEE version and 
the kernel version. Currently, this patch is only designed to address 
the problem of ensuring that the kernel does not panic when the TEE 
version is incorrect.

The virt_addr_valid() is used to check the validity of the pointer 
address, because during the test, it did occur that the shm->ctx pointer 
address is the address between the user space and the kernel space. At 
this point, using IS_ERR_OR_NULL cannot detect the legitimacy of the 
pointer address.

The problem can be viewed at the link: 
https://github.com/OP-TEE/optee_os/issues/7575

Thanks.

On 2025/11/6 19:31, Jens Wiklander wrote:
> Hi,
>
> On Thu, Nov 6, 2025 at 11:20 AM yangzhao <yangzhao@kylinos.cn> wrote:
>> In tee_shm_put(), there is not only the NULL pointer dereference,
>> but also the illegal pointer dereference.
>>
>> shutdown() --->
>>      __optee_disable_shm_cache -->
>>          shm = reg_pair_to_ptr(...);
>>          tee_shm_free(shm); -->
>>              tee_shm_put(shm); //crash: shm->ctx maybe NULL pointer or illegal pointer
>>
>> Check whether the pointer is NULL and whether the pointer address is valid.
>>
>> This issue occurs when rich world uses the 6.x version of the kernel and
>> optee secure world uses a lower version (such as version 3.2), and it is
>> highly likely to trigger a kernel panic when conducting hibernate tests.
> It sounds like the root of the problem is in the TEE rather than in
> the kernel. How about fixing the TEE to avoid supplying garbage
> pointers?
>
>> Fixes: e4a718a3a47e ("tee: fix NULL pointer dereference in tee_shm_put")
>> Signed-off-by: yangzhao <yangzhao@kylinos.cn>
>> ---
>>   drivers/tee/tee_shm.c | 9 ++++++++-
>>   1 file changed, 8 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
>> index 4a47de4bb2e5..de01d16409c1 100644
>> --- a/drivers/tee/tee_shm.c
>> +++ b/drivers/tee/tee_shm.c
>> @@ -722,7 +722,14 @@ void tee_shm_put(struct tee_shm *shm)
>>          struct tee_device *teedev;
>>          bool do_release = false;
>>
>> -       if (!shm || !shm->ctx || !shm->ctx->teedev)
>> +       /* checking pointer */
>> +       if (IS_ERR_OR_NULL(shm) || !virt_addr_valid(shm))
> The IS_ERR_OR_NULL() check might make sense, but the virt_addr_valid()
> does not. virt_addr_valid() might catch a few garbage pointers, but
> the real problem is that someone is supplying garbage pointers.
>
>> +               return;
>> +
>> +       if (IS_ERR_OR_NULL(shm->ctx) || !virt_addr_valid(shm->ctx))
>> +               return;
>> +
>> +       if (IS_ERR_OR_NULL(shm->ctx->teedev) || !virt_addr_valid(shm->ctx->teedev))
>>                  return;
> shm->ctx or shm->ctx->teedev should never be an ERR pointer. The
> virt_addr_valid() test doesn't make sense.
>
> Cheers,
> Jens
>
>>          teedev = shm->ctx->teedev;
>> --
>> 2.25.1
>>

Re: [PATCH] tee: fix illegal pointer dereference in tee_shm_put()

Posted by Jens Wiklander 1 month, 1 week ago

Hi,

On Fri, Nov 7, 2025 at 7:14 AM yangzhao <yangzhao@kylinos.cn> wrote:
>
> Hi，
>
> This issue is indeed caused by the mismatch between the TEE version and
> the kernel version. Currently, this patch is only designed to address
> the problem of ensuring that the kernel does not panic when the TEE
> version is incorrect.
>
> The virt_addr_valid() is used to check the validity of the pointer
> address, because during the test, it did occur that the shm->ctx pointer
> address is the address between the user space and the kernel space. At
> this point, using IS_ERR_OR_NULL cannot detect the legitimacy of the
> pointer address.
>
> The problem can be viewed at the link:
> https://github.com/OP-TEE/optee_os/issues/7575

OK, let's sort out the secure world side of things in that issue.

Cheers,
Jens

>
> Thanks.
>
> On 2025/11/6 19:31, Jens Wiklander wrote:
> > Hi,
> >
> > On Thu, Nov 6, 2025 at 11:20 AM yangzhao <yangzhao@kylinos.cn> wrote:
> >> In tee_shm_put(), there is not only the NULL pointer dereference,
> >> but also the illegal pointer dereference.
> >>
> >> shutdown() --->
> >>      __optee_disable_shm_cache -->
> >>          shm = reg_pair_to_ptr(...);
> >>          tee_shm_free(shm); -->
> >>              tee_shm_put(shm); //crash: shm->ctx maybe NULL pointer or illegal pointer
> >>
> >> Check whether the pointer is NULL and whether the pointer address is valid.
> >>
> >> This issue occurs when rich world uses the 6.x version of the kernel and
> >> optee secure world uses a lower version (such as version 3.2), and it is
> >> highly likely to trigger a kernel panic when conducting hibernate tests.
> > It sounds like the root of the problem is in the TEE rather than in
> > the kernel. How about fixing the TEE to avoid supplying garbage
> > pointers?
> >
> >> Fixes: e4a718a3a47e ("tee: fix NULL pointer dereference in tee_shm_put")
> >> Signed-off-by: yangzhao <yangzhao@kylinos.cn>
> >> ---
> >>   drivers/tee/tee_shm.c | 9 ++++++++-
> >>   1 file changed, 8 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
> >> index 4a47de4bb2e5..de01d16409c1 100644
> >> --- a/drivers/tee/tee_shm.c
> >> +++ b/drivers/tee/tee_shm.c
> >> @@ -722,7 +722,14 @@ void tee_shm_put(struct tee_shm *shm)
> >>          struct tee_device *teedev;
> >>          bool do_release = false;
> >>
> >> -       if (!shm || !shm->ctx || !shm->ctx->teedev)
> >> +       /* checking pointer */
> >> +       if (IS_ERR_OR_NULL(shm) || !virt_addr_valid(shm))
> > The IS_ERR_OR_NULL() check might make sense, but the virt_addr_valid()
> > does not. virt_addr_valid() might catch a few garbage pointers, but
> > the real problem is that someone is supplying garbage pointers.
> >
> >> +               return;
> >> +
> >> +       if (IS_ERR_OR_NULL(shm->ctx) || !virt_addr_valid(shm->ctx))
> >> +               return;
> >> +
> >> +       if (IS_ERR_OR_NULL(shm->ctx->teedev) || !virt_addr_valid(shm->ctx->teedev))
> >>                  return;
> > shm->ctx or shm->ctx->teedev should never be an ERR pointer. The
> > virt_addr_valid() test doesn't make sense.
> >
> > Cheers,
> > Jens
> >
> >>          teedev = shm->ctx->teedev;
> >> --
> >> 2.25.1
> >>
>