Re: [Xen-devel] [PATCH v3 09/11] optee: add support for RPC commands

[Julien Grall <julien.grall@xxxxxxx>]

Hi,

On 19/02/2019 16:14, Volodymyr Babchuk wrote:
> Hi Julien,
>
> Julien Grall writes:
>
> > Hi Volodymyr,
> >
> > On 18/12/2018 21:11, Volodymyr Babchuk wrote:
> > > From: Volodymyr Babchuk <vlad.babchuk@xxxxxxxxx>
> > >
> > > OP-TEE can issue multiple RPC requests. We are interested mostly in
> > > request that asks NW to allocate/free shared memory for OP-TEE
> > > needs, because mediator need to do address translation in the same
> > NIT: the mediator needs
> >
> > > way as it was done for shared buffers registered by NW.
> > >
> > > As mediator now accesses shared command buffer, we need to shadow
> > > it in the same way, as we shadow request buffers for STD calls.
> > This is a bit confusing, does it means patch #8 is not doing the right thing?
> No, it was patch #6 :)
>
> And I can't say that it did something wrong. Remember that prior to the
> last patch in series DomU can't use the mediator. And for Dom0 it is okay to
> map RPC command buffer directly. Description of patch #4 mentions that
> we need all patches in the series for a complete mediator.
Not all the memory in Dom0 is 1:1 mapped. So you may end up to use the wrong 
address here.
But, it is not very intuitive to have to read the commit message of patch #4 to 
understand that patch #8 is fixing a flaw in patch #6.
Technically, earlier patch should not have allowed to use shared command buffer 
until now. While I appreciate it is hard to split big series, we at least need 
to write clear commit message. In that case "now accesses" clear lead to think 
something wrong has been done before. So a reminder in the commit message would 
help the reviewer here.
> > > Signed-off-by: Volodymyr Babchuk <vlad.babchuk@xxxxxxxxx>
> > > ---
> > >
> > >    Changes from v2:
> > >    - Use access_guest_memory_by_ipa() instead of direct mapping
> > >
> > >    xen/arch/arm/tee/optee.c | 136 +++++++++++++++++++++++++++++++++++++--
> > >    1 file changed, 130 insertions(+), 6 deletions(-)
> > >
> > > diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
> > > index cfc3b34df7..bf3535946d 100644
> > > --- a/xen/arch/arm/tee/optee.c
> > > +++ b/xen/arch/arm/tee/optee.c
> > > @@ -67,6 +67,8 @@ struct optee_std_call {
> > >    struct shm_rpc {
> > >        struct list_head list;
> > >        struct page_info *guest_page;
> > > +    struct optee_msg_arg *xen_arg;
> > > +    paddr_t guest_ipa;
> > >        uint64_t cookie;
> > >    };
> > >    @@ -290,6 +292,11 @@ static struct shm_rpc
> > > *allocate_and_pin_shm_rpc(struct optee_domain *ctx,
> > >                                                P2M_ALLOC);
> > >        if ( !shm_rpc->guest_page )
> > >            goto err;
> > > +    shm_rpc->guest_ipa = gaddr;
> > > +
> > > +    shm_rpc->xen_arg = alloc_xenheap_page();
> > Based on the discussion in patch #6, I think you want to use to
> > allocate the memory from domheap.
> Yes, will do.
> I already did it for #6 and now there is a lots of places
> where I am mapping/unmapping that page. So, it is somewhat
> inconvenient...
Well, security is always inconvenient until we found a flaw ;).

Cheers,

--
Julien Grall

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel