[PATCH] optee: Check return value of tee_shm_get_va()

Chen Ni posted 1 patch 1 month ago
drivers/tee/optee/rpc.c | 5 +++++
1 file changed, 5 insertions(+)
[PATCH] optee: Check return value of tee_shm_get_va()
Posted by Chen Ni 1 month ago
The function tee_shm_get_va() can return an error pointer if the shared
memory is not properly mapped or if the offset is invalid. Without this
check, passing the error pointer to subsequent memory operations could
lead to a kernel panic.

Add a check for IS_ERR() on the return value of tee_shm_get_va().

Fixes: f0c8431568ee ("optee: probe RPMB device using RPMB subsystem")
Signed-off-by: Chen Ni <nichen@iscas.ac.cn>
---
 drivers/tee/optee/rpc.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/tee/optee/rpc.c b/drivers/tee/optee/rpc.c
index b0ed4cb49452..32f7742c094c 100644
--- a/drivers/tee/optee/rpc.c
+++ b/drivers/tee/optee/rpc.c
@@ -393,6 +393,11 @@ static void handle_rpc_func_rpmb_frames(struct tee_context *ctx,
 			    params[0].u.memref.shm_offs);
 	p1 = tee_shm_get_va(params[1].u.memref.shm,
 			    params[1].u.memref.shm_offs);
+	if (IS_ERR(p0) || IS_ERR(p1)) {
+		arg->ret = TEEC_ERROR_BAD_PARAMETERS;
+		goto out;
+	}
+
 	if (rpmb_route_frames(rdev, p0, params[0].u.memref.size, p1,
 			      params[1].u.memref.size)) {
 		arg->ret = TEEC_ERROR_BAD_PARAMETERS;
-- 
2.25.1
Re: [PATCH] optee: Check return value of tee_shm_get_va()
Posted by Markus Elfring 1 month ago
…
> +++ b/drivers/tee/optee/rpc.c
> @@ -393,6 +393,11 @@ static void handle_rpc_func_rpmb_frames(struct tee_context *ctx,
>  			    params[0].u.memref.shm_offs);
>  	p1 = tee_shm_get_va(params[1].u.memref.shm,
>  			    params[1].u.memref.shm_offs);
> +	if (IS_ERR(p0) || IS_ERR(p1)) {
> +		arg->ret = TEEC_ERROR_BAD_PARAMETERS;
> +		goto out;
> +	}
> +
>  	if (rpmb_route_frames(rdev, p0, params[0].u.memref.size, p1,
>  			      params[1].u.memref.size)) {
>  		arg->ret = TEEC_ERROR_BAD_PARAMETERS;
…

How do you think about to use an additional label for the shown
error code assignment?
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v7.0-rc2#n526

Regards,
Markus
Re: [PATCH] optee: Check return value of tee_shm_get_va()
Posted by Sumit Garg 4 weeks ago
On Fri, Mar 06, 2026 at 12:52:39PM +0100, Markus Elfring wrote:
> …
> > +++ b/drivers/tee/optee/rpc.c
> > @@ -393,6 +393,11 @@ static void handle_rpc_func_rpmb_frames(struct tee_context *ctx,
> >  			    params[0].u.memref.shm_offs);
> >  	p1 = tee_shm_get_va(params[1].u.memref.shm,
> >  			    params[1].u.memref.shm_offs);
> > +	if (IS_ERR(p0) || IS_ERR(p1)) {
> > +		arg->ret = TEEC_ERROR_BAD_PARAMETERS;
> > +		goto out;
> > +	}
> > +
> >  	if (rpmb_route_frames(rdev, p0, params[0].u.memref.size, p1,
> >  			      params[1].u.memref.size)) {
> >  		arg->ret = TEEC_ERROR_BAD_PARAMETERS;
> …
> 
> How do you think about to use an additional label for the shown
> error code assignment?
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v7.0-rc2#n526
>

I suppose here you meant to update the label name since it's the same
error type used by other code paths too. So following label rename
should be fine I think as per coding guidelines:

s/out/err_dev_put/

-Sumit
Re: [PATCH] optee: Check return value of tee_shm_get_va()
Posted by Jens Wiklander 4 weeks ago
On Thu, Mar 12, 2026 at 10:16 AM Sumit Garg <sumit.garg@kernel.org> wrote:
>
> On Fri, Mar 06, 2026 at 12:52:39PM +0100, Markus Elfring wrote:
> > …
> > > +++ b/drivers/tee/optee/rpc.c
> > > @@ -393,6 +393,11 @@ static void handle_rpc_func_rpmb_frames(struct tee_context *ctx,
> > >                         params[0].u.memref.shm_offs);
> > >     p1 = tee_shm_get_va(params[1].u.memref.shm,
> > >                         params[1].u.memref.shm_offs);
> > > +   if (IS_ERR(p0) || IS_ERR(p1)) {
> > > +           arg->ret = TEEC_ERROR_BAD_PARAMETERS;
> > > +           goto out;
> > > +   }
> > > +
> > >     if (rpmb_route_frames(rdev, p0, params[0].u.memref.size, p1,
> > >                           params[1].u.memref.size)) {
> > >             arg->ret = TEEC_ERROR_BAD_PARAMETERS;
> > …
> >
> > How do you think about to use an additional label for the shown
> > error code assignment?
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v7.0-rc2#n526
> >
>
> I suppose here you meant to update the label name since it's the same
> error type used by other code paths too. So following label rename
> should be fine I think as per coding guidelines:
>
> s/out/err_dev_put/

Wouldn't the name err_dev_put suggest this only occurs in the error path?

Cheers,
Jens

>
> -Sumit
Re: [PATCH] optee: Check return value of tee_shm_get_va()
Posted by Sumit Garg 3 weeks, 4 days ago
On Thu, Mar 12, 2026 at 12:16:11PM +0100, Jens Wiklander wrote:
> On Thu, Mar 12, 2026 at 10:16 AM Sumit Garg <sumit.garg@kernel.org> wrote:
> >
> > On Fri, Mar 06, 2026 at 12:52:39PM +0100, Markus Elfring wrote:
> > > …
> > > > +++ b/drivers/tee/optee/rpc.c
> > > > @@ -393,6 +393,11 @@ static void handle_rpc_func_rpmb_frames(struct tee_context *ctx,
> > > >                         params[0].u.memref.shm_offs);
> > > >     p1 = tee_shm_get_va(params[1].u.memref.shm,
> > > >                         params[1].u.memref.shm_offs);
> > > > +   if (IS_ERR(p0) || IS_ERR(p1)) {
> > > > +           arg->ret = TEEC_ERROR_BAD_PARAMETERS;
> > > > +           goto out;
> > > > +   }
> > > > +
> > > >     if (rpmb_route_frames(rdev, p0, params[0].u.memref.size, p1,
> > > >                           params[1].u.memref.size)) {
> > > >             arg->ret = TEEC_ERROR_BAD_PARAMETERS;
> > > …
> > >
> > > How do you think about to use an additional label for the shown
> > > error code assignment?
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?h=v7.0-rc2#n526
> > >
> >
> > I suppose here you meant to update the label name since it's the same
> > error type used by other code paths too. So following label rename
> > should be fine I think as per coding guidelines:
> >
> > s/out/err_dev_put/
> 
> Wouldn't the name err_dev_put suggest this only occurs in the error path?
> 

Okay, let rather rename it to out_dev_put.

-Sumit