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?
>
> Signed-off-by: Souptick Joarder <jrdr.li...@gmail.com>
> Cc: John Hubbard <jhubb...@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);
> --
> 1.9.1
>
Thanks,
Jens
