media: iris: Fix issues in the driver unbind path

The driver can be unbound through sysfs while a client has a video session open. The core is devm allocated and freed when remove() returns, so the open sessions then use freed memory.

Only the core structure needs to outlive remove(), because it is the only thing the file operations of an open session still use. Allocate it with kzalloc and tie it to the reference count of its v4l2_device, which every registered video device holds. It is freed when the last file descriptor is closed. The other devm resources (register mapping, response packet, clocks, power domains, interconnects) are only used by the power on/off sequence, the clock and bandwidth votes and the interrupt handler. All of them are released with the unbind, after the core is shut down.

To make sure nothing uses them afterwards, set a removing flag at the start of iris_remove(). It makes core init, new sessions, command writes and the clock and interconnect votes fail with -ENODEV, so a client still holding a file descriptor cannot power the hardware on or vote for resources that are gone. A client waiting for the firmware only times out and returns an error without touching them. Runtime PM is also disabled explicitly in iris_remove() instead of with devm, because its callbacks use the core.

Fixes 38506cb7e8d2 ("media: iris: add platform driver for iris video device")

Reported-by sashiko-bot <[email protected]>

Closes https://lore.kernel.org/all/[email protected] open here

Cc [email protected]

Signed-off-by Vishnu Reddy <[email protected]>

---
  drivers/media/platform/qcom/iris/iris_core.c      | 10 +++++-
  drivers/media/platform/qcom/iris/iris_core.h      |  2 ++
  drivers/media/platform/qcom/iris/iris_hfi_queue.c | 15 +++++++-
  drivers/media/platform/qcom/iris/iris_power.c     | 13 +++++++
  drivers/media/platform/qcom/iris/iris_probe.c     | 44 ++++++++++++++++++-----
  drivers/media/platform/qcom/iris/iris_vidc.c      |  3 ++
  6 files changed, 76 insertions(+), 11 deletions(-)

drivers/media/platform/qcom/iris/iris_core.c +9 -1

index d558475986ff0..bdac6b8c98e44 100644
@@ -43,7 +43,10 @@ static int iris_wait_for_system_response(struct iris_core *core)
  	ret = wait_for_completion_timeout(&core->core_init_done,
  					  msecs_to_jiffies(HW_RESPONSE_TIMEOUT_VALUE));
  	if (!ret) {
-		core->state = IRIS_CORE_ERROR;
+		mutex_lock(&core->lock);
+		if (!core->removing)
+			core->state = IRIS_CORE_ERROR;
+		mutex_unlock(&core->lock);
  		return -ETIMEDOUT;
  	}
 

@@ -56,6 +59,11 @@ int iris_core_init(struct iris_core *core)
  	int ret;
 
  	mutex_lock(&core->lock);
+	if (core->removing) {
+		ret = -ENODEV;
+		goto exit;
+	}
+
  	if (core->state == IRIS_CORE_INIT) {
  		ret = 0;
  		goto exit;

drivers/media/platform/qcom/iris/iris_core.h +2 -0

index 3e4cf652bf39a..379f0666a42cb 100644
@@ -62,6 +62,7 @@ struct qcom_ubwc_cfg_data;
   * @iris_firmware_desc: a pointer to the firmware-specific descriptive data
   * @ubwc_cfg: UBWC configuration for the platform
   * @state: current state of core
+ * @removing: set when driver removal starts, blocks core init and resource votes
   * @iface_q_table_daddr: device address for interface queue table memory
   * @sfr_daddr: device address for SFR (Sub System Failure Reason) register memory
   * @iface_q_table_vaddr: virtual address for interface queue table memory

@@ -108,6 +109,7 @@ struct iris_core {
  	const struct iris_firmware_desc		*iris_firmware_desc;
  	const struct qcom_ubwc_cfg_data		*ubwc_cfg;
  	enum iris_core_state			state;
+	bool					removing;
  	dma_addr_t				iface_q_table_daddr;
  	dma_addr_t				sfr_daddr;
  	void					*iface_q_table_vaddr;

drivers/media/platform/qcom/iris/iris_hfi_queue.c +8 -1

index 545bbe75e1740..e0cfa5107d1d9 100644
@@ -128,13 +128,26 @@ int iris_hfi_queue_cmd_write_locked(struct iris_core *core, void *pkt, u32 pkt_s
 
  int iris_hfi_queue_cmd_write(struct iris_core *core, void *pkt, u32 pkt_size)
  {
-	int ret;
+	int ret = 0;
+
+	mutex_lock(&core->lock);
+	if (core->removing)
+		ret = -ENODEV;
+	mutex_unlock(&core->lock);
+	if (ret)
+		return ret;
 
  	ret = pm_runtime_resume_and_get(core->dev);

Quoted 111 lines · Vishnu Reddy

The driver can be unbound through sysfs while a client has a video session open. The core is devm allocated and freed when remove() returns, so the open sessions then use freed memory.

Only the core structure needs to outlive remove(), because it is the only thing the file operations of an open session still use. Allocate it with kzalloc and tie it to the reference count of its v4l2_device, which every registered video device holds. It is freed when the last file descriptor is closed. The other devm resources (register mapping, response packet, clocks, power domains, interconnects) are only used by the power on/off sequence, the clock and bandwidth votes and the interrupt handler. All of them are released with the unbind, after the core is shut down.

To make sure nothing uses them afterwards, set a removing flag at the start of iris_remove(). It makes core init, new sessions, command writes and the clock and interconnect votes fail with -ENODEV, so a client still holding a file descriptor cannot power the hardware on or vote for resources that are gone. A client waiting for the firmware only times out and returns an error without touching them. Runtime PM is also disabled explicitly in iris_remove() instead of with devm, because its callbacks use the core.

Fixes 38506cb7e8d2 ("media: iris: add platform driver for iris video device")

Reported-by sashiko-bot <[email protected]>

Closes https://lore.kernel.org/all/[email protected] open here

Cc [email protected]

Signed-off-by Vishnu Reddy <[email protected]>

---
  drivers/media/platform/qcom/iris/iris_core.c      | 10 +++++- drivers/media/platform/qcom/iris/iris_core.h      |  2 ++ drivers/media/platform/qcom/iris/iris_hfi_queue.c | 15 +++++++- drivers/media/platform/qcom/iris/iris_power.c     | 13 +++++++ drivers/media/platform/qcom/iris/iris_probe.c     | 44 ++++++++++++++++++----- drivers/media/platform/qcom/iris/iris_vidc.c      |  3 ++

  6 files changed, 76 insertions(+), 11 deletions(-)

drivers/media/platform/qcom/iris/iris_core.c +4 -1

index d558475986ff0..bdac6b8c98e44 100644
@@ -43,7 +43,10 @@ static int iris_wait_for_system_response(struct iris_core
 *core)
       ret = wait_for_completion_timeout(&core->core_init_done,
                         msecs_to_jiffies(HW_RESPONSE_TIMEOUT_VALUE));
       if (!ret) {
-        core->state = IRIS_CORE_ERROR;
+        mutex_lock(&core->lock);
+        if (!core->removing)
+            core->state = IRIS_CORE_ERROR;
+        mutex_unlock(&core->lock);
           return -ETIMEDOUT;
       }

  @@ -56,6 +59,11 @@ int iris_core_init(struct iris_core *core) int ret;
        mutex_lock(&core->lock);
+    if (core->removing) {
+        ret = -ENODEV;
+        goto exit;
+    }
+
      if (core->state == IRIS_CORE_INIT) {
          ret = 0;
          goto exit;

drivers/media/platform/qcom/iris/iris_core.h +1 -0

index 3e4cf652bf39a..379f0666a42cb 100644
@@ -62,6 +62,7 @@ struct qcom_ubwc_cfg_data;
    * @iris_firmware_desc: a pointer to the firmware-specific descriptive data
    * @ubwc_cfg: UBWC configuration for the platform
    * @state: current state of core
+ * @removing: set when driver removal starts, blocks core init and resource

votes
   * @iface_q_table_daddr: device address for interface queue table memory
   * @sfr_daddr: device address for SFR (Sub System Failure Reason) register memory
   * @iface_q_table_vaddr: virtual address for interface queue table memory

drivers/media/platform/qcom/iris/iris_core.h +1 -0

@@ -108,6 +109,7 @@ struct iris_core {
       const struct iris_firmware_desc        *iris_firmware_desc;
       const struct qcom_ubwc_cfg_data        *ubwc_cfg;
       enum iris_core_state            state;
+    bool                    removing;
       dma_addr_t                iface_q_table_daddr;
       dma_addr_t                sfr_daddr;
       void                    *iface_q_table_vaddr;

drivers/media/platform/qcom/iris/iris_hfi_queue.c +0 -0

index 545bbe75e1740..e0cfa5107d1d9 100644
@@ -128,13 +128,26 @@ int iris_hfi_queue_cmd_write_locked(struct iris_core
 *core, void *pkt, u32 pkt_s

    int iris_hfi_queue_cmd_write(struct iris_core *core, void *pkt, u32 pkt_size)
  {
-    int ret;
+    int ret = 0;
+
+    mutex_lock(&core->lock);
+    if (core->removing)
+        ret = -ENODEV;
+    mutex_unlock(&core->lock);
+    if (ret)
+        return ret;
        ret = pm_runtime_resume_and_get(core->dev);
      if (ret < 0)
          return ret;
        mutex_lock(&core->lock);

Quoted 6 lines

+    if (core->removing) {
+        mutex_unlock(&core->lock);
+        ret = -ENODEV;
+        goto exit;
+    }
+

This locking scheme is looks racy, what's the point of this granularity ?

---
bod