Re: [PATCH 01/15] media: coda: fix memory corruption in case more than 32 instances are opened

[Ian Arkver]

Hi Philipp,

On 05/11/2018 15:24, Philipp Zabel wrote:

The ffz() return value is undefined if the instance mask does not
contain any zeros. If it returned 32, the following set_bit would
corrupt the debugfs_root pointer.
Switch to IDA for context index allocation. This also removes the
artificial 32 instance limit for all except CodaDx6.

Signed-off-by: Philipp Zabel <p.za...@pengutronix.de>
---
  drivers/media/platform/coda/coda-common.c | 25 ++++++++---------------
  drivers/media/platform/coda/coda.h        |  2 +-
  2 files changed, 10 insertions(+), 17 deletions(-)

diff --git a/drivers/media/platform/coda/coda-common.c 
b/drivers/media/platform/coda/coda-common.c
index 2848ea5f464d..cbb59c2f3a82 100644
--- a/drivers/media/platform/coda/coda-common.c
+++ b/drivers/media/platform/coda/coda-common.c
@@ -2099,17 +2099,6 @@ int coda_decoder_queue_init(void *priv, struct vb2_queue 
*src_vq,
        return coda_queue_init(priv, dst_vq);
  }

-static int coda_next_free_instance(struct coda_dev *dev)

-{
-       int idx = ffz(dev->instance_mask);
-
-       if ((idx < 0) ||
-           (dev->devtype->product == CODA_DX6 && idx > CODADX6_MAX_INSTANCES))
-               return -EBUSY;
-
-       return idx;
-}
-
  /*
   * File operations
   */
@@ -2118,7 +2107,8 @@ static int coda_open(struct file *file)
  {
        struct video_device *vdev = video_devdata(file);
        struct coda_dev *dev = video_get_drvdata(vdev);
-       struct coda_ctx *ctx = NULL;
+       struct coda_ctx *ctx;
+       unsigned int max = ~0;
        char *name;
        int ret;
        int idx;
@@ -2127,12 +2117,13 @@ static int coda_open(struct file *file)
        if (!ctx)
                return -ENOMEM;

- idx = coda_next_free_instance(dev);

+       if (dev->devtype->product == CODA_DX6)
+               max = CODADX6_MAX_INSTANCES - 1;
+       idx = ida_alloc_max(&dev->ida, max, GFP_KERNEL);
        if (idx < 0) {
                ret = idx;
                goto err_coda_max;
        }
-       set_bit(idx, &dev->instance_mask);

name = kasprintf(GFP_KERNEL, "context%d", idx);

        if (!name) {
@@ -2241,8 +2232,8 @@ static int coda_open(struct file *file)
  err_pm_get:
        v4l2_fh_del(&ctx->fh);
        v4l2_fh_exit(&ctx->fh);
-       clear_bit(ctx->idx, &dev->instance_mask);
  err_coda_name_init:
+       ida_free(&dev->ida, ctx->idx);
  err_coda_max:
        kfree(ctx);
        return ret;
@@ -2284,7 +2275,7 @@ static int coda_release(struct file *file)
        pm_runtime_put_sync(&dev->plat_dev->dev);
        v4l2_fh_del(&ctx->fh);
        v4l2_fh_exit(&ctx->fh);
-       clear_bit(ctx->idx, &dev->instance_mask);
+       ida_free(&dev->ida, ctx->idx);
        if (ctx->ops->release)
                ctx->ops->release(ctx);
        debugfs_remove_recursive(ctx->debugfs_entry);
@@ -2745,6 +2736,7 @@ static int coda_probe(struct platform_device *pdev)

mutex_init(&dev->dev_mutex);

        mutex_init(&dev->coda_mutex);
+       ida_init(&dev->ida);

dev->debugfs_root = debugfs_create_dir("coda", NULL);

        if (!dev->debugfs_root)
@@ -2832,6 +2824,7 @@ static int coda_remove(struct platform_device *pdev)
        coda_free_aux_buf(dev, &dev->tempbuf);
        coda_free_aux_buf(dev, &dev->workbuf);
        debugfs_remove_recursive(dev->debugfs_root);
+       ida_destroy(&dev->ida);
        return 0;
  }

diff --git a/drivers/media/platform/coda/coda.h b/drivers/media/platform/coda/coda.h

index 19ac0b9dc6eb..b6cd14ee91ea 100644
--- a/drivers/media/platform/coda/coda.h
+++ b/drivers/media/platform/coda/coda.h

Should you add:
#include <linux/idr.h>
to this header?

Regards,
Ian

@@ -95,7 +95,7 @@ struct coda_dev {
        struct workqueue_struct *workqueue;
        struct v4l2_m2m_dev     *m2m_dev;
        struct list_head        instances;
-       unsigned long           instance_mask;
+       struct ida              ida;
        struct dentry           *debugfs_root;
  };