2021-04-08 07:40:59

by Mauro Carvalho Chehab

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

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 <[email protected]>
---
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

2021-04-08 07:42:15

by Mauro Carvalho Chehab

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

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 <[email protected]>
---
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

2021-04-09 11:15:30

by Stanimir Varbanov

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

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 <[email protected]>
> ---
> drivers/media/platform/qcom/venus/core.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)

Acked-by: Stanimir Varbanov <[email protected]>

>
> 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

2021-04-09 11:15:36

by Stanimir Varbanov

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

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 <[email protected]>
> ---
> drivers/media/platform/qcom/venus/hfi_venus.c | 9 ++++++---
> 1 file changed, 6 insertions(+), 3 deletions(-)

Acked-by: Stanimir Varbanov <[email protected]>

>
> 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