linux-kernel - Re: [PATCH v2 2/2] firmware: qcom: qcom

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAMRc=MfR8rK3EnZx3_9rxkwgv6f8jA4X0u0cGBkpJ89d5i1MKw@mail.gmail.com>
Date: Mon, 14 Oct 2024 15:08:53 +0200
From: Bartosz Golaszewski <brgl@...ev.pl>
To: Kuldeep Singh <quic_kuldsing@...cinc.com>
Cc: Bjorn Andersson <andersson@...nel.org>, Konrad Dybcio <konradybcio@...nel.org>, 
	Bartosz Golaszewski <bartosz.golaszewski@...aro.org>, linux-arm-msm@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/2] firmware: qcom: qcom_tzmem: Implement sanity checks

On Mon, Oct 14, 2024 at 1:19 PM Kuldeep Singh <quic_kuldsing@...cinc.com> 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@...cinc.com>
> ---
>  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.

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

Bart

> --
> 2.34.1
>
>