On Mon, Sep 14, 2020 at 1:59 AM Jens Wiklander jens.wiklander@linaro.org wrote:

On Sun, Sep 13, 2020 at 10:12:11AM +0530, Souptick Joarder wrote:

...

When shm->num_pages <= 0, we should avoid calling release_registered_pages() in error handling path.

What are we fixing?

Current code is working fine and this patch is not needed. Sorry for the noise.

...

Signed-off-by: Souptick Joarder jrdr.linux@gmail.com Cc: John Hubbard jhubbard@nvidia.com

---

drivers/tee/tee_shm.c | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/tee/tee_shm.c b/drivers/tee/tee_shm.c index 00472f5..e517d9f 100644 --- a/drivers/tee/tee_shm.c +++ b/drivers/tee/tee_shm.c @@ -260,8 +260,7 @@ struct tee_shm *tee_shm_register(struct tee_context *ctx, unsigned long addr, rc = get_kernel_pages(kiov, num_pages, 0, shm->pages); kfree(kiov); }

• if (rc > 0)
  

•         shm->num_pages = rc;
  

• shm->num_pages = rc;
  

Why not avoiding assigning invalid values to shm->num_pages? By the way, shm->num_pages is a size_t.

...

  if (rc != num_pages) {
          if (rc >= 0)
                  rc = -ENOMEM;

@@ -309,7 +308,9 @@ struct tee_shm *tee_shm_register(struct tee_context *ctx, unsigned long addr, idr_remove(&teedev->idr, shm->id); mutex_unlock(&teedev->mutex); }

•         release_registered_pages(shm);
  

•         if (shm->pages && (shm->num_pages > 0))
  

•                 release_registered_pages(shm);
  

With this we'll leak if shm->pages has been assigned something.

...

  }
  kfree(shm);
  teedev_ctx_put(ctx);