Re: [PATCH 2/2] firmware: qcom: qcom_tzmem: Implement sanity checks

[Bartosz Golaszewski]

On Mon, 7 Oct 2024 at 22:44, Kuldeep Singh <quic_kuldsing@xxxxxxxxxxx> wrote:
>
>
>
> On 10/8/2024 1:43 AM, Dmitry Baryshkov wrote:
> > On Mon, 7 Oct 2024 at 21:17, Kuldeep Singh <quic_kuldsing@xxxxxxxxxxx> 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