[PATCH 1/2] tee: remove tee_shm_va2pa() and tee_shm_pa2va()

[PATCH 1/2] tee: remove tee_shm_va2pa() and tee_shm_pa2va()

[Andrew Davis]

We should not need to index into SHMs based on absolute VA/PA.
These functions are not used and this kind of usage should not be
encouraged anyway. Remove these functions.

Signed-off-by: Andrew Davis <afd@ti.com>
---
 drivers/tee/tee_shm.c   | 50 -----------------------------------------
 include/linux/tee_drv.h | 18 ---------------
 2 files changed, 68 deletions(-)

diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
index f31e29e8f1cac..b0c6d553d3a70 100644
--- a/drivers/tee/tee_shm.c
+++ b/drivers/tee/tee_shm.c
@@ -414,56 +414,6 @@ void tee_shm_free(struct tee_shm *shm)
 }
 EXPORT_SYMBOL_GPL(tee_shm_free);
 
-/**
- * tee_shm_va2pa() - Get physical address of a virtual address
- * @shm:	Shared memory handle
- * @va:		Virtual address to tranlsate
- * @pa:		Returned physical address
- * @returns 0 on success and < 0 on failure
- */
-int tee_shm_va2pa(struct tee_shm *shm, void *va, phys_addr_t *pa)
-{
-	if (!shm->kaddr)
-		return -EINVAL;
-	/* Check that we're in the range of the shm */
-	if ((char *)va < (char *)shm->kaddr)
-		return -EINVAL;
-	if ((char *)va >= ((char *)shm->kaddr + shm->size))
-		return -EINVAL;
-
-	return tee_shm_get_pa(
-			shm, (unsigned long)va - (unsigned long)shm->kaddr, pa);
-}
-EXPORT_SYMBOL_GPL(tee_shm_va2pa);
-
-/**
- * tee_shm_pa2va() - Get virtual address of a physical address
- * @shm:	Shared memory handle
- * @pa:		Physical address to tranlsate
- * @va:		Returned virtual address
- * @returns 0 on success and < 0 on failure
- */
-int tee_shm_pa2va(struct tee_shm *shm, phys_addr_t pa, void **va)
-{
-	if (!shm->kaddr)
-		return -EINVAL;
-	/* Check that we're in the range of the shm */
-	if (pa < shm->paddr)
-		return -EINVAL;
-	if (pa >= (shm->paddr + shm->size))
-		return -EINVAL;
-
-	if (va) {
-		void *v = tee_shm_get_va(shm, pa - shm->paddr);
-
-		if (IS_ERR(v))
-			return PTR_ERR(v);
-		*va = v;
-	}
-	return 0;
-}
-EXPORT_SYMBOL_GPL(tee_shm_pa2va);
-
 /**
  * tee_shm_get_va() - Get virtual address of a shared memory plus an offset
  * @shm:	Shared memory handle
diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
index 911cad324acc7..17eb1c5205d34 100644
--- a/include/linux/tee_drv.h
+++ b/include/linux/tee_drv.h
@@ -298,24 +298,6 @@ void tee_shm_free(struct tee_shm *shm);
  */
 void tee_shm_put(struct tee_shm *shm);
 
-/**
- * tee_shm_va2pa() - Get physical address of a virtual address
- * @shm:	Shared memory handle
- * @va:		Virtual address to tranlsate
- * @pa:		Returned physical address
- * @returns 0 on success and < 0 on failure
- */
-int tee_shm_va2pa(struct tee_shm *shm, void *va, phys_addr_t *pa);
-
-/**
- * tee_shm_pa2va() - Get virtual address of a physical address
- * @shm:	Shared memory handle
- * @pa:		Physical address to tranlsate
- * @va:		Returned virtual address
- * @returns 0 on success and < 0 on failure
- */
-int tee_shm_pa2va(struct tee_shm *shm, phys_addr_t pa, void **va);
-
 /**
  * tee_shm_get_va() - Get virtual address of a shared memory plus an offset
  * @shm:	Shared memory handle
-- 
2.17.1

[PATCH 2/2] tee: remove flags TEE_IOCTL_SHM_MAPPED and TEE_IOCTL_SHM_DMA_BUF

[Andrew Davis]

These look to be leftover from an early edition of this driver. Userspace
does not need this information. Checking all users of this that I have
access to I have verified no one is using them.

They leak internal use flags out to userspace. Even more they are not
correct anymore after a45ea4efa358. Lets drop these flags before
someone does try to use them for something and they become ABI.

Signed-off-by: Andrew Davis <afd@ti.com>
---
 drivers/tee/tee_core.c   | 1 -
 include/uapi/linux/tee.h | 4 ----
 2 files changed, 5 deletions(-)

diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
index 8aa1a4836b92f..650dd87a38e77 100644
--- a/drivers/tee/tee_core.c
+++ b/drivers/tee/tee_core.c
@@ -339,7 +339,6 @@ tee_ioctl_shm_register(struct tee_context *ctx,
 		return PTR_ERR(shm);
 
 	data.id = shm->id;
-	data.flags = shm->flags;
 	data.length = shm->size;
 
 	if (copy_to_user(udata, &data, sizeof(data)))
diff --git a/include/uapi/linux/tee.h b/include/uapi/linux/tee.h
index 25a6c534beb1b..23e57164693c4 100644
--- a/include/uapi/linux/tee.h
+++ b/include/uapi/linux/tee.h
@@ -42,10 +42,6 @@
 #define TEE_IOC_MAGIC	0xa4
 #define TEE_IOC_BASE	0
 
-/* Flags relating to shared memory */
-#define TEE_IOCTL_SHM_MAPPED	0x1	/* memory mapped in normal world */
-#define TEE_IOCTL_SHM_DMA_BUF	0x2	/* dma-buf handle on shared memory */
-
 #define TEE_MAX_ARG_SIZE	1024
 
 #define TEE_GEN_CAP_GP		(1 << 0)/* GlobalPlatform compliant TEE */
-- 
2.17.1

Re: [PATCH 1/2] tee: remove tee_shm_va2pa() and tee_shm_pa2va()

[Sumit Garg]

On Fri, 22 Apr 2022 at 23:31, Andrew Davis <afd@ti.com> wrote:
>
> We should not need to index into SHMs based on absolute VA/PA.
> These functions are not used and this kind of usage should not be
> encouraged anyway. Remove these functions.
>
> Signed-off-by: Andrew Davis <afd@ti.com>
> ---
>  drivers/tee/tee_shm.c   | 50 -----------------------------------------
>  include/linux/tee_drv.h | 18 ---------------
>  2 files changed, 68 deletions(-)
>

Sounds good to me as there are tee_shm_get_{va/pa}() which are well
suited/used for index based VA/PA. FWIW:

Reviewed-by: Sumit Garg <sumit.garg@linaro.org>

-Sumit

> diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c
> index f31e29e8f1cac..b0c6d553d3a70 100644
> --- a/drivers/tee/tee_shm.c
> +++ b/drivers/tee/tee_shm.c
> @@ -414,56 +414,6 @@ void tee_shm_free(struct tee_shm *shm)
>  }
>  EXPORT_SYMBOL_GPL(tee_shm_free);
>
> -/**
> - * tee_shm_va2pa() - Get physical address of a virtual address
> - * @shm:       Shared memory handle
> - * @va:                Virtual address to tranlsate
> - * @pa:                Returned physical address
> - * @returns 0 on success and < 0 on failure
> - */
> -int tee_shm_va2pa(struct tee_shm *shm, void *va, phys_addr_t *pa)
> -{
> -       if (!shm->kaddr)
> -               return -EINVAL;
> -       /* Check that we're in the range of the shm */
> -       if ((char *)va < (char *)shm->kaddr)
> -               return -EINVAL;
> -       if ((char *)va >= ((char *)shm->kaddr + shm->size))
> -               return -EINVAL;
> -
> -       return tee_shm_get_pa(
> -                       shm, (unsigned long)va - (unsigned long)shm->kaddr, pa);
> -}
> -EXPORT_SYMBOL_GPL(tee_shm_va2pa);
> -
> -/**
> - * tee_shm_pa2va() - Get virtual address of a physical address
> - * @shm:       Shared memory handle
> - * @pa:                Physical address to tranlsate
> - * @va:                Returned virtual address
> - * @returns 0 on success and < 0 on failure
> - */
> -int tee_shm_pa2va(struct tee_shm *shm, phys_addr_t pa, void **va)
> -{
> -       if (!shm->kaddr)
> -               return -EINVAL;
> -       /* Check that we're in the range of the shm */
> -       if (pa < shm->paddr)
> -               return -EINVAL;
> -       if (pa >= (shm->paddr + shm->size))
> -               return -EINVAL;
> -
> -       if (va) {
> -               void *v = tee_shm_get_va(shm, pa - shm->paddr);
> -
> -               if (IS_ERR(v))
> -                       return PTR_ERR(v);
> -               *va = v;
> -       }
> -       return 0;
> -}
> -EXPORT_SYMBOL_GPL(tee_shm_pa2va);
> -
>  /**
>   * tee_shm_get_va() - Get virtual address of a shared memory plus an offset
>   * @shm:       Shared memory handle
> diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
> index 911cad324acc7..17eb1c5205d34 100644
> --- a/include/linux/tee_drv.h
> +++ b/include/linux/tee_drv.h
> @@ -298,24 +298,6 @@ void tee_shm_free(struct tee_shm *shm);
>   */
>  void tee_shm_put(struct tee_shm *shm);
>
> -/**
> - * tee_shm_va2pa() - Get physical address of a virtual address
> - * @shm:       Shared memory handle
> - * @va:                Virtual address to tranlsate
> - * @pa:                Returned physical address
> - * @returns 0 on success and < 0 on failure
> - */
> -int tee_shm_va2pa(struct tee_shm *shm, void *va, phys_addr_t *pa);
> -
> -/**
> - * tee_shm_pa2va() - Get virtual address of a physical address
> - * @shm:       Shared memory handle
> - * @pa:                Physical address to tranlsate
> - * @va:                Returned virtual address
> - * @returns 0 on success and < 0 on failure
> - */
> -int tee_shm_pa2va(struct tee_shm *shm, phys_addr_t pa, void **va);
> -
>  /**
>   * tee_shm_get_va() - Get virtual address of a shared memory plus an offset
>   * @shm:       Shared memory handle
> --
> 2.17.1
>

Re: [PATCH 2/2] tee: remove flags TEE_IOCTL_SHM_MAPPED and TEE_IOCTL_SHM_DMA_BUF

[Sumit Garg]

On Fri, 22 Apr 2022 at 23:31, Andrew Davis <afd@ti.com> wrote:
>
> These look to be leftover from an early edition of this driver. Userspace
> does not need this information. Checking all users of this that I have
> access to I have verified no one is using them.
>
> They leak internal use flags out to userspace. Even more they are not
> correct anymore after a45ea4efa358. Lets drop these flags before
> someone does try to use them for something and they become ABI.
>

Sounds reasonable to me.

> Signed-off-by: Andrew Davis <afd@ti.com>
> ---
>  drivers/tee/tee_core.c   | 1 -
>  include/uapi/linux/tee.h | 4 ----
>  2 files changed, 5 deletions(-)
>
> diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
> index 8aa1a4836b92f..650dd87a38e77 100644
> --- a/drivers/tee/tee_core.c
> +++ b/drivers/tee/tee_core.c
> @@ -339,7 +339,6 @@ tee_ioctl_shm_register(struct tee_context *ctx,
>                 return PTR_ERR(shm);
>
>         data.id = shm->id;
> -       data.flags = shm->flags;
>         data.length = shm->size;
>

This change is required for tee_ioctl_shm_alloc() as well.

-Sumit

>         if (copy_to_user(udata, &data, sizeof(data)))
> diff --git a/include/uapi/linux/tee.h b/include/uapi/linux/tee.h
> index 25a6c534beb1b..23e57164693c4 100644
> --- a/include/uapi/linux/tee.h
> +++ b/include/uapi/linux/tee.h
> @@ -42,10 +42,6 @@
>  #define TEE_IOC_MAGIC  0xa4
>  #define TEE_IOC_BASE   0
>
> -/* Flags relating to shared memory */
> -#define TEE_IOCTL_SHM_MAPPED   0x1     /* memory mapped in normal world */
> -#define TEE_IOCTL_SHM_DMA_BUF  0x2     /* dma-buf handle on shared memory */
> -
>  #define TEE_MAX_ARG_SIZE       1024
>
>  #define TEE_GEN_CAP_GP         (1 << 0)/* GlobalPlatform compliant TEE */
> --
> 2.17.1
>

Re: [PATCH 2/2] tee: remove flags TEE_IOCTL_SHM_MAPPED and TEE_IOCTL_SHM_DMA_BUF

[Andrew Davis]

On 4/25/22 1:46 AM, Sumit Garg wrote:
> On Fri, 22 Apr 2022 at 23:31, Andrew Davis <afd@ti.com> wrote:
>>
>> These look to be leftover from an early edition of this driver. Userspace
>> does not need this information. Checking all users of this that I have
>> access to I have verified no one is using them.
>>
>> They leak internal use flags out to userspace. Even more they are not
>> correct anymore after a45ea4efa358. Lets drop these flags before
>> someone does try to use them for something and they become ABI.
>>
> 
> Sounds reasonable to me.
> 
>> Signed-off-by: Andrew Davis <afd@ti.com>
>> ---
>>   drivers/tee/tee_core.c   | 1 -
>>   include/uapi/linux/tee.h | 4 ----
>>   2 files changed, 5 deletions(-)
>>
>> diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
>> index 8aa1a4836b92f..650dd87a38e77 100644
>> --- a/drivers/tee/tee_core.c
>> +++ b/drivers/tee/tee_core.c
>> @@ -339,7 +339,6 @@ tee_ioctl_shm_register(struct tee_context *ctx,
>>                  return PTR_ERR(shm);
>>
>>          data.id = shm->id;
>> -       data.flags = shm->flags;
>>          data.length = shm->size;
>>
> 
> This change is required for tee_ioctl_shm_alloc() as well.
> 


Indeed it is, adding for v2.

Thanks,
Andrew


> -Sumit
> 
>>          if (copy_to_user(udata, &data, sizeof(data)))
>> diff --git a/include/uapi/linux/tee.h b/include/uapi/linux/tee.h
>> index 25a6c534beb1b..23e57164693c4 100644
>> --- a/include/uapi/linux/tee.h
>> +++ b/include/uapi/linux/tee.h
>> @@ -42,10 +42,6 @@
>>   #define TEE_IOC_MAGIC  0xa4
>>   #define TEE_IOC_BASE   0
>>
>> -/* Flags relating to shared memory */
>> -#define TEE_IOCTL_SHM_MAPPED   0x1     /* memory mapped in normal world */
>> -#define TEE_IOCTL_SHM_DMA_BUF  0x2     /* dma-buf handle on shared memory */
>> -
>>   #define TEE_MAX_ARG_SIZE       1024
>>
>>   #define TEE_GEN_CAP_GP         (1 << 0)/* GlobalPlatform compliant TEE */
>> --
>> 2.17.1
>>