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] [day] [month] [year] [list]

Message-ID: <CAMRc=MfUzvxmOYsuDKnp3HGvNYYdYFK0Le0aFkFVeHoqoL3Kog@mail.gmail.com>
Date: Wed, 23 Oct 2024 09:31:49 +0200
From: Bartosz Golaszewski <brgl@...ev.pl>
To: Kuldeep Singh <quic_kuldsing@...cinc.com>
Cc: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>, Bjorn Andersson <andersson@...nel.org>, 
	Konrad Dybcio <konradybcio@...nel.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 Tue, Oct 22, 2024 at 8:34 PM Kuldeep Singh <quic_kuldsing@...cinc.com> wrote:
>
>
>
> On 10/22/2024 12:27 PM, Bartosz Golaszewski wrote:
> > On Tue, 22 Oct 2024 at 07:43, Kuldeep Singh <quic_kuldsing@...cinc.com> wrote:
> >>
> >>
> >>
> >> On 10/16/2024 2:31 PM, Kuldeep Singh wrote:
> >>>
> >>> On 10/14/2024 6:38 PM, Bartosz Golaszewski wrote:
> >>>> 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.
> >>>
> >>> 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
> >>>
> >>
> >> Bartosz,
> >> I am thinking to remove below check in next rev like mentioned above.
> >> https://elixir.bootlin.com/linux/v6.12-rc3/source/drivers/firmware/qcom/qcom_tzmem.c#L268
> >>
> >> Do you have any other opinion here?
> >> Please let me know.
> >>
> >
> > No, let's keep the NULL-pointer check and add it to qcom_tzmem_free(),
> > I'm not against it. I was just saying that in the latter case it will
> > already be handled by the radix tree lookup.
>
> Hey, I think you misread my comment. Let me explain more.
> As agreed, Will drop (!config->policy) check from qcom_tzmem_pool_new
> because it's already present.
> https://elixir.bootlin.com/linux/v6.12-rc3/source/drivers/firmware/qcom/qcom_tzmem.c#L218
>
> Keep (!vaddr) check in qcom_tzmem_free as discussed above.
> https://elixir.bootlin.com/linux/v6.12-rc3/source/drivers/firmware/qcom/qcom_tzmem.c#L411
>
> And last thing, like we don't check (!pool) in qcom_tzmem_alloc as it
> cannot be null, same way I believe (!pool) is unnecessary in
> qcom_tzmem_pool_free as qcom_tzmem_pool_new should return valid pool and
> if not, should be handled by calling driver.
> https://elixir.bootlin.com/linux/v6.12-rc3/source/drivers/firmware/qcom/qcom_tzmem.c#L369
> https://elixir.bootlin.com/linux/v6.12-rc3/source/drivers/firmware/qcom/qcom_tzmem.c#L268
>

Well I would say this is just churn if it's already there but yeah it
cannot be NULL so I won't object.

Bart