On 10/14/2024 6:38 PM, Bartosz Golaszewski wrote: > On Mon, Oct 14, 2024 at 1:19 PM Kuldeep Singh <quic_kuldsing@xxxxxxxxxxx> wrote: >> >> The qcom_tzmem driver currently has exposed APIs that lack validations >> on required input parameters. This oversight can lead to unexpected null >> pointer dereference crashes. >> > > The commit message is not true. None of the things you changed below > can lead to a NULL-pointer dereference.> >> To address this issue, add sanity for required input parameters. >> >> Signed-off-by: Kuldeep Singh <quic_kuldsing@xxxxxxxxxxx> >> --- >> drivers/firmware/qcom/qcom_tzmem.c | 6 ++++++ >> 1 file changed, 6 insertions(+) >> >> diff --git a/drivers/firmware/qcom/qcom_tzmem.c b/drivers/firmware/qcom/qcom_tzmem.c >> index 92b365178235..977e48fec32f 100644 >> --- a/drivers/firmware/qcom/qcom_tzmem.c >> +++ b/drivers/firmware/qcom/qcom_tzmem.c >> @@ -203,6 +203,9 @@ qcom_tzmem_pool_new(const struct qcom_tzmem_pool_config *config) >> >> might_sleep(); >> >> + if (!config->policy) >> + return ERR_PTR(-EINVAL); > > This is already handled by the default case of the switch. Ack. Need to drop. https://elixir.bootlin.com/linux/v6.12-rc3/source/drivers/firmware/qcom/qcom_tzmem.c#L218 While examining qcom_tzmem_pool_free under the same principle, it appears the following check is unnecessary. https://elixir.bootlin.com/linux/v6.12-rc3/source/drivers/firmware/qcom/qcom_tzmem.c#L268 > >> + >> switch (config->policy) { >> case QCOM_TZMEM_POLICY_STATIC: >> if (!config->initial_size) >> @@ -412,6 +415,9 @@ void qcom_tzmem_free(void *vaddr) >> { >> struct qcom_tzmem_chunk *chunk; >> >> + if (!vaddr) >> + return; >> + >> scoped_guard(spinlock_irqsave, &qcom_tzmem_chunks_lock) >> chunk = radix_tree_delete_item(&qcom_tzmem_chunks, >> (unsigned long)vaddr, NULL); > > This would lead to a WARN() as the lookup would inevitably fail. We > can possibly keep this bit but please change the commit message. Sure, will reword commit message. -- Regards Kuldeep
