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

[PATCH v2 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>
Reviewed-by: Sumit Garg <sumit.garg@linaro.org>
---
 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 v2 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>
---

Changes from v1:
 - Removed flags return from tee_ioctl_shm_alloc()

 drivers/tee/tee_core.c   | 2 --
 include/uapi/linux/tee.h | 4 ----
 2 files changed, 6 deletions(-)

diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
index 8aa1a4836b92f..af0f7c603fa46 100644
--- a/drivers/tee/tee_core.c
+++ b/drivers/tee/tee_core.c
@@ -302,7 +302,6 @@ static int tee_ioctl_shm_alloc(struct tee_context *ctx,
 		return PTR_ERR(shm);
 
 	data.id = shm->id;
-	data.flags = shm->flags;
 	data.size = shm->size;
 
 	if (copy_to_user(udata, &data, sizeof(data)))
@@ -339,7 +338,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 v2 2/2] tee: remove flags TEE_IOCTL_SHM_MAPPED and TEE_IOCTL_SHM_DMA_BUF

[Sumit Garg]

On Mon, 25 Apr 2022 at 19:46, 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.
>
> Signed-off-by: Andrew Davis <afd@ti.com>
> ---
>
> Changes from v1:
>  - Removed flags return from tee_ioctl_shm_alloc()
>

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

-Sumit

>  drivers/tee/tee_core.c   | 2 --
>  include/uapi/linux/tee.h | 4 ----
>  2 files changed, 6 deletions(-)
>
> diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
> index 8aa1a4836b92f..af0f7c603fa46 100644
> --- a/drivers/tee/tee_core.c
> +++ b/drivers/tee/tee_core.c
> @@ -302,7 +302,6 @@ static int tee_ioctl_shm_alloc(struct tee_context *ctx,
>                 return PTR_ERR(shm);
>
>         data.id = shm->id;
> -       data.flags = shm->flags;
>         data.size = shm->size;
>
>         if (copy_to_user(udata, &data, sizeof(data)))
> @@ -339,7 +338,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 v2 1/2] tee: remove tee_shm_va2pa() and tee_shm_pa2va()

[Jens Wiklander]

On Mon, Apr 25, 2022 at 4:16 PM 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>
> Reviewed-by: Sumit Garg <sumit.garg@linaro.org>
> ---
>  drivers/tee/tee_shm.c   | 50 -----------------------------------------
>  include/linux/tee_drv.h | 18 ---------------
>  2 files changed, 68 deletions(-)

Looks good to me, I'm picking up this.

Thanks,
Jens

>
> 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 v2 2/2] tee: remove flags TEE_IOCTL_SHM_MAPPED and TEE_IOCTL_SHM_DMA_BUF

[Jens Wiklander]

On Mon, Apr 25, 2022 at 4:18 PM Sumit Garg <sumit.garg@linaro.org> wrote:
>
> On Mon, 25 Apr 2022 at 19:46, 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.
> >
> > Signed-off-by: Andrew Davis <afd@ti.com>
> > ---
> >
> > Changes from v1:
> >  - Removed flags return from tee_ioctl_shm_alloc()
> >
>
> Acked-by: Sumit Garg <sumit.garg@linaro.org>
>
> -Sumit
>
> >  drivers/tee/tee_core.c   | 2 --
> >  include/uapi/linux/tee.h | 4 ----
> >  2 files changed, 6 deletions(-)


Looks good to me, I'm picking up this.

Thanks,
Jens

> >
> > diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
> > index 8aa1a4836b92f..af0f7c603fa46 100644
> > --- a/drivers/tee/tee_core.c
> > +++ b/drivers/tee/tee_core.c
> > @@ -302,7 +302,6 @@ static int tee_ioctl_shm_alloc(struct tee_context *ctx,
> >                 return PTR_ERR(shm);
> >
> >         data.id = shm->id;
> > -       data.flags = shm->flags;
> >         data.size = shm->size;
> >
> >         if (copy_to_user(udata, &data, sizeof(data)))
> > @@ -339,7 +338,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 v2 2/2] tee: remove flags TEE_IOCTL_SHM_MAPPED and TEE_IOCTL_SHM_DMA_BUF

[Eugene Syromiatnikov]

On Mon, Apr 25, 2022 at 09:16:17AM -0500, Andrew Davis 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.

This change has broken build of strace's test suite against the latest kernel
headers[1];  the usage prety much shows up in the Debian's code search[2].

[1] https://github.com/strace/strace/runs/6794205205?check_suite_focus=true#step:4:3862
[2] https://codesearch.debian.net/search?q=TEE_IOCTL_SHM_MAPPED+package%3A%5CQstrace%5CE&literal=1

> 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.

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

[Andrew Davis]

On 6/8/22 8:52 AM, Eugene Syromiatnikov wrote:
> On Mon, Apr 25, 2022 at 09:16:17AM -0500, Andrew Davis 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.
> 
> This change has broken build of strace's test suite against the latest kernel
> headers[1];  the usage prety much shows up in the Debian's code search[2].
> 
> [1] https://github.com/strace/strace/runs/6794205205?check_suite_focus=true#step:4:3862
> [2] https://codesearch.debian.net/search?q=TEE_IOCTL_SHM_MAPPED+package%3A%5CQstrace%5CE&literal=1
> 

Thanks for the headsup, I've sent a patch to fix strace tests.

Andrew

>> 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.
>