* [PATCH 1/3] media: venus: use NULL instead of zero for pointers @ 2021-04-08 7:40 Mauro Carvalho Chehab 2021-04-08 7:40 ` [PATCH 3/3] media: venus: don't de-reference NULL pointers at IRQ time Mauro Carvalho Chehab 2021-04-09 11:08 ` [PATCH 1/3] media: venus: use NULL instead of zero for pointers Stanimir Varbanov 0 siblings, 2 replies; 4+ messages in thread From: Mauro Carvalho Chehab @ 2021-04-08 7:40 UTC (permalink / raw) Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, Andy Gross, Bjorn Andersson, Mauro Carvalho Chehab, Stanimir Varbanov, linux-arm-msm, linux-kernel, linux-media 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* [PATCH 3/3] media: venus: don't de-reference NULL pointers at IRQ time 2021-04-08 7:40 [PATCH 1/3] media: venus: use NULL instead of zero for pointers Mauro Carvalho Chehab @ 2021-04-08 7:40 ` Mauro Carvalho Chehab 2021-04-09 11:08 ` Stanimir Varbanov 2021-04-09 11:08 ` [PATCH 1/3] media: venus: use NULL instead of zero for pointers Stanimir Varbanov 1 sibling, 1 reply; 4+ messages in thread From: Mauro Carvalho Chehab @ 2021-04-08 7:40 UTC (permalink / raw) Cc: linuxarm, mauro.chehab, Mauro Carvalho Chehab, Andy Gross, Bjorn Andersson, Mauro Carvalho Chehab, Stanimir Varbanov, linux-arm-msm, linux-kernel, linux-media 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 ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 3/3] media: venus: don't de-reference NULL pointers at IRQ time 2021-04-08 7:40 ` [PATCH 3/3] media: venus: don't de-reference NULL pointers at IRQ time Mauro Carvalho Chehab @ 2021-04-09 11:08 ` Stanimir Varbanov 0 siblings, 0 replies; 4+ messages in thread From: Stanimir Varbanov @ 2021-04-09 11:08 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: linuxarm, mauro.chehab, Andy Gross, Bjorn Andersson, Mauro Carvalho Chehab, Stanimir Varbanov, linux-arm-msm, linux-kernel, linux-media 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/3] media: venus: use NULL instead of zero for pointers 2021-04-08 7:40 [PATCH 1/3] media: venus: use NULL instead of zero for pointers Mauro Carvalho Chehab 2021-04-08 7:40 ` [PATCH 3/3] media: venus: don't de-reference NULL pointers at IRQ time Mauro Carvalho Chehab @ 2021-04-09 11:08 ` Stanimir Varbanov 1 sibling, 0 replies; 4+ messages in thread From: Stanimir Varbanov @ 2021-04-09 11:08 UTC (permalink / raw) To: Mauro Carvalho Chehab Cc: linuxarm, mauro.chehab, Andy Gross, Bjorn Andersson, Mauro Carvalho Chehab, Stanimir Varbanov, linux-arm-msm, linux-kernel, linux-media 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 ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-04-09 11:14 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-08 7:40 [PATCH 1/3] media: venus: use NULL instead of zero for pointers Mauro Carvalho Chehab 2021-04-08 7:40 ` [PATCH 3/3] media: venus: don't de-reference NULL pointers at IRQ time Mauro Carvalho Chehab 2021-04-09 11:08 ` Stanimir Varbanov 2021-04-09 11:08 ` [PATCH 1/3] media: venus: use NULL instead of zero for pointers Stanimir Varbanov
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).