[PATCH v2 0/2] optee: SHM handling fixes

[PATCH v2 0/2] optee: SHM handling fixes

[Volodymyr Babchuk]

[-- Attachment #1: Type: text/plain, Size: 591 bytes --]

There are two patches that previously was mailed separatedly. Both
patches fix issues found during testing the OP-TEE 3.9 release.

Julien and Stefano suggested to include this patches in Xen 4.14
release, because optee support still in the preview state and those
patches provide no new functionality, bugfixes only.

Volodymyr Babchuk (2):
  optee: immediately free buffers that are released by OP-TEE
  optee: allow plain TMEM buffers with NULL address

 xen/arch/arm/tee/optee.c | 59 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 52 insertions(+), 7 deletions(-)

-- 
2.26.2

[PATCH v2 0/2] optee: SHM handling fixes

[Volodymyr Babchuk]

There are two patches that previously was mailed separatedly. Both
patches fix issues found during testing the OP-TEE 3.9 release.

Julien and Stefano suggested to include this patches in Xen 4.14
release, because optee support still in the preview state and those
patches provide no new functionality, bugfixes only.

Volodymyr Babchuk (2):
  optee: immediately free buffers that are released by OP-TEE
  optee: allow plain TMEM buffers with NULL address

 xen/arch/arm/tee/optee.c | 59 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 52 insertions(+), 7 deletions(-)

-- 
2.26.2

[PATCH v2 1/2] optee: immediately free buffers that are released by OP-TEE

[Volodymyr Babchuk]

[-- Attachment #1: Type: text/plain, Size: 3798 bytes --]

Normal World can share buffer with OP-TEE for two reasons:
1. Some client application wants to exchange data with TA
2. OP-TEE asks for shared buffer for internal needs

The second case was handle more strictly than necessary:

1. In RPC request OP-TEE asks for buffer
2. NW allocates buffer and provides it via RPC response
3. Xen pins pages and translates data
4. Xen provides buffer to OP-TEE
5. OP-TEE uses it
6. OP-TEE sends request to free the buffer
7. NW frees the buffer and sends the RPC response
8. Xen unpins pages and forgets about the buffer

The problem is that Xen should forget about buffer in between stages 6
and 7. I.e. the right flow should be like this:

6. OP-TEE sends request to free the buffer
7. Xen unpins pages and forgets about the buffer
8. NW frees the buffer and sends the RPC response

This is because OP-TEE internally frees the buffer before sending the
"free SHM buffer" request. So we have no reason to hold reference for
this buffer anymore. Moreover, in multiprocessor systems NW have time
to reuse buffer cookie for another buffer. Xen complained about this
and denied the new buffer registration. I have seen this issue while
running tests on iMX SoC.

So, this patch basically corrects that behavior by freeing the buffer
earlier, when handling RPC return from OP-TEE.

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---

Changes from v1:
 - reworded the comments
 - added WARN() for a case when OP-TEE wants to release not the
   buffer it requeset to allocate durint this call

---
 xen/arch/arm/tee/optee.c | 32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
index 6a035355db..6963238056 100644
--- a/xen/arch/arm/tee/optee.c
+++ b/xen/arch/arm/tee/optee.c
@@ -1099,6 +1099,34 @@ static int handle_rpc_return(struct optee_domain *ctx,
         if ( shm_rpc->xen_arg->cmd == OPTEE_RPC_CMD_SHM_ALLOC )
             call->rpc_buffer_type = shm_rpc->xen_arg->params[0].u.value.a;
 
+        /*
+         * OP-TEE is signalling that it has freed the buffer that it
+         * requested before. This is the right time for us to do the
+         * same.
+         */
+        if ( shm_rpc->xen_arg->cmd == OPTEE_RPC_CMD_SHM_FREE )
+        {
+            uint64_t cookie = shm_rpc->xen_arg->params[0].u.value.b;
+
+            free_optee_shm_buf(ctx, cookie);
+
+            /*
+             * OP-TEE asks to free buffer, but this is not the same
+             * buffer we previously allocated for it. While nothing
+             * prevents OP-TEE from asking this, it is the strange
+             * situation. This may or may not be caused by a bug in
+             * OP-TEE or mediator. But is better to print warning.
+             */
+            if ( call->rpc_data_cookie && call->rpc_data_cookie != cookie )
+            {
+                gprintk(XENLOG_ERR,
+                        "Saved RPC cookie does not corresponds to OP-TEE's (%"PRIx64" != %"PRIx64")\n",
+                        call->rpc_data_cookie, cookie);
+
+                WARN();
+            }
+            call->rpc_data_cookie = 0;
+        }
         unmap_domain_page(shm_rpc->xen_arg);
     }
 
@@ -1464,10 +1492,6 @@ static void handle_rpc_cmd(struct optee_domain *ctx, struct cpu_user_regs *regs,
             }
             break;
         case OPTEE_RPC_CMD_SHM_FREE:
-            free_optee_shm_buf(ctx, shm_rpc->xen_arg->params[0].u.value.b);
-            if ( call->rpc_data_cookie ==
-                 shm_rpc->xen_arg->params[0].u.value.b )
-                call->rpc_data_cookie = 0;
             break;
         default:
             break;
-- 
2.26.2

[PATCH v2 1/2] optee: immediately free buffers that are released by OP-TEE

[Volodymyr Babchuk]

Normal World can share buffer with OP-TEE for two reasons:
1. Some client application wants to exchange data with TA
2. OP-TEE asks for shared buffer for internal needs

The second case was handle more strictly than necessary:

1. In RPC request OP-TEE asks for buffer
2. NW allocates buffer and provides it via RPC response
3. Xen pins pages and translates data
4. Xen provides buffer to OP-TEE
5. OP-TEE uses it
6. OP-TEE sends request to free the buffer
7. NW frees the buffer and sends the RPC response
8. Xen unpins pages and forgets about the buffer

The problem is that Xen should forget about buffer in between stages 6
and 7. I.e. the right flow should be like this:

6. OP-TEE sends request to free the buffer
7. Xen unpins pages and forgets about the buffer
8. NW frees the buffer and sends the RPC response

This is because OP-TEE internally frees the buffer before sending the
"free SHM buffer" request. So we have no reason to hold reference for
this buffer anymore. Moreover, in multiprocessor systems NW have time
to reuse buffer cookie for another buffer. Xen complained about this
and denied the new buffer registration. I have seen this issue while
running tests on iMX SoC.

So, this patch basically corrects that behavior by freeing the buffer
earlier, when handling RPC return from OP-TEE.

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---

Changes from v1:
 - reworded the comments
 - added WARN() for a case when OP-TEE wants to release not the
   buffer it requeset to allocate durint this call

---
 xen/arch/arm/tee/optee.c | 32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
index 6a035355db..6963238056 100644
--- a/xen/arch/arm/tee/optee.c
+++ b/xen/arch/arm/tee/optee.c
@@ -1099,6 +1099,34 @@ static int handle_rpc_return(struct optee_domain *ctx,
         if ( shm_rpc->xen_arg->cmd == OPTEE_RPC_CMD_SHM_ALLOC )
             call->rpc_buffer_type = shm_rpc->xen_arg->params[0].u.value.a;
 
+        /*
+         * OP-TEE is signalling that it has freed the buffer that it
+         * requested before. This is the right time for us to do the
+         * same.
+         */
+        if ( shm_rpc->xen_arg->cmd == OPTEE_RPC_CMD_SHM_FREE )
+        {
+            uint64_t cookie = shm_rpc->xen_arg->params[0].u.value.b;
+
+            free_optee_shm_buf(ctx, cookie);
+
+            /*
+             * OP-TEE asks to free buffer, but this is not the same
+             * buffer we previously allocated for it. While nothing
+             * prevents OP-TEE from asking this, it is the strange
+             * situation. This may or may not be caused by a bug in
+             * OP-TEE or mediator. But is better to print warning.
+             */
+            if ( call->rpc_data_cookie && call->rpc_data_cookie != cookie )
+            {
+                gprintk(XENLOG_ERR,
+                        "Saved RPC cookie does not corresponds to OP-TEE's (%"PRIx64" != %"PRIx64")\n",
+                        call->rpc_data_cookie, cookie);
+
+                WARN();
+            }
+            call->rpc_data_cookie = 0;
+        }
         unmap_domain_page(shm_rpc->xen_arg);
     }
 
@@ -1464,10 +1492,6 @@ static void handle_rpc_cmd(struct optee_domain *ctx, struct cpu_user_regs *regs,
             }
             break;
         case OPTEE_RPC_CMD_SHM_FREE:
-            free_optee_shm_buf(ctx, shm_rpc->xen_arg->params[0].u.value.b);
-            if ( call->rpc_data_cookie ==
-                 shm_rpc->xen_arg->params[0].u.value.b )
-                call->rpc_data_cookie = 0;
             break;
         default:
             break;
-- 
2.26.2

Re: [PATCH v2 1/2] optee: immediately free buffers that are released by OP-TEE

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 4383 bytes --]

On Fri, 19 Jun 2020, Volodymyr Babchuk wrote:
> Normal World can share buffer with OP-TEE for two reasons:
> 1. Some client application wants to exchange data with TA
> 2. OP-TEE asks for shared buffer for internal needs
> 
> The second case was handle more strictly than necessary:
> 
> 1. In RPC request OP-TEE asks for buffer
> 2. NW allocates buffer and provides it via RPC response
> 3. Xen pins pages and translates data
> 4. Xen provides buffer to OP-TEE
> 5. OP-TEE uses it
> 6. OP-TEE sends request to free the buffer
> 7. NW frees the buffer and sends the RPC response
> 8. Xen unpins pages and forgets about the buffer
> 
> The problem is that Xen should forget about buffer in between stages 6
> and 7. I.e. the right flow should be like this:
> 
> 6. OP-TEE sends request to free the buffer
> 7. Xen unpins pages and forgets about the buffer
> 8. NW frees the buffer and sends the RPC response
> 
> This is because OP-TEE internally frees the buffer before sending the
> "free SHM buffer" request. So we have no reason to hold reference for
> this buffer anymore. Moreover, in multiprocessor systems NW have time
> to reuse buffer cookie for another buffer. Xen complained about this
> and denied the new buffer registration. I have seen this issue while
> running tests on iMX SoC.
> 
> So, this patch basically corrects that behavior by freeing the buffer
> earlier, when handling RPC return from OP-TEE.
> 
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

There are a couple of grammar issues in the comments, but we can fix
them on commit.

Acked-by: Stefano Stabellini <sstabellini@kernel.org>



> ---
> 
> Changes from v1:
>  - reworded the comments
>  - added WARN() for a case when OP-TEE wants to release not the
>    buffer it requeset to allocate durint this call
> 
> ---
>  xen/arch/arm/tee/optee.c | 32 ++++++++++++++++++++++++++++----
>  1 file changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
> index 6a035355db..6963238056 100644
> --- a/xen/arch/arm/tee/optee.c
> +++ b/xen/arch/arm/tee/optee.c
> @@ -1099,6 +1099,34 @@ static int handle_rpc_return(struct optee_domain *ctx,
>          if ( shm_rpc->xen_arg->cmd == OPTEE_RPC_CMD_SHM_ALLOC )
>              call->rpc_buffer_type = shm_rpc->xen_arg->params[0].u.value.a;
>  
> +        /*
> +         * OP-TEE is signalling that it has freed the buffer that it
> +         * requested before. This is the right time for us to do the
> +         * same.
> +         */
> +        if ( shm_rpc->xen_arg->cmd == OPTEE_RPC_CMD_SHM_FREE )
> +        {
> +            uint64_t cookie = shm_rpc->xen_arg->params[0].u.value.b;
> +
> +            free_optee_shm_buf(ctx, cookie);
> +
> +            /*
> +             * OP-TEE asks to free buffer, but this is not the same
> +             * buffer we previously allocated for it. While nothing
> +             * prevents OP-TEE from asking this, it is the strange
                                                          ^ a

> +             * situation. This may or may not be caused by a bug in
> +             * OP-TEE or mediator. But is better to print warning.
                                          ^ it is

> +             */
> +            if ( call->rpc_data_cookie && call->rpc_data_cookie != cookie )
> +            {
> +                gprintk(XENLOG_ERR,
> +                        "Saved RPC cookie does not corresponds to OP-TEE's (%"PRIx64" != %"PRIx64")\n",
                                                      ^ correspond


> +                        call->rpc_data_cookie, cookie);
> +
> +                WARN();
> +            }
> +            call->rpc_data_cookie = 0;
> +        }
>          unmap_domain_page(shm_rpc->xen_arg);
>      }
>  
> @@ -1464,10 +1492,6 @@ static void handle_rpc_cmd(struct optee_domain *ctx, struct cpu_user_regs *regs,
>              }
>              break;
>          case OPTEE_RPC_CMD_SHM_FREE:
> -            free_optee_shm_buf(ctx, shm_rpc->xen_arg->params[0].u.value.b);
> -            if ( call->rpc_data_cookie ==
> -                 shm_rpc->xen_arg->params[0].u.value.b )
> -                call->rpc_data_cookie = 0;
>              break;
>          default:
>              break;
> -- 
> 2.26.2
> 

Re: [PATCH v2 1/2] optee: immediately free buffers that are released by OP-TEE

[Stefano Stabellini]

On Fri, 19 Jun 2020, Volodymyr Babchuk wrote:
> Normal World can share buffer with OP-TEE for two reasons:
> 1. Some client application wants to exchange data with TA
> 2. OP-TEE asks for shared buffer for internal needs
> 
> The second case was handle more strictly than necessary:
> 
> 1. In RPC request OP-TEE asks for buffer
> 2. NW allocates buffer and provides it via RPC response
> 3. Xen pins pages and translates data
> 4. Xen provides buffer to OP-TEE
> 5. OP-TEE uses it
> 6. OP-TEE sends request to free the buffer
> 7. NW frees the buffer and sends the RPC response
> 8. Xen unpins pages and forgets about the buffer
> 
> The problem is that Xen should forget about buffer in between stages 6
> and 7. I.e. the right flow should be like this:
> 
> 6. OP-TEE sends request to free the buffer
> 7. Xen unpins pages and forgets about the buffer
> 8. NW frees the buffer and sends the RPC response
> 
> This is because OP-TEE internally frees the buffer before sending the
> "free SHM buffer" request. So we have no reason to hold reference for
> this buffer anymore. Moreover, in multiprocessor systems NW have time
> to reuse buffer cookie for another buffer. Xen complained about this
> and denied the new buffer registration. I have seen this issue while
> running tests on iMX SoC.
> 
> So, this patch basically corrects that behavior by freeing the buffer
> earlier, when handling RPC return from OP-TEE.
> 
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>

There are a couple of grammar issues in the comments, but we can fix
them on commit.

Acked-by: Stefano Stabellini <sstabellini@kernel.org>



> ---
> 
> Changes from v1:
>  - reworded the comments
>  - added WARN() for a case when OP-TEE wants to release not the
>    buffer it requeset to allocate durint this call
> 
> ---
>  xen/arch/arm/tee/optee.c | 32 ++++++++++++++++++++++++++++----
>  1 file changed, 28 insertions(+), 4 deletions(-)
> 
> diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
> index 6a035355db..6963238056 100644
> --- a/xen/arch/arm/tee/optee.c
> +++ b/xen/arch/arm/tee/optee.c
> @@ -1099,6 +1099,34 @@ static int handle_rpc_return(struct optee_domain *ctx,
>          if ( shm_rpc->xen_arg->cmd == OPTEE_RPC_CMD_SHM_ALLOC )
>              call->rpc_buffer_type = shm_rpc->xen_arg->params[0].u.value.a;
>  
> +        /*
> +         * OP-TEE is signalling that it has freed the buffer that it
> +         * requested before. This is the right time for us to do the
> +         * same.
> +         */
> +        if ( shm_rpc->xen_arg->cmd == OPTEE_RPC_CMD_SHM_FREE )
> +        {
> +            uint64_t cookie = shm_rpc->xen_arg->params[0].u.value.b;
> +
> +            free_optee_shm_buf(ctx, cookie);
> +
> +            /*
> +             * OP-TEE asks to free buffer, but this is not the same
> +             * buffer we previously allocated for it. While nothing
> +             * prevents OP-TEE from asking this, it is the strange
                                                          ^ a

> +             * situation. This may or may not be caused by a bug in
> +             * OP-TEE or mediator. But is better to print warning.
                                          ^ it is

> +             */
> +            if ( call->rpc_data_cookie && call->rpc_data_cookie != cookie )
> +            {
> +                gprintk(XENLOG_ERR,
> +                        "Saved RPC cookie does not corresponds to OP-TEE's (%"PRIx64" != %"PRIx64")\n",
                                                      ^ correspond


> +                        call->rpc_data_cookie, cookie);
> +
> +                WARN();
> +            }
> +            call->rpc_data_cookie = 0;
> +        }
>          unmap_domain_page(shm_rpc->xen_arg);
>      }
>  
> @@ -1464,10 +1492,6 @@ static void handle_rpc_cmd(struct optee_domain *ctx, struct cpu_user_regs *regs,
>              }
>              break;
>          case OPTEE_RPC_CMD_SHM_FREE:
> -            free_optee_shm_buf(ctx, shm_rpc->xen_arg->params[0].u.value.b);
> -            if ( call->rpc_data_cookie ==
> -                 shm_rpc->xen_arg->params[0].u.value.b )
> -                call->rpc_data_cookie = 0;
>              break;
>          default:
>              break;
> -- 
> 2.26.2
> 

[PATCH v2 2/2] optee: allow plain TMEM buffers with NULL address

[Volodymyr Babchuk]

[-- Attachment #1: Type: text/plain, Size: 3476 bytes --]

Trusted Applications use popular approach to determine required size
of buffer: client provides a memory reference with the NULL pointer to
a buffer. This is so called "Null memory reference". TA updates the
reference with the required size and returns it back to the
client. Then client allocates buffer of needed size and repeats the
operation.

This behavior is described in TEE Client API Specification, paragraph
3.2.5. Memory References.

OP-TEE represents this null memory reference as a TMEM parameter with
buf_ptr = 0x0. This is the only case when we should allow TMEM
buffer without the OPTEE_MSG_ATTR_NONCONTIG flag. This also the
special case for a buffer with OPTEE_MSG_ATTR_NONCONTIG flag.

This could lead to a potential issue, because IPA 0x0 is a valid
address, but OP-TEE will treat it as a special case. So, care should
be taken when construction OP-TEE enabled guest to make sure that such
guest have no memory at IPA 0x0 and none of its memory is mapped at PA
0x0.

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---

Changes from v1:
 - Added comment with TODO about possible PA/IPA 0x0 issue
 - The same is described in the commit message
 - Added check in translate_noncontig() for the NULL ptr buffer

---
 xen/arch/arm/tee/optee.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
index 6963238056..70bfef7e5f 100644
--- a/xen/arch/arm/tee/optee.c
+++ b/xen/arch/arm/tee/optee.c
@@ -215,6 +215,15 @@ static bool optee_probe(void)
     return true;
 }
 
+/*
+ * TODO: There is a potential issue with guests that either have RAM
+ * at IPA of 0x0 or some of theirs memory is mapped at PA 0x0. This is
+ * because PA of 0x0 is considered as NULL pointer by OP-TEE. It will
+ * not be able to map buffer with such pointer to TA address space, or
+ * use such buffer for communication with the guest. We either need to
+ * check that guest have no such mappings or ensure that OP-TEE
+ * enabled guest will not be created with such mappings.
+ */
 static int optee_domain_init(struct domain *d)
 {
     struct arm_smccc_res resp;
@@ -725,6 +734,15 @@ static int translate_noncontig(struct optee_domain *ctx,
         uint64_t next_page_data;
     } *guest_data, *xen_data;
 
+    /*
+     * Special case: buffer with buf_ptr == 0x0 is considered as NULL
+     * pointer by OP-TEE. No translation is needed. This can lead to
+     * an issue as IPA 0x0 is a valid address for Xen. See the comment
+     * near optee_domain_init()
+     */
+    if ( !param->u.tmem.buf_ptr )
+        return 0;
+
     /* Offset of user buffer withing OPTEE_MSG_NONCONTIG_PAGE_SIZE-sized page */
     offset = param->u.tmem.buf_ptr & (OPTEE_MSG_NONCONTIG_PAGE_SIZE - 1);
 
@@ -865,9 +883,12 @@ static int translate_params(struct optee_domain *ctx,
             }
             else
             {
-                gdprintk(XENLOG_WARNING, "Guest tries to use old tmem arg\n");
-                ret = -EINVAL;
-                goto out;
+                if ( call->xen_arg->params[i].u.tmem.buf_ptr )
+                {
+                    gdprintk(XENLOG_WARNING, "Guest tries to use old tmem arg\n");
+                    ret = -EINVAL;
+                    goto out;
+                }
             }
             break;
         case OPTEE_MSG_ATTR_TYPE_NONE:
-- 
2.26.2

[PATCH v2 2/2] optee: allow plain TMEM buffers with NULL address

[Volodymyr Babchuk]

Trusted Applications use popular approach to determine required size
of buffer: client provides a memory reference with the NULL pointer to
a buffer. This is so called "Null memory reference". TA updates the
reference with the required size and returns it back to the
client. Then client allocates buffer of needed size and repeats the
operation.

This behavior is described in TEE Client API Specification, paragraph
3.2.5. Memory References.

OP-TEE represents this null memory reference as a TMEM parameter with
buf_ptr = 0x0. This is the only case when we should allow TMEM
buffer without the OPTEE_MSG_ATTR_NONCONTIG flag. This also the
special case for a buffer with OPTEE_MSG_ATTR_NONCONTIG flag.

This could lead to a potential issue, because IPA 0x0 is a valid
address, but OP-TEE will treat it as a special case. So, care should
be taken when construction OP-TEE enabled guest to make sure that such
guest have no memory at IPA 0x0 and none of its memory is mapped at PA
0x0.

Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
---

Changes from v1:
 - Added comment with TODO about possible PA/IPA 0x0 issue
 - The same is described in the commit message
 - Added check in translate_noncontig() for the NULL ptr buffer

---
 xen/arch/arm/tee/optee.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
index 6963238056..70bfef7e5f 100644
--- a/xen/arch/arm/tee/optee.c
+++ b/xen/arch/arm/tee/optee.c
@@ -215,6 +215,15 @@ static bool optee_probe(void)
     return true;
 }
 
+/*
+ * TODO: There is a potential issue with guests that either have RAM
+ * at IPA of 0x0 or some of theirs memory is mapped at PA 0x0. This is
+ * because PA of 0x0 is considered as NULL pointer by OP-TEE. It will
+ * not be able to map buffer with such pointer to TA address space, or
+ * use such buffer for communication with the guest. We either need to
+ * check that guest have no such mappings or ensure that OP-TEE
+ * enabled guest will not be created with such mappings.
+ */
 static int optee_domain_init(struct domain *d)
 {
     struct arm_smccc_res resp;
@@ -725,6 +734,15 @@ static int translate_noncontig(struct optee_domain *ctx,
         uint64_t next_page_data;
     } *guest_data, *xen_data;
 
+    /*
+     * Special case: buffer with buf_ptr == 0x0 is considered as NULL
+     * pointer by OP-TEE. No translation is needed. This can lead to
+     * an issue as IPA 0x0 is a valid address for Xen. See the comment
+     * near optee_domain_init()
+     */
+    if ( !param->u.tmem.buf_ptr )
+        return 0;
+
     /* Offset of user buffer withing OPTEE_MSG_NONCONTIG_PAGE_SIZE-sized page */
     offset = param->u.tmem.buf_ptr & (OPTEE_MSG_NONCONTIG_PAGE_SIZE - 1);
 
@@ -865,9 +883,12 @@ static int translate_params(struct optee_domain *ctx,
             }
             else
             {
-                gdprintk(XENLOG_WARNING, "Guest tries to use old tmem arg\n");
-                ret = -EINVAL;
-                goto out;
+                if ( call->xen_arg->params[i].u.tmem.buf_ptr )
+                {
+                    gdprintk(XENLOG_WARNING, "Guest tries to use old tmem arg\n");
+                    ret = -EINVAL;
+                    goto out;
+                }
             }
             break;
         case OPTEE_MSG_ATTR_TYPE_NONE:
-- 
2.26.2

Re: [PATCH v2 2/2] optee: allow plain TMEM buffers with NULL address

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 4059 bytes --]

On Fri, 19 Jun 2020, Volodymyr Babchuk wrote:
> Trusted Applications use popular approach to determine required size
> of buffer: client provides a memory reference with the NULL pointer to
> a buffer. This is so called "Null memory reference". TA updates the
> reference with the required size and returns it back to the
> client. Then client allocates buffer of needed size and repeats the
> operation.
> 
> This behavior is described in TEE Client API Specification, paragraph
> 3.2.5. Memory References.
> 
> OP-TEE represents this null memory reference as a TMEM parameter with
> buf_ptr = 0x0. This is the only case when we should allow TMEM
> buffer without the OPTEE_MSG_ATTR_NONCONTIG flag. This also the
> special case for a buffer with OPTEE_MSG_ATTR_NONCONTIG flag.
> 
> This could lead to a potential issue, because IPA 0x0 is a valid
> address, but OP-TEE will treat it as a special case. So, care should
> be taken when construction OP-TEE enabled guest to make sure that such
> guest have no memory at IPA 0x0 and none of its memory is mapped at PA
> 0x0.
> 
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
> 
> Changes from v1:
>  - Added comment with TODO about possible PA/IPA 0x0 issue
>  - The same is described in the commit message
>  - Added check in translate_noncontig() for the NULL ptr buffer
> 
> ---
>  xen/arch/arm/tee/optee.c | 27 ++++++++++++++++++++++++---
>  1 file changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
> index 6963238056..70bfef7e5f 100644
> --- a/xen/arch/arm/tee/optee.c
> +++ b/xen/arch/arm/tee/optee.c
> @@ -215,6 +215,15 @@ static bool optee_probe(void)
>      return true;
>  }
>  
> +/*
> + * TODO: There is a potential issue with guests that either have RAM
> + * at IPA of 0x0 or some of theirs memory is mapped at PA 0x0. This is
                               ^ their

> + * because PA of 0x0 is considered as NULL pointer by OP-TEE. It will
> + * not be able to map buffer with such pointer to TA address space, or
> + * use such buffer for communication with the guest. We either need to
> + * check that guest have no such mappings or ensure that OP-TEE
> + * enabled guest will not be created with such mappings.
> + */
>  static int optee_domain_init(struct domain *d)
>  {
>      struct arm_smccc_res resp;
> @@ -725,6 +734,15 @@ static int translate_noncontig(struct optee_domain *ctx,
>          uint64_t next_page_data;
>      } *guest_data, *xen_data;
>  
> +    /*
> +     * Special case: buffer with buf_ptr == 0x0 is considered as NULL
> +     * pointer by OP-TEE. No translation is needed. This can lead to
> +     * an issue as IPA 0x0 is a valid address for Xen. See the comment
> +     * near optee_domain_init()
> +     */
> +    if ( !param->u.tmem.buf_ptr )
> +        return 0;

Given that today it is not possible for this to happen, it could even be
an ASSERT. But I think I would just return an error, maybe -EINVAL?

Aside from this, and the small grammar issue, everything else looks fine
to me.

Let's wait for Julien's reply, but if this is the only thing I could fix
on commit.


>      /* Offset of user buffer withing OPTEE_MSG_NONCONTIG_PAGE_SIZE-sized page */
>      offset = param->u.tmem.buf_ptr & (OPTEE_MSG_NONCONTIG_PAGE_SIZE - 1);
>  
> @@ -865,9 +883,12 @@ static int translate_params(struct optee_domain *ctx,
>              }
>              else
>              {
> -                gdprintk(XENLOG_WARNING, "Guest tries to use old tmem arg\n");
> -                ret = -EINVAL;
> -                goto out;
> +                if ( call->xen_arg->params[i].u.tmem.buf_ptr )
> +                {
> +                    gdprintk(XENLOG_WARNING, "Guest tries to use old tmem arg\n");
> +                    ret = -EINVAL;
> +                    goto out;
> +                }
>              }
>              break;
>          case OPTEE_MSG_ATTR_TYPE_NONE:
> -- 
> 2.26.2
> 

Re: [PATCH v2 2/2] optee: allow plain TMEM buffers with NULL address

[Stefano Stabellini]

On Fri, 19 Jun 2020, Volodymyr Babchuk wrote:
> Trusted Applications use popular approach to determine required size
> of buffer: client provides a memory reference with the NULL pointer to
> a buffer. This is so called "Null memory reference". TA updates the
> reference with the required size and returns it back to the
> client. Then client allocates buffer of needed size and repeats the
> operation.
> 
> This behavior is described in TEE Client API Specification, paragraph
> 3.2.5. Memory References.
> 
> OP-TEE represents this null memory reference as a TMEM parameter with
> buf_ptr = 0x0. This is the only case when we should allow TMEM
> buffer without the OPTEE_MSG_ATTR_NONCONTIG flag. This also the
> special case for a buffer with OPTEE_MSG_ATTR_NONCONTIG flag.
> 
> This could lead to a potential issue, because IPA 0x0 is a valid
> address, but OP-TEE will treat it as a special case. So, care should
> be taken when construction OP-TEE enabled guest to make sure that such
> guest have no memory at IPA 0x0 and none of its memory is mapped at PA
> 0x0.
> 
> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> ---
> 
> Changes from v1:
>  - Added comment with TODO about possible PA/IPA 0x0 issue
>  - The same is described in the commit message
>  - Added check in translate_noncontig() for the NULL ptr buffer
> 
> ---
>  xen/arch/arm/tee/optee.c | 27 ++++++++++++++++++++++++---
>  1 file changed, 24 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
> index 6963238056..70bfef7e5f 100644
> --- a/xen/arch/arm/tee/optee.c
> +++ b/xen/arch/arm/tee/optee.c
> @@ -215,6 +215,15 @@ static bool optee_probe(void)
>      return true;
>  }
>  
> +/*
> + * TODO: There is a potential issue with guests that either have RAM
> + * at IPA of 0x0 or some of theirs memory is mapped at PA 0x0. This is
                               ^ their

> + * because PA of 0x0 is considered as NULL pointer by OP-TEE. It will
> + * not be able to map buffer with such pointer to TA address space, or
> + * use such buffer for communication with the guest. We either need to
> + * check that guest have no such mappings or ensure that OP-TEE
> + * enabled guest will not be created with such mappings.
> + */
>  static int optee_domain_init(struct domain *d)
>  {
>      struct arm_smccc_res resp;
> @@ -725,6 +734,15 @@ static int translate_noncontig(struct optee_domain *ctx,
>          uint64_t next_page_data;
>      } *guest_data, *xen_data;
>  
> +    /*
> +     * Special case: buffer with buf_ptr == 0x0 is considered as NULL
> +     * pointer by OP-TEE. No translation is needed. This can lead to
> +     * an issue as IPA 0x0 is a valid address for Xen. See the comment
> +     * near optee_domain_init()
> +     */
> +    if ( !param->u.tmem.buf_ptr )
> +        return 0;

Given that today it is not possible for this to happen, it could even be
an ASSERT. But I think I would just return an error, maybe -EINVAL?

Aside from this, and the small grammar issue, everything else looks fine
to me.

Let's wait for Julien's reply, but if this is the only thing I could fix
on commit.


>      /* Offset of user buffer withing OPTEE_MSG_NONCONTIG_PAGE_SIZE-sized page */
>      offset = param->u.tmem.buf_ptr & (OPTEE_MSG_NONCONTIG_PAGE_SIZE - 1);
>  
> @@ -865,9 +883,12 @@ static int translate_params(struct optee_domain *ctx,
>              }
>              else
>              {
> -                gdprintk(XENLOG_WARNING, "Guest tries to use old tmem arg\n");
> -                ret = -EINVAL;
> -                goto out;
> +                if ( call->xen_arg->params[i].u.tmem.buf_ptr )
> +                {
> +                    gdprintk(XENLOG_WARNING, "Guest tries to use old tmem arg\n");
> +                    ret = -EINVAL;
> +                    goto out;
> +                }
>              }
>              break;
>          case OPTEE_MSG_ATTR_TYPE_NONE:
> -- 
> 2.26.2
> 

Re: [PATCH v2 2/2] optee: allow plain TMEM buffers with NULL address

[Volodymyr Babchuk]

[-- Attachment #1: Type: text/plain, Size: 4667 bytes --]


Hi Stefano,

Stefano Stabellini writes:

> On Fri, 19 Jun 2020, Volodymyr Babchuk wrote:
>> Trusted Applications use popular approach to determine required size
>> of buffer: client provides a memory reference with the NULL pointer to
>> a buffer. This is so called "Null memory reference". TA updates the
>> reference with the required size and returns it back to the
>> client. Then client allocates buffer of needed size and repeats the
>> operation.
>> 
>> This behavior is described in TEE Client API Specification, paragraph
>> 3.2.5. Memory References.
>> 
>> OP-TEE represents this null memory reference as a TMEM parameter with
>> buf_ptr = 0x0. This is the only case when we should allow TMEM
>> buffer without the OPTEE_MSG_ATTR_NONCONTIG flag. This also the
>> special case for a buffer with OPTEE_MSG_ATTR_NONCONTIG flag.
>> 
>> This could lead to a potential issue, because IPA 0x0 is a valid
>> address, but OP-TEE will treat it as a special case. So, care should
>> be taken when construction OP-TEE enabled guest to make sure that such
>> guest have no memory at IPA 0x0 and none of its memory is mapped at PA
>> 0x0.
>> 
>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>> ---
>> 
>> Changes from v1:
>>  - Added comment with TODO about possible PA/IPA 0x0 issue
>>  - The same is described in the commit message
>>  - Added check in translate_noncontig() for the NULL ptr buffer
>> 
>> ---
>>  xen/arch/arm/tee/optee.c | 27 ++++++++++++++++++++++++---
>>  1 file changed, 24 insertions(+), 3 deletions(-)
>> 
>> diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
>> index 6963238056..70bfef7e5f 100644
>> --- a/xen/arch/arm/tee/optee.c
>> +++ b/xen/arch/arm/tee/optee.c
>> @@ -215,6 +215,15 @@ static bool optee_probe(void)
>>      return true;
>>  }
>>  
>> +/*
>> + * TODO: There is a potential issue with guests that either have RAM
>> + * at IPA of 0x0 or some of theirs memory is mapped at PA 0x0. This is
>                                ^ their
>
>> + * because PA of 0x0 is considered as NULL pointer by OP-TEE. It will
>> + * not be able to map buffer with such pointer to TA address space, or
>> + * use such buffer for communication with the guest. We either need to
>> + * check that guest have no such mappings or ensure that OP-TEE
>> + * enabled guest will not be created with such mappings.
>> + */
>>  static int optee_domain_init(struct domain *d)
>>  {
>>      struct arm_smccc_res resp;
>> @@ -725,6 +734,15 @@ static int translate_noncontig(struct optee_domain *ctx,
>>          uint64_t next_page_data;
>>      } *guest_data, *xen_data;
>>  
>> +    /*
>> +     * Special case: buffer with buf_ptr == 0x0 is considered as NULL
>> +     * pointer by OP-TEE. No translation is needed. This can lead to
>> +     * an issue as IPA 0x0 is a valid address for Xen. See the comment
>> +     * near optee_domain_init()
>> +     */
>> +    if ( !param->u.tmem.buf_ptr )
>> +        return 0;
>
> Given that today it is not possible for this to happen, it could even be
> an ASSERT. But I think I would just return an error, maybe -EINVAL?

Hmm, looks like my comment is somewhat misleading :(

What I mean, is that param->u.tmem.buf_ptr == 0 is the normal situation.
This is the special case, when OP-TEE treats this buffer as a NULL. So
we are doing nothing there. Thus, "return 0".

But, as Julien pointed out, we can have machine where 0x0 is the valid
memory address and there is a chance, that some guest will use it as a
pointer to buffer.

> Aside from this, and the small grammar issue, everything else looks fine
> to me.
>
> Let's wait for Julien's reply, but if this is the only thing I could fix
> on commit.
>
>
>>      /* Offset of user buffer withing OPTEE_MSG_NONCONTIG_PAGE_SIZE-sized page */
>>      offset = param->u.tmem.buf_ptr & (OPTEE_MSG_NONCONTIG_PAGE_SIZE - 1);
>>  
>> @@ -865,9 +883,12 @@ static int translate_params(struct optee_domain *ctx,
>>              }
>>              else
>>              {
>> -                gdprintk(XENLOG_WARNING, "Guest tries to use old tmem arg\n");
>> -                ret = -EINVAL;
>> -                goto out;
>> +                if ( call->xen_arg->params[i].u.tmem.buf_ptr )
>> +                {
>> +                    gdprintk(XENLOG_WARNING, "Guest tries to use old tmem arg\n");
>> +                    ret = -EINVAL;
>> +                    goto out;
>> +                }
>>              }
>>              break;
>>          case OPTEE_MSG_ATTR_TYPE_NONE:
>> -- 
>> 2.26.2
>> 


-- 
Volodymyr Babchuk at EPAM

Re: [PATCH v2 2/2] optee: allow plain TMEM buffers with NULL address

[Volodymyr Babchuk]

Hi Stefano,

Stefano Stabellini writes:

> On Fri, 19 Jun 2020, Volodymyr Babchuk wrote:
>> Trusted Applications use popular approach to determine required size
>> of buffer: client provides a memory reference with the NULL pointer to
>> a buffer. This is so called "Null memory reference". TA updates the
>> reference with the required size and returns it back to the
>> client. Then client allocates buffer of needed size and repeats the
>> operation.
>> 
>> This behavior is described in TEE Client API Specification, paragraph
>> 3.2.5. Memory References.
>> 
>> OP-TEE represents this null memory reference as a TMEM parameter with
>> buf_ptr = 0x0. This is the only case when we should allow TMEM
>> buffer without the OPTEE_MSG_ATTR_NONCONTIG flag. This also the
>> special case for a buffer with OPTEE_MSG_ATTR_NONCONTIG flag.
>> 
>> This could lead to a potential issue, because IPA 0x0 is a valid
>> address, but OP-TEE will treat it as a special case. So, care should
>> be taken when construction OP-TEE enabled guest to make sure that such
>> guest have no memory at IPA 0x0 and none of its memory is mapped at PA
>> 0x0.
>> 
>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>> ---
>> 
>> Changes from v1:
>>  - Added comment with TODO about possible PA/IPA 0x0 issue
>>  - The same is described in the commit message
>>  - Added check in translate_noncontig() for the NULL ptr buffer
>> 
>> ---
>>  xen/arch/arm/tee/optee.c | 27 ++++++++++++++++++++++++---
>>  1 file changed, 24 insertions(+), 3 deletions(-)
>> 
>> diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
>> index 6963238056..70bfef7e5f 100644
>> --- a/xen/arch/arm/tee/optee.c
>> +++ b/xen/arch/arm/tee/optee.c
>> @@ -215,6 +215,15 @@ static bool optee_probe(void)
>>      return true;
>>  }
>>  
>> +/*
>> + * TODO: There is a potential issue with guests that either have RAM
>> + * at IPA of 0x0 or some of theirs memory is mapped at PA 0x0. This is
>                                ^ their
>
>> + * because PA of 0x0 is considered as NULL pointer by OP-TEE. It will
>> + * not be able to map buffer with such pointer to TA address space, or
>> + * use such buffer for communication with the guest. We either need to
>> + * check that guest have no such mappings or ensure that OP-TEE
>> + * enabled guest will not be created with such mappings.
>> + */
>>  static int optee_domain_init(struct domain *d)
>>  {
>>      struct arm_smccc_res resp;
>> @@ -725,6 +734,15 @@ static int translate_noncontig(struct optee_domain *ctx,
>>          uint64_t next_page_data;
>>      } *guest_data, *xen_data;
>>  
>> +    /*
>> +     * Special case: buffer with buf_ptr == 0x0 is considered as NULL
>> +     * pointer by OP-TEE. No translation is needed. This can lead to
>> +     * an issue as IPA 0x0 is a valid address for Xen. See the comment
>> +     * near optee_domain_init()
>> +     */
>> +    if ( !param->u.tmem.buf_ptr )
>> +        return 0;
>
> Given that today it is not possible for this to happen, it could even be
> an ASSERT. But I think I would just return an error, maybe -EINVAL?

Hmm, looks like my comment is somewhat misleading :(

What I mean, is that param->u.tmem.buf_ptr == 0 is the normal situation.
This is the special case, when OP-TEE treats this buffer as a NULL. So
we are doing nothing there. Thus, "return 0".

But, as Julien pointed out, we can have machine where 0x0 is the valid
memory address and there is a chance, that some guest will use it as a
pointer to buffer.

> Aside from this, and the small grammar issue, everything else looks fine
> to me.
>
> Let's wait for Julien's reply, but if this is the only thing I could fix
> on commit.
>
>
>>      /* Offset of user buffer withing OPTEE_MSG_NONCONTIG_PAGE_SIZE-sized page */
>>      offset = param->u.tmem.buf_ptr & (OPTEE_MSG_NONCONTIG_PAGE_SIZE - 1);
>>  
>> @@ -865,9 +883,12 @@ static int translate_params(struct optee_domain *ctx,
>>              }
>>              else
>>              {
>> -                gdprintk(XENLOG_WARNING, "Guest tries to use old tmem arg\n");
>> -                ret = -EINVAL;
>> -                goto out;
>> +                if ( call->xen_arg->params[i].u.tmem.buf_ptr )
>> +                {
>> +                    gdprintk(XENLOG_WARNING, "Guest tries to use old tmem arg\n");
>> +                    ret = -EINVAL;
>> +                    goto out;
>> +                }
>>              }
>>              break;
>>          case OPTEE_MSG_ATTR_TYPE_NONE:
>> -- 
>> 2.26.2
>> 


-- 
Volodymyr Babchuk at EPAM

Re: [PATCH v2 2/2] optee: allow plain TMEM buffers with NULL address

[Julien Grall]

[-- Attachment #1: Type: text/plain, Size: 4435 bytes --]



On 23/06/2020 03:49, Volodymyr Babchuk wrote:
> 
> Hi Stefano,
> 
> Stefano Stabellini writes:
> 
>> On Fri, 19 Jun 2020, Volodymyr Babchuk wrote:
>>> Trusted Applications use popular approach to determine required size
>>> of buffer: client provides a memory reference with the NULL pointer to
>>> a buffer. This is so called "Null memory reference". TA updates the
>>> reference with the required size and returns it back to the
>>> client. Then client allocates buffer of needed size and repeats the
>>> operation.
>>>
>>> This behavior is described in TEE Client API Specification, paragraph
>>> 3.2.5. Memory References.
>>>
>>> OP-TEE represents this null memory reference as a TMEM parameter with
>>> buf_ptr = 0x0. This is the only case when we should allow TMEM
>>> buffer without the OPTEE_MSG_ATTR_NONCONTIG flag. This also the
>>> special case for a buffer with OPTEE_MSG_ATTR_NONCONTIG flag.
>>>
>>> This could lead to a potential issue, because IPA 0x0 is a valid
>>> address, but OP-TEE will treat it as a special case. So, care should
>>> be taken when construction OP-TEE enabled guest to make sure that such
>>> guest have no memory at IPA 0x0 and none of its memory is mapped at PA
>>> 0x0.
>>>
>>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>>> ---
>>>
>>> Changes from v1:
>>>   - Added comment with TODO about possible PA/IPA 0x0 issue
>>>   - The same is described in the commit message
>>>   - Added check in translate_noncontig() for the NULL ptr buffer
>>>
>>> ---
>>>   xen/arch/arm/tee/optee.c | 27 ++++++++++++++++++++++++---
>>>   1 file changed, 24 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
>>> index 6963238056..70bfef7e5f 100644
>>> --- a/xen/arch/arm/tee/optee.c
>>> +++ b/xen/arch/arm/tee/optee.c
>>> @@ -215,6 +215,15 @@ static bool optee_probe(void)
>>>       return true;
>>>   }
>>>   
>>> +/*
>>> + * TODO: There is a potential issue with guests that either have RAM
>>> + * at IPA of 0x0 or some of theirs memory is mapped at PA 0x0. This is
>>                                 ^ their
>>
>>> + * because PA of 0x0 is considered as NULL pointer by OP-TEE. It will
>>> + * not be able to map buffer with such pointer to TA address space, or
>>> + * use such buffer for communication with the guest. We either need to
>>> + * check that guest have no such mappings or ensure that OP-TEE
>>> + * enabled guest will not be created with such mappings.
>>> + */
>>>   static int optee_domain_init(struct domain *d)
>>>   {
>>>       struct arm_smccc_res resp;
>>> @@ -725,6 +734,15 @@ static int translate_noncontig(struct optee_domain *ctx,
>>>           uint64_t next_page_data;
>>>       } *guest_data, *xen_data;
>>>   
>>> +    /*
>>> +     * Special case: buffer with buf_ptr == 0x0 is considered as NULL
>>> +     * pointer by OP-TEE. No translation is needed. This can lead to
>>> +     * an issue as IPA 0x0 is a valid address for Xen. See the comment
>>> +     * near optee_domain_init()
>>> +     */
>>> +    if ( !param->u.tmem.buf_ptr )
>>> +        return 0;
>>
>> Given that today it is not possible for this to happen, it could even be
>> an ASSERT. But I think I would just return an error, maybe -EINVAL?
> 
> Hmm, looks like my comment is somewhat misleading :(

How about the following comment:

We don't want to translate NULL (0) as it can be used by the guest to 
fetch the size of the buffer to allocate. This behavior depends on TA, 
but there is a guarantee that OP-TEE will not try to map it (see more 
details on top of optee_domain_init()).

> 
> What I mean, is that param->u.tmem.buf_ptr == 0 is the normal situation.
> This is the special case, when OP-TEE treats this buffer as a NULL. So
> we are doing nothing there. Thus, "return 0".
> 
> But, as Julien pointed out, we can have machine where 0x0 is the valid
> memory address and there is a chance, that some guest will use it as a
> pointer to buffer.
> 
>> Aside from this, and the small grammar issue, everything else looks fine
>> to me.
>>
>> Let's wait for Julien's reply, but if this is the only thing I could fix
>> on commit.

I agree with Volodymyr, this is the normal case here. There are more 
work to prevent MFN 0 to be mapped in the guest but this shouldn't be an 
issue today.

Cheers,

-- 
Julien Grall

Re: [PATCH v2 2/2] optee: allow plain TMEM buffers with NULL address

[Julien Grall]

On 23/06/2020 03:49, Volodymyr Babchuk wrote:
> 
> Hi Stefano,
> 
> Stefano Stabellini writes:
> 
>> On Fri, 19 Jun 2020, Volodymyr Babchuk wrote:
>>> Trusted Applications use popular approach to determine required size
>>> of buffer: client provides a memory reference with the NULL pointer to
>>> a buffer. This is so called "Null memory reference". TA updates the
>>> reference with the required size and returns it back to the
>>> client. Then client allocates buffer of needed size and repeats the
>>> operation.
>>>
>>> This behavior is described in TEE Client API Specification, paragraph
>>> 3.2.5. Memory References.
>>>
>>> OP-TEE represents this null memory reference as a TMEM parameter with
>>> buf_ptr = 0x0. This is the only case when we should allow TMEM
>>> buffer without the OPTEE_MSG_ATTR_NONCONTIG flag. This also the
>>> special case for a buffer with OPTEE_MSG_ATTR_NONCONTIG flag.
>>>
>>> This could lead to a potential issue, because IPA 0x0 is a valid
>>> address, but OP-TEE will treat it as a special case. So, care should
>>> be taken when construction OP-TEE enabled guest to make sure that such
>>> guest have no memory at IPA 0x0 and none of its memory is mapped at PA
>>> 0x0.
>>>
>>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>>> ---
>>>
>>> Changes from v1:
>>>   - Added comment with TODO about possible PA/IPA 0x0 issue
>>>   - The same is described in the commit message
>>>   - Added check in translate_noncontig() for the NULL ptr buffer
>>>
>>> ---
>>>   xen/arch/arm/tee/optee.c | 27 ++++++++++++++++++++++++---
>>>   1 file changed, 24 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
>>> index 6963238056..70bfef7e5f 100644
>>> --- a/xen/arch/arm/tee/optee.c
>>> +++ b/xen/arch/arm/tee/optee.c
>>> @@ -215,6 +215,15 @@ static bool optee_probe(void)
>>>       return true;
>>>   }
>>>   
>>> +/*
>>> + * TODO: There is a potential issue with guests that either have RAM
>>> + * at IPA of 0x0 or some of theirs memory is mapped at PA 0x0. This is
>>                                 ^ their
>>
>>> + * because PA of 0x0 is considered as NULL pointer by OP-TEE. It will
>>> + * not be able to map buffer with such pointer to TA address space, or
>>> + * use such buffer for communication with the guest. We either need to
>>> + * check that guest have no such mappings or ensure that OP-TEE
>>> + * enabled guest will not be created with such mappings.
>>> + */
>>>   static int optee_domain_init(struct domain *d)
>>>   {
>>>       struct arm_smccc_res resp;
>>> @@ -725,6 +734,15 @@ static int translate_noncontig(struct optee_domain *ctx,
>>>           uint64_t next_page_data;
>>>       } *guest_data, *xen_data;
>>>   
>>> +    /*
>>> +     * Special case: buffer with buf_ptr == 0x0 is considered as NULL
>>> +     * pointer by OP-TEE. No translation is needed. This can lead to
>>> +     * an issue as IPA 0x0 is a valid address for Xen. See the comment
>>> +     * near optee_domain_init()
>>> +     */
>>> +    if ( !param->u.tmem.buf_ptr )
>>> +        return 0;
>>
>> Given that today it is not possible for this to happen, it could even be
>> an ASSERT. But I think I would just return an error, maybe -EINVAL?
> 
> Hmm, looks like my comment is somewhat misleading :(

How about the following comment:

We don't want to translate NULL (0) as it can be used by the guest to 
fetch the size of the buffer to allocate. This behavior depends on TA, 
but there is a guarantee that OP-TEE will not try to map it (see more 
details on top of optee_domain_init()).

> 
> What I mean, is that param->u.tmem.buf_ptr == 0 is the normal situation.
> This is the special case, when OP-TEE treats this buffer as a NULL. So
> we are doing nothing there. Thus, "return 0".
> 
> But, as Julien pointed out, we can have machine where 0x0 is the valid
> memory address and there is a chance, that some guest will use it as a
> pointer to buffer.
> 
>> Aside from this, and the small grammar issue, everything else looks fine
>> to me.
>>
>> Let's wait for Julien's reply, but if this is the only thing I could fix
>> on commit.

I agree with Volodymyr, this is the normal case here. There are more 
work to prevent MFN 0 to be mapped in the guest but this shouldn't be an 
issue today.

Cheers,

-- 
Julien Grall

Re: [PATCH v2 2/2] optee: allow plain TMEM buffers with NULL address

[Stefano Stabellini]

[-- Attachment #1: Type: text/plain, Size: 5443 bytes --]

On Tue, 23 Jun 2020, Julien Grall wrote:
> On 23/06/2020 03:49, Volodymyr Babchuk wrote:
> > 
> > Hi Stefano,
> > 
> > Stefano Stabellini writes:
> > 
> > > On Fri, 19 Jun 2020, Volodymyr Babchuk wrote:
> > > > Trusted Applications use popular approach to determine required size
> > > > of buffer: client provides a memory reference with the NULL pointer to
> > > > a buffer. This is so called "Null memory reference". TA updates the
> > > > reference with the required size and returns it back to the
> > > > client. Then client allocates buffer of needed size and repeats the
> > > > operation.
> > > > 
> > > > This behavior is described in TEE Client API Specification, paragraph
> > > > 3.2.5. Memory References.
> > > > 
> > > > OP-TEE represents this null memory reference as a TMEM parameter with
> > > > buf_ptr = 0x0. This is the only case when we should allow TMEM
> > > > buffer without the OPTEE_MSG_ATTR_NONCONTIG flag. This also the
> > > > special case for a buffer with OPTEE_MSG_ATTR_NONCONTIG flag.
> > > > 
> > > > This could lead to a potential issue, because IPA 0x0 is a valid
> > > > address, but OP-TEE will treat it as a special case. So, care should
> > > > be taken when construction OP-TEE enabled guest to make sure that such
> > > > guest have no memory at IPA 0x0 and none of its memory is mapped at PA
> > > > 0x0.
> > > > 
> > > > Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> > > > ---
> > > > 
> > > > Changes from v1:
> > > >   - Added comment with TODO about possible PA/IPA 0x0 issue
> > > >   - The same is described in the commit message
> > > >   - Added check in translate_noncontig() for the NULL ptr buffer
> > > > 
> > > > ---
> > > >   xen/arch/arm/tee/optee.c | 27 ++++++++++++++++++++++++---
> > > >   1 file changed, 24 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
> > > > index 6963238056..70bfef7e5f 100644
> > > > --- a/xen/arch/arm/tee/optee.c
> > > > +++ b/xen/arch/arm/tee/optee.c
> > > > @@ -215,6 +215,15 @@ static bool optee_probe(void)
> > > >       return true;
> > > >   }
> > > >   +/*
> > > > + * TODO: There is a potential issue with guests that either have RAM
> > > > + * at IPA of 0x0 or some of theirs memory is mapped at PA 0x0. This is
> > >                                 ^ their
> > > 
> > > > + * because PA of 0x0 is considered as NULL pointer by OP-TEE. It will
> > > > + * not be able to map buffer with such pointer to TA address space, or
> > > > + * use such buffer for communication with the guest. We either need to
> > > > + * check that guest have no such mappings or ensure that OP-TEE
> > > > + * enabled guest will not be created with such mappings.
> > > > + */
> > > >   static int optee_domain_init(struct domain *d)
> > > >   {
> > > >       struct arm_smccc_res resp;
> > > > @@ -725,6 +734,15 @@ static int translate_noncontig(struct optee_domain
> > > > *ctx,
> > > >           uint64_t next_page_data;
> > > >       } *guest_data, *xen_data;
> > > >   +    /*
> > > > +     * Special case: buffer with buf_ptr == 0x0 is considered as NULL
> > > > +     * pointer by OP-TEE. No translation is needed. This can lead to
> > > > +     * an issue as IPA 0x0 is a valid address for Xen. See the comment
> > > > +     * near optee_domain_init()
> > > > +     */
> > > > +    if ( !param->u.tmem.buf_ptr )
> > > > +        return 0;
> > > 
> > > Given that today it is not possible for this to happen, it could even be
> > > an ASSERT. But I think I would just return an error, maybe -EINVAL?
> > 
> > Hmm, looks like my comment is somewhat misleading :(
> 
> How about the following comment:
> 
> We don't want to translate NULL (0) as it can be used by the guest to fetch
> the size of the buffer to allocate. This behavior depends on TA, but there is
> a guarantee that OP-TEE will not try to map it (see more details on top of
> optee_domain_init()).
> 
> > 
> > What I mean, is that param->u.tmem.buf_ptr == 0 is the normal situation.
> > This is the special case, when OP-TEE treats this buffer as a NULL. So
> > we are doing nothing there. Thus, "return 0".
> > 
> > But, as Julien pointed out, we can have machine where 0x0 is the valid
> > memory address and there is a chance, that some guest will use it as a
> > pointer to buffer.
> > 
> > > Aside from this, and the small grammar issue, everything else looks fine
> > > to me.
> > > 
> > > Let's wait for Julien's reply, but if this is the only thing I could fix
> > > on commit.
> 
> I agree with Volodymyr, this is the normal case here. There are more work to
> prevent MFN 0 to be mapped in the guest but this shouldn't be an issue today.

Let's put the MFN 0 issue aside for a moment.

From the commit message I thought that if the guest wanted to pass a
NULL buffer ("Null memory reference") then it would also *not* set
OPTEE_MSG_ATTR_NONCONTIG, which would be handled by the "else" statement
also modified by this patch. Thus, I thought that reaching
translate_noncontig with buf_ptr == NULL would always be an error.

But re-reading the commit message and from both your answers it is not
the case: a "Null memory reference" is allowed with
OPTEE_MSG_ATTR_NONCONTIG set too.

Thus, I have no further comments and the improvements on the in-code
comment could be done on commit. 

Re: [PATCH v2 2/2] optee: allow plain TMEM buffers with NULL address

[Stefano Stabellini]

On Tue, 23 Jun 2020, Julien Grall wrote:
> On 23/06/2020 03:49, Volodymyr Babchuk wrote:
> > 
> > Hi Stefano,
> > 
> > Stefano Stabellini writes:
> > 
> > > On Fri, 19 Jun 2020, Volodymyr Babchuk wrote:
> > > > Trusted Applications use popular approach to determine required size
> > > > of buffer: client provides a memory reference with the NULL pointer to
> > > > a buffer. This is so called "Null memory reference". TA updates the
> > > > reference with the required size and returns it back to the
> > > > client. Then client allocates buffer of needed size and repeats the
> > > > operation.
> > > > 
> > > > This behavior is described in TEE Client API Specification, paragraph
> > > > 3.2.5. Memory References.
> > > > 
> > > > OP-TEE represents this null memory reference as a TMEM parameter with
> > > > buf_ptr = 0x0. This is the only case when we should allow TMEM
> > > > buffer without the OPTEE_MSG_ATTR_NONCONTIG flag. This also the
> > > > special case for a buffer with OPTEE_MSG_ATTR_NONCONTIG flag.
> > > > 
> > > > This could lead to a potential issue, because IPA 0x0 is a valid
> > > > address, but OP-TEE will treat it as a special case. So, care should
> > > > be taken when construction OP-TEE enabled guest to make sure that such
> > > > guest have no memory at IPA 0x0 and none of its memory is mapped at PA
> > > > 0x0.
> > > > 
> > > > Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> > > > ---
> > > > 
> > > > Changes from v1:
> > > >   - Added comment with TODO about possible PA/IPA 0x0 issue
> > > >   - The same is described in the commit message
> > > >   - Added check in translate_noncontig() for the NULL ptr buffer
> > > > 
> > > > ---
> > > >   xen/arch/arm/tee/optee.c | 27 ++++++++++++++++++++++++---
> > > >   1 file changed, 24 insertions(+), 3 deletions(-)
> > > > 
> > > > diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
> > > > index 6963238056..70bfef7e5f 100644
> > > > --- a/xen/arch/arm/tee/optee.c
> > > > +++ b/xen/arch/arm/tee/optee.c
> > > > @@ -215,6 +215,15 @@ static bool optee_probe(void)
> > > >       return true;
> > > >   }
> > > >   +/*
> > > > + * TODO: There is a potential issue with guests that either have RAM
> > > > + * at IPA of 0x0 or some of theirs memory is mapped at PA 0x0. This is
> > >                                 ^ their
> > > 
> > > > + * because PA of 0x0 is considered as NULL pointer by OP-TEE. It will
> > > > + * not be able to map buffer with such pointer to TA address space, or
> > > > + * use such buffer for communication with the guest. We either need to
> > > > + * check that guest have no such mappings or ensure that OP-TEE
> > > > + * enabled guest will not be created with such mappings.
> > > > + */
> > > >   static int optee_domain_init(struct domain *d)
> > > >   {
> > > >       struct arm_smccc_res resp;
> > > > @@ -725,6 +734,15 @@ static int translate_noncontig(struct optee_domain
> > > > *ctx,
> > > >           uint64_t next_page_data;
> > > >       } *guest_data, *xen_data;
> > > >   +    /*
> > > > +     * Special case: buffer with buf_ptr == 0x0 is considered as NULL
> > > > +     * pointer by OP-TEE. No translation is needed. This can lead to
> > > > +     * an issue as IPA 0x0 is a valid address for Xen. See the comment
> > > > +     * near optee_domain_init()
> > > > +     */
> > > > +    if ( !param->u.tmem.buf_ptr )
> > > > +        return 0;
> > > 
> > > Given that today it is not possible for this to happen, it could even be
> > > an ASSERT. But I think I would just return an error, maybe -EINVAL?
> > 
> > Hmm, looks like my comment is somewhat misleading :(
> 
> How about the following comment:
> 
> We don't want to translate NULL (0) as it can be used by the guest to fetch
> the size of the buffer to allocate. This behavior depends on TA, but there is
> a guarantee that OP-TEE will not try to map it (see more details on top of
> optee_domain_init()).
> 
> > 
> > What I mean, is that param->u.tmem.buf_ptr == 0 is the normal situation.
> > This is the special case, when OP-TEE treats this buffer as a NULL. So
> > we are doing nothing there. Thus, "return 0".
> > 
> > But, as Julien pointed out, we can have machine where 0x0 is the valid
> > memory address and there is a chance, that some guest will use it as a
> > pointer to buffer.
> > 
> > > Aside from this, and the small grammar issue, everything else looks fine
> > > to me.
> > > 
> > > Let's wait for Julien's reply, but if this is the only thing I could fix
> > > on commit.
> 
> I agree with Volodymyr, this is the normal case here. There are more work to
> prevent MFN 0 to be mapped in the guest but this shouldn't be an issue today.

Let's put the MFN 0 issue aside for a moment.

From the commit message I thought that if the guest wanted to pass a
NULL buffer ("Null memory reference") then it would also *not* set
OPTEE_MSG_ATTR_NONCONTIG, which would be handled by the "else" statement
also modified by this patch. Thus, I thought that reaching
translate_noncontig with buf_ptr == NULL would always be an error.

But re-reading the commit message and from both your answers it is not
the case: a "Null memory reference" is allowed with
OPTEE_MSG_ATTR_NONCONTIG set too.

Thus, I have no further comments and the improvements on the in-code
comment could be done on commit. 

Re: [PATCH v2 2/2] optee: allow plain TMEM buffers with NULL address

[Julien Grall]

[-- Attachment #1: Type: text/plain, Size: 5618 bytes --]

(using paul xen.org's email)

Hi,

Apologies for the late answer.

On 23/06/2020 22:09, Stefano Stabellini wrote:
> On Tue, 23 Jun 2020, Julien Grall wrote:
>> On 23/06/2020 03:49, Volodymyr Babchuk wrote:
>>>
>>> Hi Stefano,
>>>
>>> Stefano Stabellini writes:
>>>
>>>> On Fri, 19 Jun 2020, Volodymyr Babchuk wrote:
>>>>> Trusted Applications use popular approach to determine required size
>>>>> of buffer: client provides a memory reference with the NULL pointer to
>>>>> a buffer. This is so called "Null memory reference". TA updates the
>>>>> reference with the required size and returns it back to the
>>>>> client. Then client allocates buffer of needed size and repeats the
>>>>> operation.
>>>>>
>>>>> This behavior is described in TEE Client API Specification, paragraph
>>>>> 3.2.5. Memory References.
>>>>>
>>>>> OP-TEE represents this null memory reference as a TMEM parameter with
>>>>> buf_ptr = 0x0. This is the only case when we should allow TMEM
>>>>> buffer without the OPTEE_MSG_ATTR_NONCONTIG flag. This also the
>>>>> special case for a buffer with OPTEE_MSG_ATTR_NONCONTIG flag.
>>>>>
>>>>> This could lead to a potential issue, because IPA 0x0 is a valid
>>>>> address, but OP-TEE will treat it as a special case. So, care should
>>>>> be taken when construction OP-TEE enabled guest to make sure that such
>>>>> guest have no memory at IPA 0x0 and none of its memory is mapped at PA
>>>>> 0x0.
>>>>>
>>>>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>>>>> ---
>>>>>
>>>>> Changes from v1:
>>>>>    - Added comment with TODO about possible PA/IPA 0x0 issue
>>>>>    - The same is described in the commit message
>>>>>    - Added check in translate_noncontig() for the NULL ptr buffer
>>>>>
>>>>> ---
>>>>>    xen/arch/arm/tee/optee.c | 27 ++++++++++++++++++++++++---
>>>>>    1 file changed, 24 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
>>>>> index 6963238056..70bfef7e5f 100644
>>>>> --- a/xen/arch/arm/tee/optee.c
>>>>> +++ b/xen/arch/arm/tee/optee.c
>>>>> @@ -215,6 +215,15 @@ static bool optee_probe(void)
>>>>>        return true;
>>>>>    }
>>>>>    +/*
>>>>> + * TODO: There is a potential issue with guests that either have RAM
>>>>> + * at IPA of 0x0 or some of theirs memory is mapped at PA 0x0. This is
>>>>                                  ^ their
>>>>
>>>>> + * because PA of 0x0 is considered as NULL pointer by OP-TEE. It will
>>>>> + * not be able to map buffer with such pointer to TA address space, or
>>>>> + * use such buffer for communication with the guest. We either need to
>>>>> + * check that guest have no such mappings or ensure that OP-TEE
>>>>> + * enabled guest will not be created with such mappings.
>>>>> + */
>>>>>    static int optee_domain_init(struct domain *d)
>>>>>    {
>>>>>        struct arm_smccc_res resp;
>>>>> @@ -725,6 +734,15 @@ static int translate_noncontig(struct optee_domain
>>>>> *ctx,
>>>>>            uint64_t next_page_data;
>>>>>        } *guest_data, *xen_data;
>>>>>    +    /*
>>>>> +     * Special case: buffer with buf_ptr == 0x0 is considered as NULL
>>>>> +     * pointer by OP-TEE. No translation is needed. This can lead to
>>>>> +     * an issue as IPA 0x0 is a valid address for Xen. See the comment
>>>>> +     * near optee_domain_init()
>>>>> +     */
>>>>> +    if ( !param->u.tmem.buf_ptr )
>>>>> +        return 0;
>>>>
>>>> Given that today it is not possible for this to happen, it could even be
>>>> an ASSERT. But I think I would just return an error, maybe -EINVAL?
>>>
>>> Hmm, looks like my comment is somewhat misleading :(
>>
>> How about the following comment:
>>
>> We don't want to translate NULL (0) as it can be used by the guest to fetch
>> the size of the buffer to allocate. This behavior depends on TA, but there is
>> a guarantee that OP-TEE will not try to map it (see more details on top of
>> optee_domain_init()).
>>
>>>
>>> What I mean, is that param->u.tmem.buf_ptr == 0 is the normal situation.
>>> This is the special case, when OP-TEE treats this buffer as a NULL. So
>>> we are doing nothing there. Thus, "return 0".
>>>
>>> But, as Julien pointed out, we can have machine where 0x0 is the valid
>>> memory address and there is a chance, that some guest will use it as a
>>> pointer to buffer.
>>>
>>>> Aside from this, and the small grammar issue, everything else looks fine
>>>> to me.
>>>>
>>>> Let's wait for Julien's reply, but if this is the only thing I could fix
>>>> on commit.
>>
>> I agree with Volodymyr, this is the normal case here. There are more work to
>> prevent MFN 0 to be mapped in the guest but this shouldn't be an issue today.
> 
> Let's put the MFN 0 issue aside for a moment.
> 
>  From the commit message I thought that if the guest wanted to pass a
> NULL buffer ("Null memory reference") then it would also *not* set
> OPTEE_MSG_ATTR_NONCONTIG, which would be handled by the "else" statement
> also modified by this patch. Thus, I thought that reaching
> translate_noncontig with buf_ptr == NULL would always be an error.
> 
> But re-reading the commit message and from both your answers it is not
> the case: a "Null memory reference" is allowed with
> OPTEE_MSG_ATTR_NONCONTIG set too.
> 
> Thus, I have no further comments and the improvements on the in-code
> comment could be done on commit.

Good :). IIRC Paul gave a provisional RaB for this series. @Paul, now 
that we are settled, could we get a formal one?

Cheers,

-- 
Julien Grall

Re: [PATCH v2 2/2] optee: allow plain TMEM buffers with NULL address

[Julien Grall]

(using paul xen.org's email)

Hi,

Apologies for the late answer.

On 23/06/2020 22:09, Stefano Stabellini wrote:
> On Tue, 23 Jun 2020, Julien Grall wrote:
>> On 23/06/2020 03:49, Volodymyr Babchuk wrote:
>>>
>>> Hi Stefano,
>>>
>>> Stefano Stabellini writes:
>>>
>>>> On Fri, 19 Jun 2020, Volodymyr Babchuk wrote:
>>>>> Trusted Applications use popular approach to determine required size
>>>>> of buffer: client provides a memory reference with the NULL pointer to
>>>>> a buffer. This is so called "Null memory reference". TA updates the
>>>>> reference with the required size and returns it back to the
>>>>> client. Then client allocates buffer of needed size and repeats the
>>>>> operation.
>>>>>
>>>>> This behavior is described in TEE Client API Specification, paragraph
>>>>> 3.2.5. Memory References.
>>>>>
>>>>> OP-TEE represents this null memory reference as a TMEM parameter with
>>>>> buf_ptr = 0x0. This is the only case when we should allow TMEM
>>>>> buffer without the OPTEE_MSG_ATTR_NONCONTIG flag. This also the
>>>>> special case for a buffer with OPTEE_MSG_ATTR_NONCONTIG flag.
>>>>>
>>>>> This could lead to a potential issue, because IPA 0x0 is a valid
>>>>> address, but OP-TEE will treat it as a special case. So, care should
>>>>> be taken when construction OP-TEE enabled guest to make sure that such
>>>>> guest have no memory at IPA 0x0 and none of its memory is mapped at PA
>>>>> 0x0.
>>>>>
>>>>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>>>>> ---
>>>>>
>>>>> Changes from v1:
>>>>>    - Added comment with TODO about possible PA/IPA 0x0 issue
>>>>>    - The same is described in the commit message
>>>>>    - Added check in translate_noncontig() for the NULL ptr buffer
>>>>>
>>>>> ---
>>>>>    xen/arch/arm/tee/optee.c | 27 ++++++++++++++++++++++++---
>>>>>    1 file changed, 24 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
>>>>> index 6963238056..70bfef7e5f 100644
>>>>> --- a/xen/arch/arm/tee/optee.c
>>>>> +++ b/xen/arch/arm/tee/optee.c
>>>>> @@ -215,6 +215,15 @@ static bool optee_probe(void)
>>>>>        return true;
>>>>>    }
>>>>>    +/*
>>>>> + * TODO: There is a potential issue with guests that either have RAM
>>>>> + * at IPA of 0x0 or some of theirs memory is mapped at PA 0x0. This is
>>>>                                  ^ their
>>>>
>>>>> + * because PA of 0x0 is considered as NULL pointer by OP-TEE. It will
>>>>> + * not be able to map buffer with such pointer to TA address space, or
>>>>> + * use such buffer for communication with the guest. We either need to
>>>>> + * check that guest have no such mappings or ensure that OP-TEE
>>>>> + * enabled guest will not be created with such mappings.
>>>>> + */
>>>>>    static int optee_domain_init(struct domain *d)
>>>>>    {
>>>>>        struct arm_smccc_res resp;
>>>>> @@ -725,6 +734,15 @@ static int translate_noncontig(struct optee_domain
>>>>> *ctx,
>>>>>            uint64_t next_page_data;
>>>>>        } *guest_data, *xen_data;
>>>>>    +    /*
>>>>> +     * Special case: buffer with buf_ptr == 0x0 is considered as NULL
>>>>> +     * pointer by OP-TEE. No translation is needed. This can lead to
>>>>> +     * an issue as IPA 0x0 is a valid address for Xen. See the comment
>>>>> +     * near optee_domain_init()
>>>>> +     */
>>>>> +    if ( !param->u.tmem.buf_ptr )
>>>>> +        return 0;
>>>>
>>>> Given that today it is not possible for this to happen, it could even be
>>>> an ASSERT. But I think I would just return an error, maybe -EINVAL?
>>>
>>> Hmm, looks like my comment is somewhat misleading :(
>>
>> How about the following comment:
>>
>> We don't want to translate NULL (0) as it can be used by the guest to fetch
>> the size of the buffer to allocate. This behavior depends on TA, but there is
>> a guarantee that OP-TEE will not try to map it (see more details on top of
>> optee_domain_init()).
>>
>>>
>>> What I mean, is that param->u.tmem.buf_ptr == 0 is the normal situation.
>>> This is the special case, when OP-TEE treats this buffer as a NULL. So
>>> we are doing nothing there. Thus, "return 0".
>>>
>>> But, as Julien pointed out, we can have machine where 0x0 is the valid
>>> memory address and there is a chance, that some guest will use it as a
>>> pointer to buffer.
>>>
>>>> Aside from this, and the small grammar issue, everything else looks fine
>>>> to me.
>>>>
>>>> Let's wait for Julien's reply, but if this is the only thing I could fix
>>>> on commit.
>>
>> I agree with Volodymyr, this is the normal case here. There are more work to
>> prevent MFN 0 to be mapped in the guest but this shouldn't be an issue today.
> 
> Let's put the MFN 0 issue aside for a moment.
> 
>  From the commit message I thought that if the guest wanted to pass a
> NULL buffer ("Null memory reference") then it would also *not* set
> OPTEE_MSG_ATTR_NONCONTIG, which would be handled by the "else" statement
> also modified by this patch. Thus, I thought that reaching
> translate_noncontig with buf_ptr == NULL would always be an error.
> 
> But re-reading the commit message and from both your answers it is not
> the case: a "Null memory reference" is allowed with
> OPTEE_MSG_ATTR_NONCONTIG set too.
> 
> Thus, I have no further comments and the improvements on the in-code
> comment could be done on commit.

Good :). IIRC Paul gave a provisional RaB for this series. @Paul, now 
that we are settled, could we get a formal one?

Cheers,

-- 
Julien Grall

RE: [PATCH v2 2/2] optee: allow plain TMEM buffers with NULL address

[Paul Durrant]

[-- Attachment #1: Type: text/plain, Size: 6368 bytes --]

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: 26 June 2020 18:54
> To: Stefano Stabellini <sstabellini@kernel.org>; paul(a)xen.org
> Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; xen-devel(a)lists.xenproject.org; op-
> tee(a)lists.trustedfirmware.org
> Subject: Re: [PATCH v2 2/2] optee: allow plain TMEM buffers with NULL address
> 
> (using paul xen.org's email)
> 

Thanks. Avoids annoying warning banners :-)

> Hi,
> 
> Apologies for the late answer.
> 
> On 23/06/2020 22:09, Stefano Stabellini wrote:
> > On Tue, 23 Jun 2020, Julien Grall wrote:
> >> On 23/06/2020 03:49, Volodymyr Babchuk wrote:
> >>>
> >>> Hi Stefano,
> >>>
> >>> Stefano Stabellini writes:
> >>>
> >>>> On Fri, 19 Jun 2020, Volodymyr Babchuk wrote:
> >>>>> Trusted Applications use popular approach to determine required size
> >>>>> of buffer: client provides a memory reference with the NULL pointer to
> >>>>> a buffer. This is so called "Null memory reference". TA updates the
> >>>>> reference with the required size and returns it back to the
> >>>>> client. Then client allocates buffer of needed size and repeats the
> >>>>> operation.
> >>>>>
> >>>>> This behavior is described in TEE Client API Specification, paragraph
> >>>>> 3.2.5. Memory References.
> >>>>>
> >>>>> OP-TEE represents this null memory reference as a TMEM parameter with
> >>>>> buf_ptr = 0x0. This is the only case when we should allow TMEM
> >>>>> buffer without the OPTEE_MSG_ATTR_NONCONTIG flag. This also the
> >>>>> special case for a buffer with OPTEE_MSG_ATTR_NONCONTIG flag.
> >>>>>
> >>>>> This could lead to a potential issue, because IPA 0x0 is a valid
> >>>>> address, but OP-TEE will treat it as a special case. So, care should
> >>>>> be taken when construction OP-TEE enabled guest to make sure that such
> >>>>> guest have no memory at IPA 0x0 and none of its memory is mapped at PA
> >>>>> 0x0.
> >>>>>
> >>>>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> >>>>> ---
> >>>>>
> >>>>> Changes from v1:
> >>>>>    - Added comment with TODO about possible PA/IPA 0x0 issue
> >>>>>    - The same is described in the commit message
> >>>>>    - Added check in translate_noncontig() for the NULL ptr buffer
> >>>>>
> >>>>> ---
> >>>>>    xen/arch/arm/tee/optee.c | 27 ++++++++++++++++++++++++---
> >>>>>    1 file changed, 24 insertions(+), 3 deletions(-)
> >>>>>
> >>>>> diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
> >>>>> index 6963238056..70bfef7e5f 100644
> >>>>> --- a/xen/arch/arm/tee/optee.c
> >>>>> +++ b/xen/arch/arm/tee/optee.c
> >>>>> @@ -215,6 +215,15 @@ static bool optee_probe(void)
> >>>>>        return true;
> >>>>>    }
> >>>>>    +/*
> >>>>> + * TODO: There is a potential issue with guests that either have RAM
> >>>>> + * at IPA of 0x0 or some of theirs memory is mapped at PA 0x0. This is
> >>>>                                  ^ their
> >>>>
> >>>>> + * because PA of 0x0 is considered as NULL pointer by OP-TEE. It will
> >>>>> + * not be able to map buffer with such pointer to TA address space, or
> >>>>> + * use such buffer for communication with the guest. We either need to
> >>>>> + * check that guest have no such mappings or ensure that OP-TEE
> >>>>> + * enabled guest will not be created with such mappings.
> >>>>> + */
> >>>>>    static int optee_domain_init(struct domain *d)
> >>>>>    {
> >>>>>        struct arm_smccc_res resp;
> >>>>> @@ -725,6 +734,15 @@ static int translate_noncontig(struct optee_domain
> >>>>> *ctx,
> >>>>>            uint64_t next_page_data;
> >>>>>        } *guest_data, *xen_data;
> >>>>>    +    /*
> >>>>> +     * Special case: buffer with buf_ptr == 0x0 is considered as NULL
> >>>>> +     * pointer by OP-TEE. No translation is needed. This can lead to
> >>>>> +     * an issue as IPA 0x0 is a valid address for Xen. See the comment
> >>>>> +     * near optee_domain_init()
> >>>>> +     */
> >>>>> +    if ( !param->u.tmem.buf_ptr )
> >>>>> +        return 0;
> >>>>
> >>>> Given that today it is not possible for this to happen, it could even be
> >>>> an ASSERT. But I think I would just return an error, maybe -EINVAL?
> >>>
> >>> Hmm, looks like my comment is somewhat misleading :(
> >>
> >> How about the following comment:
> >>
> >> We don't want to translate NULL (0) as it can be used by the guest to fetch
> >> the size of the buffer to allocate. This behavior depends on TA, but there is
> >> a guarantee that OP-TEE will not try to map it (see more details on top of
> >> optee_domain_init()).
> >>
> >>>
> >>> What I mean, is that param->u.tmem.buf_ptr == 0 is the normal situation.
> >>> This is the special case, when OP-TEE treats this buffer as a NULL. So
> >>> we are doing nothing there. Thus, "return 0".
> >>>
> >>> But, as Julien pointed out, we can have machine where 0x0 is the valid
> >>> memory address and there is a chance, that some guest will use it as a
> >>> pointer to buffer.
> >>>
> >>>> Aside from this, and the small grammar issue, everything else looks fine
> >>>> to me.
> >>>>
> >>>> Let's wait for Julien's reply, but if this is the only thing I could fix
> >>>> on commit.
> >>
> >> I agree with Volodymyr, this is the normal case here. There are more work to
> >> prevent MFN 0 to be mapped in the guest but this shouldn't be an issue today.
> >
> > Let's put the MFN 0 issue aside for a moment.
> >
> >  From the commit message I thought that if the guest wanted to pass a
> > NULL buffer ("Null memory reference") then it would also *not* set
> > OPTEE_MSG_ATTR_NONCONTIG, which would be handled by the "else" statement
> > also modified by this patch. Thus, I thought that reaching
> > translate_noncontig with buf_ptr == NULL would always be an error.
> >
> > But re-reading the commit message and from both your answers it is not
> > the case: a "Null memory reference" is allowed with
> > OPTEE_MSG_ATTR_NONCONTIG set too.
> >
> > Thus, I have no further comments and the improvements on the in-code
> > comment could be done on commit.
> 
> Good :). IIRC Paul gave a provisional RaB for this series. @Paul, now
> that we are settled, could we get a formal one?

Sure.

Release-acked-by: Paul Durrant <paul@xen.org>

> 
> Cheers,
> 
> --
> Julien Grall

RE: [PATCH v2 2/2] optee: allow plain TMEM buffers with NULL address

[Paul Durrant]

> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: 26 June 2020 18:54
> To: Stefano Stabellini <sstabellini@kernel.org>; paul@xen.org
> Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; xen-devel@lists.xenproject.org; op-
> tee@lists.trustedfirmware.org
> Subject: Re: [PATCH v2 2/2] optee: allow plain TMEM buffers with NULL address
> 
> (using paul xen.org's email)
> 

Thanks. Avoids annoying warning banners :-)

> Hi,
> 
> Apologies for the late answer.
> 
> On 23/06/2020 22:09, Stefano Stabellini wrote:
> > On Tue, 23 Jun 2020, Julien Grall wrote:
> >> On 23/06/2020 03:49, Volodymyr Babchuk wrote:
> >>>
> >>> Hi Stefano,
> >>>
> >>> Stefano Stabellini writes:
> >>>
> >>>> On Fri, 19 Jun 2020, Volodymyr Babchuk wrote:
> >>>>> Trusted Applications use popular approach to determine required size
> >>>>> of buffer: client provides a memory reference with the NULL pointer to
> >>>>> a buffer. This is so called "Null memory reference". TA updates the
> >>>>> reference with the required size and returns it back to the
> >>>>> client. Then client allocates buffer of needed size and repeats the
> >>>>> operation.
> >>>>>
> >>>>> This behavior is described in TEE Client API Specification, paragraph
> >>>>> 3.2.5. Memory References.
> >>>>>
> >>>>> OP-TEE represents this null memory reference as a TMEM parameter with
> >>>>> buf_ptr = 0x0. This is the only case when we should allow TMEM
> >>>>> buffer without the OPTEE_MSG_ATTR_NONCONTIG flag. This also the
> >>>>> special case for a buffer with OPTEE_MSG_ATTR_NONCONTIG flag.
> >>>>>
> >>>>> This could lead to a potential issue, because IPA 0x0 is a valid
> >>>>> address, but OP-TEE will treat it as a special case. So, care should
> >>>>> be taken when construction OP-TEE enabled guest to make sure that such
> >>>>> guest have no memory at IPA 0x0 and none of its memory is mapped at PA
> >>>>> 0x0.
> >>>>>
> >>>>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
> >>>>> ---
> >>>>>
> >>>>> Changes from v1:
> >>>>>    - Added comment with TODO about possible PA/IPA 0x0 issue
> >>>>>    - The same is described in the commit message
> >>>>>    - Added check in translate_noncontig() for the NULL ptr buffer
> >>>>>
> >>>>> ---
> >>>>>    xen/arch/arm/tee/optee.c | 27 ++++++++++++++++++++++++---
> >>>>>    1 file changed, 24 insertions(+), 3 deletions(-)
> >>>>>
> >>>>> diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
> >>>>> index 6963238056..70bfef7e5f 100644
> >>>>> --- a/xen/arch/arm/tee/optee.c
> >>>>> +++ b/xen/arch/arm/tee/optee.c
> >>>>> @@ -215,6 +215,15 @@ static bool optee_probe(void)
> >>>>>        return true;
> >>>>>    }
> >>>>>    +/*
> >>>>> + * TODO: There is a potential issue with guests that either have RAM
> >>>>> + * at IPA of 0x0 or some of theirs memory is mapped at PA 0x0. This is
> >>>>                                  ^ their
> >>>>
> >>>>> + * because PA of 0x0 is considered as NULL pointer by OP-TEE. It will
> >>>>> + * not be able to map buffer with such pointer to TA address space, or
> >>>>> + * use such buffer for communication with the guest. We either need to
> >>>>> + * check that guest have no such mappings or ensure that OP-TEE
> >>>>> + * enabled guest will not be created with such mappings.
> >>>>> + */
> >>>>>    static int optee_domain_init(struct domain *d)
> >>>>>    {
> >>>>>        struct arm_smccc_res resp;
> >>>>> @@ -725,6 +734,15 @@ static int translate_noncontig(struct optee_domain
> >>>>> *ctx,
> >>>>>            uint64_t next_page_data;
> >>>>>        } *guest_data, *xen_data;
> >>>>>    +    /*
> >>>>> +     * Special case: buffer with buf_ptr == 0x0 is considered as NULL
> >>>>> +     * pointer by OP-TEE. No translation is needed. This can lead to
> >>>>> +     * an issue as IPA 0x0 is a valid address for Xen. See the comment
> >>>>> +     * near optee_domain_init()
> >>>>> +     */
> >>>>> +    if ( !param->u.tmem.buf_ptr )
> >>>>> +        return 0;
> >>>>
> >>>> Given that today it is not possible for this to happen, it could even be
> >>>> an ASSERT. But I think I would just return an error, maybe -EINVAL?
> >>>
> >>> Hmm, looks like my comment is somewhat misleading :(
> >>
> >> How about the following comment:
> >>
> >> We don't want to translate NULL (0) as it can be used by the guest to fetch
> >> the size of the buffer to allocate. This behavior depends on TA, but there is
> >> a guarantee that OP-TEE will not try to map it (see more details on top of
> >> optee_domain_init()).
> >>
> >>>
> >>> What I mean, is that param->u.tmem.buf_ptr == 0 is the normal situation.
> >>> This is the special case, when OP-TEE treats this buffer as a NULL. So
> >>> we are doing nothing there. Thus, "return 0".
> >>>
> >>> But, as Julien pointed out, we can have machine where 0x0 is the valid
> >>> memory address and there is a chance, that some guest will use it as a
> >>> pointer to buffer.
> >>>
> >>>> Aside from this, and the small grammar issue, everything else looks fine
> >>>> to me.
> >>>>
> >>>> Let's wait for Julien's reply, but if this is the only thing I could fix
> >>>> on commit.
> >>
> >> I agree with Volodymyr, this is the normal case here. There are more work to
> >> prevent MFN 0 to be mapped in the guest but this shouldn't be an issue today.
> >
> > Let's put the MFN 0 issue aside for a moment.
> >
> >  From the commit message I thought that if the guest wanted to pass a
> > NULL buffer ("Null memory reference") then it would also *not* set
> > OPTEE_MSG_ATTR_NONCONTIG, which would be handled by the "else" statement
> > also modified by this patch. Thus, I thought that reaching
> > translate_noncontig with buf_ptr == NULL would always be an error.
> >
> > But re-reading the commit message and from both your answers it is not
> > the case: a "Null memory reference" is allowed with
> > OPTEE_MSG_ATTR_NONCONTIG set too.
> >
> > Thus, I have no further comments and the improvements on the in-code
> > comment could be done on commit.
> 
> Good :). IIRC Paul gave a provisional RaB for this series. @Paul, now
> that we are settled, could we get a formal one?

Sure.

Release-acked-by: Paul Durrant <paul@xen.org>

> 
> Cheers,
> 
> --
> Julien Grall

Re: [PATCH v2 2/2] optee: allow plain TMEM buffers with NULL address

[Julien Grall]

[-- Attachment #1: Type: text/plain, Size: 6577 bytes --]



On 29/06/2020 08:42, Paul Durrant wrote:
>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Sent: 26 June 2020 18:54
>> To: Stefano Stabellini <sstabellini@kernel.org>; paul(a)xen.org
>> Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; xen-devel(a)lists.xenproject.org; op-
>> tee(a)lists.trustedfirmware.org
>> Subject: Re: [PATCH v2 2/2] optee: allow plain TMEM buffers with NULL address
>>
>> (using paul xen.org's email)
>>
> 
> Thanks. Avoids annoying warning banners :-)
> 
>> Hi,
>>
>> Apologies for the late answer.
>>
>> On 23/06/2020 22:09, Stefano Stabellini wrote:
>>> On Tue, 23 Jun 2020, Julien Grall wrote:
>>>> On 23/06/2020 03:49, Volodymyr Babchuk wrote:
>>>>>
>>>>> Hi Stefano,
>>>>>
>>>>> Stefano Stabellini writes:
>>>>>
>>>>>> On Fri, 19 Jun 2020, Volodymyr Babchuk wrote:
>>>>>>> Trusted Applications use popular approach to determine required size
>>>>>>> of buffer: client provides a memory reference with the NULL pointer to
>>>>>>> a buffer. This is so called "Null memory reference". TA updates the
>>>>>>> reference with the required size and returns it back to the
>>>>>>> client. Then client allocates buffer of needed size and repeats the
>>>>>>> operation.
>>>>>>>
>>>>>>> This behavior is described in TEE Client API Specification, paragraph
>>>>>>> 3.2.5. Memory References.
>>>>>>>
>>>>>>> OP-TEE represents this null memory reference as a TMEM parameter with
>>>>>>> buf_ptr = 0x0. This is the only case when we should allow TMEM
>>>>>>> buffer without the OPTEE_MSG_ATTR_NONCONTIG flag. This also the
>>>>>>> special case for a buffer with OPTEE_MSG_ATTR_NONCONTIG flag.
>>>>>>>
>>>>>>> This could lead to a potential issue, because IPA 0x0 is a valid
>>>>>>> address, but OP-TEE will treat it as a special case. So, care should
>>>>>>> be taken when construction OP-TEE enabled guest to make sure that such
>>>>>>> guest have no memory at IPA 0x0 and none of its memory is mapped at PA
>>>>>>> 0x0.
>>>>>>>
>>>>>>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>>>>>>> ---
>>>>>>>
>>>>>>> Changes from v1:
>>>>>>>     - Added comment with TODO about possible PA/IPA 0x0 issue
>>>>>>>     - The same is described in the commit message
>>>>>>>     - Added check in translate_noncontig() for the NULL ptr buffer
>>>>>>>
>>>>>>> ---
>>>>>>>     xen/arch/arm/tee/optee.c | 27 ++++++++++++++++++++++++---
>>>>>>>     1 file changed, 24 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
>>>>>>> index 6963238056..70bfef7e5f 100644
>>>>>>> --- a/xen/arch/arm/tee/optee.c
>>>>>>> +++ b/xen/arch/arm/tee/optee.c
>>>>>>> @@ -215,6 +215,15 @@ static bool optee_probe(void)
>>>>>>>         return true;
>>>>>>>     }
>>>>>>>     +/*
>>>>>>> + * TODO: There is a potential issue with guests that either have RAM
>>>>>>> + * at IPA of 0x0 or some of theirs memory is mapped at PA 0x0. This is
>>>>>>                                   ^ their
>>>>>>
>>>>>>> + * because PA of 0x0 is considered as NULL pointer by OP-TEE. It will
>>>>>>> + * not be able to map buffer with such pointer to TA address space, or
>>>>>>> + * use such buffer for communication with the guest. We either need to
>>>>>>> + * check that guest have no such mappings or ensure that OP-TEE
>>>>>>> + * enabled guest will not be created with such mappings.
>>>>>>> + */
>>>>>>>     static int optee_domain_init(struct domain *d)
>>>>>>>     {
>>>>>>>         struct arm_smccc_res resp;
>>>>>>> @@ -725,6 +734,15 @@ static int translate_noncontig(struct optee_domain
>>>>>>> *ctx,
>>>>>>>             uint64_t next_page_data;
>>>>>>>         } *guest_data, *xen_data;
>>>>>>>     +    /*
>>>>>>> +     * Special case: buffer with buf_ptr == 0x0 is considered as NULL
>>>>>>> +     * pointer by OP-TEE. No translation is needed. This can lead to
>>>>>>> +     * an issue as IPA 0x0 is a valid address for Xen. See the comment
>>>>>>> +     * near optee_domain_init()
>>>>>>> +     */
>>>>>>> +    if ( !param->u.tmem.buf_ptr )
>>>>>>> +        return 0;
>>>>>>
>>>>>> Given that today it is not possible for this to happen, it could even be
>>>>>> an ASSERT. But I think I would just return an error, maybe -EINVAL?
>>>>>
>>>>> Hmm, looks like my comment is somewhat misleading :(
>>>>
>>>> How about the following comment:
>>>>
>>>> We don't want to translate NULL (0) as it can be used by the guest to fetch
>>>> the size of the buffer to allocate. This behavior depends on TA, but there is
>>>> a guarantee that OP-TEE will not try to map it (see more details on top of
>>>> optee_domain_init()).
>>>>
>>>>>
>>>>> What I mean, is that param->u.tmem.buf_ptr == 0 is the normal situation.
>>>>> This is the special case, when OP-TEE treats this buffer as a NULL. So
>>>>> we are doing nothing there. Thus, "return 0".
>>>>>
>>>>> But, as Julien pointed out, we can have machine where 0x0 is the valid
>>>>> memory address and there is a chance, that some guest will use it as a
>>>>> pointer to buffer.
>>>>>
>>>>>> Aside from this, and the small grammar issue, everything else looks fine
>>>>>> to me.
>>>>>>
>>>>>> Let's wait for Julien's reply, but if this is the only thing I could fix
>>>>>> on commit.
>>>>
>>>> I agree with Volodymyr, this is the normal case here. There are more work to
>>>> prevent MFN 0 to be mapped in the guest but this shouldn't be an issue today.
>>>
>>> Let's put the MFN 0 issue aside for a moment.
>>>
>>>   From the commit message I thought that if the guest wanted to pass a
>>> NULL buffer ("Null memory reference") then it would also *not* set
>>> OPTEE_MSG_ATTR_NONCONTIG, which would be handled by the "else" statement
>>> also modified by this patch. Thus, I thought that reaching
>>> translate_noncontig with buf_ptr == NULL would always be an error.
>>>
>>> But re-reading the commit message and from both your answers it is not
>>> the case: a "Null memory reference" is allowed with
>>> OPTEE_MSG_ATTR_NONCONTIG set too.
>>>
>>> Thus, I have no further comments and the improvements on the in-code
>>> comment could be done on commit.
>>
>> Good :). IIRC Paul gave a provisional RaB for this series. @Paul, now
>> that we are settled, could we get a formal one?
> 
> Sure.
> 
> Release-acked-by: Paul Durrant <paul@xen.org>

Thanks!

It is not clear to me what Stefano had in mind for the "in-code 
comment". So I will leave him committing the series.

Cheers,

-- 
Julien Grall

Re: [PATCH v2 2/2] optee: allow plain TMEM buffers with NULL address

[Julien Grall]

On 29/06/2020 08:42, Paul Durrant wrote:
>> -----Original Message-----
>> From: Julien Grall <julien@xen.org>
>> Sent: 26 June 2020 18:54
>> To: Stefano Stabellini <sstabellini@kernel.org>; paul@xen.org
>> Cc: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>; xen-devel@lists.xenproject.org; op-
>> tee@lists.trustedfirmware.org
>> Subject: Re: [PATCH v2 2/2] optee: allow plain TMEM buffers with NULL address
>>
>> (using paul xen.org's email)
>>
> 
> Thanks. Avoids annoying warning banners :-)
> 
>> Hi,
>>
>> Apologies for the late answer.
>>
>> On 23/06/2020 22:09, Stefano Stabellini wrote:
>>> On Tue, 23 Jun 2020, Julien Grall wrote:
>>>> On 23/06/2020 03:49, Volodymyr Babchuk wrote:
>>>>>
>>>>> Hi Stefano,
>>>>>
>>>>> Stefano Stabellini writes:
>>>>>
>>>>>> On Fri, 19 Jun 2020, Volodymyr Babchuk wrote:
>>>>>>> Trusted Applications use popular approach to determine required size
>>>>>>> of buffer: client provides a memory reference with the NULL pointer to
>>>>>>> a buffer. This is so called "Null memory reference". TA updates the
>>>>>>> reference with the required size and returns it back to the
>>>>>>> client. Then client allocates buffer of needed size and repeats the
>>>>>>> operation.
>>>>>>>
>>>>>>> This behavior is described in TEE Client API Specification, paragraph
>>>>>>> 3.2.5. Memory References.
>>>>>>>
>>>>>>> OP-TEE represents this null memory reference as a TMEM parameter with
>>>>>>> buf_ptr = 0x0. This is the only case when we should allow TMEM
>>>>>>> buffer without the OPTEE_MSG_ATTR_NONCONTIG flag. This also the
>>>>>>> special case for a buffer with OPTEE_MSG_ATTR_NONCONTIG flag.
>>>>>>>
>>>>>>> This could lead to a potential issue, because IPA 0x0 is a valid
>>>>>>> address, but OP-TEE will treat it as a special case. So, care should
>>>>>>> be taken when construction OP-TEE enabled guest to make sure that such
>>>>>>> guest have no memory at IPA 0x0 and none of its memory is mapped at PA
>>>>>>> 0x0.
>>>>>>>
>>>>>>> Signed-off-by: Volodymyr Babchuk <volodymyr_babchuk@epam.com>
>>>>>>> ---
>>>>>>>
>>>>>>> Changes from v1:
>>>>>>>     - Added comment with TODO about possible PA/IPA 0x0 issue
>>>>>>>     - The same is described in the commit message
>>>>>>>     - Added check in translate_noncontig() for the NULL ptr buffer
>>>>>>>
>>>>>>> ---
>>>>>>>     xen/arch/arm/tee/optee.c | 27 ++++++++++++++++++++++++---
>>>>>>>     1 file changed, 24 insertions(+), 3 deletions(-)
>>>>>>>
>>>>>>> diff --git a/xen/arch/arm/tee/optee.c b/xen/arch/arm/tee/optee.c
>>>>>>> index 6963238056..70bfef7e5f 100644
>>>>>>> --- a/xen/arch/arm/tee/optee.c
>>>>>>> +++ b/xen/arch/arm/tee/optee.c
>>>>>>> @@ -215,6 +215,15 @@ static bool optee_probe(void)
>>>>>>>         return true;
>>>>>>>     }
>>>>>>>     +/*
>>>>>>> + * TODO: There is a potential issue with guests that either have RAM
>>>>>>> + * at IPA of 0x0 or some of theirs memory is mapped at PA 0x0. This is
>>>>>>                                   ^ their
>>>>>>
>>>>>>> + * because PA of 0x0 is considered as NULL pointer by OP-TEE. It will
>>>>>>> + * not be able to map buffer with such pointer to TA address space, or
>>>>>>> + * use such buffer for communication with the guest. We either need to
>>>>>>> + * check that guest have no such mappings or ensure that OP-TEE
>>>>>>> + * enabled guest will not be created with such mappings.
>>>>>>> + */
>>>>>>>     static int optee_domain_init(struct domain *d)
>>>>>>>     {
>>>>>>>         struct arm_smccc_res resp;
>>>>>>> @@ -725,6 +734,15 @@ static int translate_noncontig(struct optee_domain
>>>>>>> *ctx,
>>>>>>>             uint64_t next_page_data;
>>>>>>>         } *guest_data, *xen_data;
>>>>>>>     +    /*
>>>>>>> +     * Special case: buffer with buf_ptr == 0x0 is considered as NULL
>>>>>>> +     * pointer by OP-TEE. No translation is needed. This can lead to
>>>>>>> +     * an issue as IPA 0x0 is a valid address for Xen. See the comment
>>>>>>> +     * near optee_domain_init()
>>>>>>> +     */
>>>>>>> +    if ( !param->u.tmem.buf_ptr )
>>>>>>> +        return 0;
>>>>>>
>>>>>> Given that today it is not possible for this to happen, it could even be
>>>>>> an ASSERT. But I think I would just return an error, maybe -EINVAL?
>>>>>
>>>>> Hmm, looks like my comment is somewhat misleading :(
>>>>
>>>> How about the following comment:
>>>>
>>>> We don't want to translate NULL (0) as it can be used by the guest to fetch
>>>> the size of the buffer to allocate. This behavior depends on TA, but there is
>>>> a guarantee that OP-TEE will not try to map it (see more details on top of
>>>> optee_domain_init()).
>>>>
>>>>>
>>>>> What I mean, is that param->u.tmem.buf_ptr == 0 is the normal situation.
>>>>> This is the special case, when OP-TEE treats this buffer as a NULL. So
>>>>> we are doing nothing there. Thus, "return 0".
>>>>>
>>>>> But, as Julien pointed out, we can have machine where 0x0 is the valid
>>>>> memory address and there is a chance, that some guest will use it as a
>>>>> pointer to buffer.
>>>>>
>>>>>> Aside from this, and the small grammar issue, everything else looks fine
>>>>>> to me.
>>>>>>
>>>>>> Let's wait for Julien's reply, but if this is the only thing I could fix
>>>>>> on commit.
>>>>
>>>> I agree with Volodymyr, this is the normal case here. There are more work to
>>>> prevent MFN 0 to be mapped in the guest but this shouldn't be an issue today.
>>>
>>> Let's put the MFN 0 issue aside for a moment.
>>>
>>>   From the commit message I thought that if the guest wanted to pass a
>>> NULL buffer ("Null memory reference") then it would also *not* set
>>> OPTEE_MSG_ATTR_NONCONTIG, which would be handled by the "else" statement
>>> also modified by this patch. Thus, I thought that reaching
>>> translate_noncontig with buf_ptr == NULL would always be an error.
>>>
>>> But re-reading the commit message and from both your answers it is not
>>> the case: a "Null memory reference" is allowed with
>>> OPTEE_MSG_ATTR_NONCONTIG set too.
>>>
>>> Thus, I have no further comments and the improvements on the in-code
>>> comment could be done on commit.
>>
>> Good :). IIRC Paul gave a provisional RaB for this series. @Paul, now
>> that we are settled, could we get a formal one?
> 
> Sure.
> 
> Release-acked-by: Paul Durrant <paul@xen.org>

Thanks!

It is not clear to me what Stefano had in mind for the "in-code 
comment". So I will leave him committing the series.

Cheers,

-- 
Julien Grall