[PATCH 1/3] media: venus: use NULL instead of zero for pointers

[PATCH 1/3] media: venus: use NULL instead of zero for pointers

[Mauro Carvalho Chehab]

As reported by sparse:

	drivers/media/platform/qcom/venus/core.c:227:41: warning: Using plain integer as NULL pointer
	drivers/media/platform/qcom/venus/core.c:228:34: warning: Using plain integer as NULL pointer

Two vars are using zero instead of NULL for pointers. Not really
an issue, but using NULL makes it clearer that the init data is
expecting a pointer.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/platform/qcom/venus/core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
index f5b88b96f5f7..4451e3c11bc0 100644
--- a/drivers/media/platform/qcom/venus/core.c
+++ b/drivers/media/platform/qcom/venus/core.c
@@ -224,8 +224,8 @@ static void venus_assign_register_offsets(struct venus_core *core)
 		core->cpu_cs_base = core->base + CPU_CS_BASE;
 		core->cpu_ic_base = core->base + CPU_IC_BASE;
 		core->wrapper_base = core->base + WRAPPER_BASE;
-		core->wrapper_tz_base = 0;
-		core->aon_base = 0;
+		core->wrapper_tz_base = NULL;
+		core->aon_base = NULL;
 	}
 }
 
-- 
2.30.2

[PATCH 3/3] media: venus: don't de-reference NULL pointers at IRQ time

[Mauro Carvalho Chehab]

Smatch is warning that:
	drivers/media/platform/qcom/venus/hfi_venus.c:1100 venus_isr() warn: variable dereferenced before check 'hdev' (see line 1097)

The logic basically does:
	hdev = to_hfi_priv(core);

with is translated to:
	hdev = core->priv;

If the IRQ code can receive a NULL pointer for hdev, there's
a bug there, as it will first try to de-reference the pointer,
and then check if it is null.

After looking at the code, it seems that this indeed can happen:
Basically, the venus IRQ thread is started with:
	devm_request_threaded_irq()
So, it will only be freed after the driver unbinds.

In order to prevent the IRQ code to work with freed data,
the logic at venus_hfi_destroy() sets core->priv to NULL,
which would make the IRQ code to ignore any pending IRQs.

There is, however a race condition, as core->priv is set
to NULL only after being freed. So, we need also to move the
core->priv = NULL to happen earlier.

Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
---
 drivers/media/platform/qcom/venus/hfi_venus.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
index cebb20cf371f..ce98c523b3c6 100644
--- a/drivers/media/platform/qcom/venus/hfi_venus.c
+++ b/drivers/media/platform/qcom/venus/hfi_venus.c
@@ -1094,12 +1094,15 @@ static irqreturn_t venus_isr(struct venus_core *core)
 {
 	struct venus_hfi_device *hdev = to_hfi_priv(core);
 	u32 status;
-	void __iomem *cpu_cs_base = hdev->core->cpu_cs_base;
-	void __iomem *wrapper_base = hdev->core->wrapper_base;
+	void __iomem *cpu_cs_base;
+	void __iomem *wrapper_base;
 
 	if (!hdev)
 		return IRQ_NONE;
 
+	cpu_cs_base = hdev->core->cpu_cs_base;
+	wrapper_base = hdev->core->wrapper_base;
+
 	status = readl(wrapper_base + WRAPPER_INTR_STATUS);
 	if (IS_V6(core)) {
 		if (status & WRAPPER_INTR_STATUS_A2H_MASK ||
@@ -1650,10 +1653,10 @@ void venus_hfi_destroy(struct venus_core *core)
 {
 	struct venus_hfi_device *hdev = to_hfi_priv(core);
 
+	core->priv = NULL;
 	venus_interface_queues_release(hdev);
 	mutex_destroy(&hdev->lock);
 	kfree(hdev);
-	core->priv = NULL;
 	core->ops = NULL;
 }
 
-- 
2.30.2

Re: [PATCH 3/3] media: venus: don't de-reference NULL pointers at IRQ time

[Stanimir Varbanov]

Hi Mauro,

On 4/8/21 10:40 AM, Mauro Carvalho Chehab wrote:
> Smatch is warning that:
> 	drivers/media/platform/qcom/venus/hfi_venus.c:1100 venus_isr() warn: variable dereferenced before check 'hdev' (see line 1097)
> 
> The logic basically does:
> 	hdev = to_hfi_priv(core);
> 
> with is translated to:
> 	hdev = core->priv;
> 
> If the IRQ code can receive a NULL pointer for hdev, there's
> a bug there, as it will first try to de-reference the pointer,
> and then check if it is null.
> 
> After looking at the code, it seems that this indeed can happen:
> Basically, the venus IRQ thread is started with:
> 	devm_request_threaded_irq()
> So, it will only be freed after the driver unbinds.
> 
> In order to prevent the IRQ code to work with freed data,
> the logic at venus_hfi_destroy() sets core->priv to NULL,
> which would make the IRQ code to ignore any pending IRQs.
> 
> There is, however a race condition, as core->priv is set
> to NULL only after being freed. So, we need also to move the
> core->priv = NULL to happen earlier.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
>  drivers/media/platform/qcom/venus/hfi_venus.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)

Acked-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>

> 
> diff --git a/drivers/media/platform/qcom/venus/hfi_venus.c b/drivers/media/platform/qcom/venus/hfi_venus.c
> index cebb20cf371f..ce98c523b3c6 100644
> --- a/drivers/media/platform/qcom/venus/hfi_venus.c
> +++ b/drivers/media/platform/qcom/venus/hfi_venus.c
> @@ -1094,12 +1094,15 @@ static irqreturn_t venus_isr(struct venus_core *core)
>  {
>  	struct venus_hfi_device *hdev = to_hfi_priv(core);
>  	u32 status;
> -	void __iomem *cpu_cs_base = hdev->core->cpu_cs_base;
> -	void __iomem *wrapper_base = hdev->core->wrapper_base;
> +	void __iomem *cpu_cs_base;
> +	void __iomem *wrapper_base;
>  
>  	if (!hdev)
>  		return IRQ_NONE;
>  
> +	cpu_cs_base = hdev->core->cpu_cs_base;
> +	wrapper_base = hdev->core->wrapper_base;
> +
>  	status = readl(wrapper_base + WRAPPER_INTR_STATUS);
>  	if (IS_V6(core)) {
>  		if (status & WRAPPER_INTR_STATUS_A2H_MASK ||
> @@ -1650,10 +1653,10 @@ void venus_hfi_destroy(struct venus_core *core)
>  {
>  	struct venus_hfi_device *hdev = to_hfi_priv(core);
>  
> +	core->priv = NULL;
>  	venus_interface_queues_release(hdev);
>  	mutex_destroy(&hdev->lock);
>  	kfree(hdev);
> -	core->priv = NULL;
>  	core->ops = NULL;
>  }
>  
> 

-- 
regards,
Stan

Re: [PATCH 1/3] media: venus: use NULL instead of zero for pointers

[Stanimir Varbanov]

Hi Mauro,

On 4/8/21 10:40 AM, Mauro Carvalho Chehab wrote:
> As reported by sparse:
> 
> 	drivers/media/platform/qcom/venus/core.c:227:41: warning: Using plain integer as NULL pointer
> 	drivers/media/platform/qcom/venus/core.c:228:34: warning: Using plain integer as NULL pointer
> 
> Two vars are using zero instead of NULL for pointers. Not really
> an issue, but using NULL makes it clearer that the init data is
> expecting a pointer.
> 
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> ---
>  drivers/media/platform/qcom/venus/core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Acked-by: Stanimir Varbanov <stanimir.varbanov@linaro.org>

> 
> diff --git a/drivers/media/platform/qcom/venus/core.c b/drivers/media/platform/qcom/venus/core.c
> index f5b88b96f5f7..4451e3c11bc0 100644
> --- a/drivers/media/platform/qcom/venus/core.c
> +++ b/drivers/media/platform/qcom/venus/core.c
> @@ -224,8 +224,8 @@ static void venus_assign_register_offsets(struct venus_core *core)
>  		core->cpu_cs_base = core->base + CPU_CS_BASE;
>  		core->cpu_ic_base = core->base + CPU_IC_BASE;
>  		core->wrapper_base = core->base + WRAPPER_BASE;
> -		core->wrapper_tz_base = 0;
> -		core->aon_base = 0;
> +		core->wrapper_tz_base = NULL;
> +		core->aon_base = NULL;
>  	}
>  }
>  
> 

-- 
regards,
Stan