linux-kernel - Re: [PATCH 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: <CACMJSesF1tQyG85uLNbdsGqqq4xY39-KKwW=3QRMOcvpOCNsxQ@mail.gmail.com>
Date: Mon, 7 Oct 2024 22:56:36 +0200
From: Bartosz Golaszewski <bartosz.golaszewski@...aro.org>
To: Kuldeep Singh <quic_kuldsing@...cinc.com>
Cc: Dmitry Baryshkov <dmitry.baryshkov@...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 2/2] firmware: qcom: qcom_tzmem: Implement sanity checks

On Mon, 7 Oct 2024 at 22:44, Kuldeep Singh <quic_kuldsing@...cinc.com> wrote:
>
>
>
> On 10/8/2024 1:43 AM, Dmitry Baryshkov wrote:
> > On Mon, 7 Oct 2024 at 21:17, Kuldeep Singh <quic_kuldsing@...cinc.com> wrote:
> >>
> >>
> >> On 10/7/2024 1:00 AM, Dmitry Baryshkov wrote:
> >>> On Sat, Oct 05, 2024 at 07:31:50PM GMT, Kuldeep Singh wrote:
> >>>> The qcom_tzmem driver currently has multiple exposed APIs that lack
> >>>> validations on input parameters. This oversight can lead to unexpected
> >>>> crashes due to null pointer dereference when incorrect inputs are
> >>>> provided.
> >>>>
> >>>> To address this issue, add required sanity for all input parameters in
> >>>> the exposed APIs.
> >>>
> >>> Please don't be overprotective. Inserting guarding conditions is good,
> >>> inserting useless guarding conditions is bad, it complicates the driver
> >>> and makes it harder to follow. Please validate return data rather than
> >>> adding extra checks to the functions.
> >>
> >> Sure, I’ll remove the redundant checks.
> >> Please see below for explanations.
> >>
> >> My intention here is to handle erroneous conditions gracefully to avoid system crashes, as crashes can be detrimental.
> >
> > Please fix the callers first, rather than adding band-aids.
>
> I see your point and understand the emphasis.
> I'll submit v2 as per suggestion.
>

Just to add to what Dmitry said: when you see this kind of checks in
the kernel, it's typically because it makes functional sense for the
API. For instance clk_get_clock_optional() can return NULL and it's
considered a no-error situation but in this case clk_set_rate() must
check whether struct clk * is NULL and it returns 0 as if the
underlying set-rate operation succeeded.

On the other hand there's no such situation where a NULL-pointer
returned by kmalloc() could be considered successful and so we don't
do NULL-checks whenever kmalloc'ed memory is expected as argument.

Similarly here: there's no chance qcom_tzmem_pool_new() will return
NULL so there's no reason to check it and if it returns an ERR_PTR()
then we have to trust the user to check the return value and not pass
it on.

If anything: you could add __must_check to the relevant definitions here.

Bart