Hi,
On 20/03/2019 16:39, Volodymyr Babchuk wrote:
Julien Grall writes:
On 07/03/2019 21:04, Volodymyr Babchuk wrote:
From: Volodymyr Babchuk vlad.babchuk@gmail.com
Some of the patches are using your EPAM e-mail addresss. Other are using your gmail address. Could you confirm this is expected?
No, I'll make sure that all patches are authored by volodymyr_babchuk@epam.com
And your signed-off-by as well :).
[...]
I can't promise Xen will only be 4K only. So it would be better to make the number agnostic. Or at least writing clearly on top of the definition that it is assumed 4KB (maybe with a BUILD_BUG_ON(PAGE_SIZE != 4096) if not already in place).
I can replace define with something like (67108864 / PAGE_SIZE). With appropriate comment, of course.
Well, none of your code is ready for another PAGE_SIZE than 4KB. So the BUILD_BUG_ON(PAGE_SIZE != 4096) is probably more suitable.
[...]
+static struct optee_shm_buf *allocate_optee_shm_buf(struct optee_domain *ctx,
uint64_t cookie,unsigned int pages_cnt,struct page_info *pg_list,unsigned int pg_list_order)+{
- struct optee_shm_buf *optee_shm_buf, *optee_shm_buf_tmp;
- int old, new;
- int err_code;
- do
- {
old = atomic_read(&ctx->optee_shm_buf_pages);new = old + pages_cnt;if ( new >= MAX_TOTAL_SMH_BUF_PG )Again, the limitation is in number of page and quite high. What would prevent a guest to register shared memory page by page? If nothing, then I think you can end up to interesting issues in Xen because of the growing list and memory used.
OP-TEE will limit this on it's side. In most cases Xen have much bigger heap than OP-TEE :-)
The main problem is not the heap. The problem is the size of the list to browse.
Do you want me to limit this both in memory size and in number of buffers?
I am not necessarily asking to put a limitation. What I ask is documenting what can happen. So we can take proper action in the future (such as when deciding whether to security support it).
[...]
[...]
static int optee_relinquish_resources(struct domain *d) { struct optee_std_call *call, *call_tmp; struct shm_rpc *shm_rpc, *shm_rpc_tmp;
- struct optee_shm_buf *optee_shm_buf, *optee_shm_buf_tmp; struct optee_domain *ctx = d->arch.tee; if ( !ctx )
@@ -381,6 +552,13 @@ static int optee_relinquish_resources(struct domain *d) list_for_each_entry_safe( shm_rpc, shm_rpc_tmp, &ctx->shm_rpc_list, list ) free_shm_rpc(ctx, shm_rpc->cookie);
- if ( hypercall_preempt_check() )
return -ERESTART;Same question as the other hypercall_preempt_check().
Excuse me, but what question? I looked thru all your emails and can't find one.
It looks like I have by mistake trimmed my question on the other patch :/.
Anyway, you should explain how you decide the placement of each hypercall_preempt_check(). For instance, if the lists are quite big, then you may want add a preempt check in the loop.
The key point here is to document the choice even if you wrote it is "random". This will help in the future if we need to revise preemption choice.
- list_for_each_entry_safe( optee_shm_buf, optee_shm_buf_tmp,
&ctx->optee_shm_buf_list, list )free_optee_shm_buf(ctx, optee_shm_buf->cookie);return 0;domain *d) ASSERT(!spin_is_locked(&ctx->lock)); ASSERT(!atomic_read(&ctx->call_count));
- ASSERT(!atomic_read(&ctx->optee_shm_buf_pages)); ASSERT(list_empty(&ctx->shm_rpc_list)); XFREE(d->arch.tee); } +#define PAGELIST_ENTRIES_PER_PAGE \
- ((OPTEE_MSG_NONCONTIG_PAGE_SIZE / sizeof(u64)) - 1)
+static size_t get_pages_list_size(size_t num_entries) +{
- int pages = DIV_ROUND_UP(num_entries, PAGELIST_ENTRIES_PER_PAGE);
- return pages * OPTEE_MSG_NONCONTIG_PAGE_SIZE;
+}
+static int translate_noncontig(struct optee_domain *ctx,
struct optee_std_call *call,struct optee_msg_param *param)+{
- uint64_t size;
- unsigned int page_offset;
- unsigned int num_pages;
- unsigned int order;
- unsigned int entries_on_page = 0;
- gfn_t gfn;
- p2m_type_t p2m;
- struct page_info *guest_page, *xen_pages;
- struct optee_shm_buf *optee_shm_buf;
- /*
* This is memory layout for page list. Basically list consists of 4k pages,* every page store 511 page addresses of user buffer and page address of* the next page of list.** Refer to OPTEE_MSG_ATTR_NONCONTIG description in optee_msg.h for details.*/- struct {
uint64_t pages_list[PAGELIST_ENTRIES_PER_PAGE];uint64_t next_page_data;- } *pages_data_guest, *pages_data_xen;
- /* Offset of user buffer withing page */
Offset of the buffer within the OP-TEE page
- page_offset = param->u.tmem.buf_ptr & (OPTEE_MSG_NONCONTIG_PAGE_SIZE - 1);
- /* Size of the user buffer in bytes */
- size = ROUNDUP(param->u.tmem.size + page_offset,
OPTEE_MSG_NONCONTIG_PAGE_SIZE);- num_pages = DIV_ROUND_UP(size, OPTEE_MSG_NONCONTIG_PAGE_SIZE);
- order = get_order_from_bytes(get_pages_list_size(num_pages));
- xen_pages = alloc_domheap_pages(current->domain, order, 0);
- if ( !xen_pages )
return -ENOMEM;- optee_shm_buf = allocate_optee_shm_buf(ctx, param->u.tmem.shm_ref,
num_pages, xen_pages, order);In an earlier version, I pointed out that you will allow to allocate up to 64MB per call. This is a potential security issue on Xen. While I said I would be happy to get this code merged as it, I would expect to at least see a TODO in the code explaining potential problem. So we know the problem exist and can't security support until this is fixed.
Sure. You want me to add this TODO there or inside allocate_optee_shm_buf() function, where it actually does allocation?
I don't mind the placement as long as the TODO is there.
I may have missed other places while reviewing this version. Please go back in the review I have made in the past and document all the potential security holes.
Okay, will do.
- if ( IS_ERR(optee_shm_buf) )
return PTR_ERR(optee_shm_buf);- gfn = gaddr_to_gfn(param->u.tmem.buf_ptr &
~(OPTEE_MSG_NONCONTIG_PAGE_SIZE - 1));- guest_page = get_page_from_gfn(current->domain, gfn_x(gfn), &p2m, P2M_ALLOC);
- if ( !guest_page || p2m != p2m_ram_rw )
return -EINVAL;- pages_data_guest = __map_domain_page(guest_page);quite
- pages_data_xen = __map_domain_page(xen_pages);
- while ( num_pages )
- {
struct page_info *page;Newline here please.
page = get_page_from_gfn(current->domain,paddr_to_pfn(pages_data_guest->pages_list[entries_on_page]),&p2m, P2M_ALLOC);The indentation is wrong here. But the problem is due to the long name. For instance, you have 3 times the word "page" on the same line. Is that necessary?
Code is quite complex. I want every variable to be as descriptive as possible. In some cases it leads to issues like this :-)
There are way to simplify this code.
1) The OP-TEE code contains the following pattern a few time.
page = get_page_from_gfn(....) if ( !page || p2mt != p2m_ram_rw ) ...
You can create an helper to encapsulate that. I think you have one case where the p2mt check is different. So potentially you want to patch the type check in parameter.
2) You probably move __map_domain_page(guest_page/xen_pages) within the loop. And just deal with guest_page/xen_pages outside.
With that and the renaming, then the code will become suddenly a bit less complex.
Cheers,