linux-kernel - Re: [PATCH v6 1/4] tee: optee: system call property

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAN5uoS_0pF=-9=gznYUOU9oWPKz8HkRb=6gTAumMv=Vey8qb0g@mail.gmail.com>
Date:   Thu, 11 May 2023 09:20:35 +0200
From:   Etienne Carriere <etienne.carriere@...aro.org>
To:     Sumit Garg <sumit.garg@...aro.org>
Cc:     linux-kernel@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        op-tee@...ts.trustedfirmware.org,
        Jens Wiklander <jens.wiklander@...aro.org>,
        Sudeep Holla <sudeep.holla@....com>,
        Cristian Marussi <cristian.marussi@....com>
Subject: Re: [PATCH v6 1/4] tee: optee: system call property

On Thu, 11 May 2023 at 08:03, Sumit Garg <sumit.garg@...aro.org> wrote:
> (snip)
> > > >
> > > >  int optee_invoke_func(struct tee_context *ctx, struct tee_ioctl_invoke_arg *arg,
> > > > @@ -408,12 +412,15 @@ int optee_invoke_func(struct tee_context *ctx, struct tee_ioctl_invoke_arg *arg,
> > > >         struct optee_msg_arg *msg_arg;
> > > >         struct optee_session *sess;
> > > >         struct tee_shm *shm;
> > > > +       bool system_thread;
> > > >         u_int offs;
> > > >         int rc;
> > > >
> > > >         /* Check that the session is valid */
> > > >         mutex_lock(&ctxdata->mutex);
> > > >         sess = find_session(ctxdata, arg->session);
> > > > +       if (sess)
> > >
> > > This check is redundant if we move the assignment below...
> >
> > Here we change the sesssion attribute while the mutex is locked, in
> > case some equivalent call with that session is issued.
> > Below we return to caller once mutex is unlocked.
> > I think it is the safer behavior. What do you think?
>
> Aren't we only reading session attribute in order to capture value in
> a local variable: system_thread? I don't think that it would require a
> mutex.

optee_system_session() sets session::use_sys_thread with mutex locked
hence I think we should get the attribute with the mutex locked.
See "[PATCH v6 3/4] tee: optee: support tracking system threads".

Etienne

>
> -Sumit
>
> >
> > Best regards,
> > Etienne
> >
> > >
> > > > +               system_thread = sess->use_sys_thread;
> > > >         mutex_unlock(&ctxdata->mutex);
> > > >         if (!sess)
> > > >                 return -EINVAL;
> > >
> > > ...here as:
> > >            system_thread = sess->use_sys_thread;
> > > (snip)