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
