Re: [Xen-devel] [PATCH v3 07/11] optee: add support for RPC SHM buffers

[Julien Grall <julien.grall@xxxxxxx>]

Hi Volodymyr,

On 18/01/2019 16:05, Volodymyr Babchuk wrote:
> Julien Grall writes:
> > > > If you happen to make MAX_STD_CALLS dynamic, then this should also be
> > > > dynamic.
> > > Of course.
> > >
> > > [...]
> > > > > > >      +static void handle_rpc_func_alloc(struct optee_domain *ctx,
> > > > > > > +                                  struct cpu_user_regs *regs)
> > > > > > > +{
> > > > > > > +    paddr_t ptr = get_user_reg(regs, 1) << 32 | get_user_reg(regs, 2);
> > > > > > > +
> > > > > > > +    if ( ptr & (OPTEE_MSG_NONCONTIG_PAGE_SIZE - 1) )
> > > > > > > +        gprintk(XENLOG_WARNING, "Domain returned invalid RPC command 
> > > > > > > buffer\n");
> > > > > > Should not you bail-out in that case? Also, I would turn it to a gdprintk.
> > > > > OP-TEE does own checks and that check will fail also. Then OP-TEE will
> > > > > issue request to free this SHM.
> > > > I think it is better if we go on the safe-side. I.e if we know there
> > > > would be an error (like here), then you need to return an error in
> > > > from Xen rather than calling OP-TEE. Otherwise, you may end up to
> > > > nasty problem.
> > > Actually, I don't see how I can do this. This is an RPC response. I
> > > can't return error to RPC response. All I can do is to mangle RPC
> > > response in a such way, that OP-TEE surely will treat it as an error and
> > > act accordingly.
> > I am not sure to understand what you mean here. Surely if the address
> > is not aligned, then OP-TEE will return an error too, right? So can't
> > you emulate OP-TEE behavior in that case?
> No, OP-TEE will not return an error. OP-TEE will handle it as an error.
Will it? Looking at the code again, you will pass either 0 (when not able to 
translate the address) or page_to_maddr(...) value. So the value will get 
truncated by Xen with just a warning.
Is it the expected behavior?

> For OP-TEE any RPC looks like a function call. So it excepts some return
> value - buffer pointer in this case. If it gets invalid pointer, it
> issues another RPC to free this buffer and then propagates error back to
> caller.
>
> So, if I'll return error back to the guest (or rather issue RPC to free
> the buffer), OP-TEE will be still blocked, waiting for a pointer from
> the guest. Of course I can go further end emulate a new buffer
> allocation RPC but from Xen side in a hope that guest will provide valid
> buffer this time. But, what if not? And why I should decide instead of
> OP-TEE?
My main concern here is you do a print a warning and continue like nothing 
happen. As a reviewer, this is worrying because it is a call for weird behavior 
to happen in Xen. We don't want that.
Looking at the code, why can't you pass 0 as you do if you fail to pin the 
pages?

Cheers,

--
Julien Grall

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