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.
Signed-off-by: Kuldeep Singh <quic_kuldsing@quicinc.com>
---
drivers/firmware/qcom/qcom_tzmem.c | 17 ++++++++++++++++-
1 file changed, 16 insertions(+), 1 deletion(-)
diff --git a/drivers/firmware/qcom/qcom_tzmem.c b/drivers/firmware/qcom/qcom_tzmem.c
index 92b365178235..2f2e1f2fa9fc 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 || !config->policy)
+ return ERR_PTR(-EINVAL);
+
switch (config->policy) {
case QCOM_TZMEM_POLICY_STATIC:
if (!config->initial_size)
@@ -316,6 +319,9 @@ devm_qcom_tzmem_pool_new(struct device *dev,
struct qcom_tzmem_pool *pool;
int ret;
+ if (!dev || !config)
+ return ERR_PTR(-EINVAL);
+
pool = qcom_tzmem_pool_new(config);
if (IS_ERR(pool))
return pool;
@@ -366,7 +372,7 @@ void *qcom_tzmem_alloc(struct qcom_tzmem_pool *pool, size_t size, gfp_t gfp)
unsigned long vaddr;
int ret;
- if (!size)
+ if (!pool || !size)
return NULL;
size = PAGE_ALIGN(size);
@@ -412,6 +418,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);
@@ -446,6 +455,9 @@ phys_addr_t qcom_tzmem_to_phys(void *vaddr)
void __rcu **slot;
phys_addr_t ret;
+ if (!vaddr)
+ return 0;
+
guard(spinlock_irqsave)(&qcom_tzmem_chunks_lock);
radix_tree_for_each_slot(slot, &qcom_tzmem_chunks, &iter, 0) {
@@ -466,6 +478,9 @@ EXPORT_SYMBOL_GPL(qcom_tzmem_to_phys);
int qcom_tzmem_enable(struct device *dev)
{
+ if (!dev)
+ return -EINVAL;
+
if (qcom_tzmem_dev)
return -EBUSY;
--
2.34.1On 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.
>
Unless there's good reason for the opposite, I rather see that we define
the API to only accept valid pointers. Then if a client passes a NULL we
get a oops with a nice callstack, which is easy to debug.
The alternative is that we return -EINVAL, which not unlikely is
propagated to some application which may or may not result in a bug
report from a user - without any tangible information about where things
went wrong.
But, if you think there's a good reason, please let me know.
Regards,
Bjorn
> Signed-off-by: Kuldeep Singh <quic_kuldsing@quicinc.com>
> ---
> drivers/firmware/qcom/qcom_tzmem.c | 17 ++++++++++++++++-
> 1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/qcom/qcom_tzmem.c b/drivers/firmware/qcom/qcom_tzmem.c
> index 92b365178235..2f2e1f2fa9fc 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 || !config->policy)
> + return ERR_PTR(-EINVAL);
> +
> switch (config->policy) {
> case QCOM_TZMEM_POLICY_STATIC:
> if (!config->initial_size)
> @@ -316,6 +319,9 @@ devm_qcom_tzmem_pool_new(struct device *dev,
> struct qcom_tzmem_pool *pool;
> int ret;
>
> + if (!dev || !config)
> + return ERR_PTR(-EINVAL);
> +
> pool = qcom_tzmem_pool_new(config);
> if (IS_ERR(pool))
> return pool;
> @@ -366,7 +372,7 @@ void *qcom_tzmem_alloc(struct qcom_tzmem_pool *pool, size_t size, gfp_t gfp)
> unsigned long vaddr;
> int ret;
>
> - if (!size)
> + if (!pool || !size)
> return NULL;
>
> size = PAGE_ALIGN(size);
> @@ -412,6 +418,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);
> @@ -446,6 +455,9 @@ phys_addr_t qcom_tzmem_to_phys(void *vaddr)
> void __rcu **slot;
> phys_addr_t ret;
>
> + if (!vaddr)
> + return 0;
> +
> guard(spinlock_irqsave)(&qcom_tzmem_chunks_lock);
>
> radix_tree_for_each_slot(slot, &qcom_tzmem_chunks, &iter, 0) {
> @@ -466,6 +478,9 @@ EXPORT_SYMBOL_GPL(qcom_tzmem_to_phys);
>
> int qcom_tzmem_enable(struct device *dev)
> {
> + if (!dev)
> + return -EINVAL;
> +
> if (qcom_tzmem_dev)
> return -EBUSY;
>
> --
> 2.34.1
>
On Mon, 7 Oct 2024 at 03:18, Bjorn Andersson <andersson@kernel.org> 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. > > > > Unless there's good reason for the opposite, I rather see that we define > the API to only accept valid pointers. Then if a client passes a NULL we > get a oops with a nice callstack, which is easy to debug. > > The alternative is that we return -EINVAL, which not unlikely is > propagated to some application which may or may not result in a bug > report from a user - without any tangible information about where things > went wrong. Agreed, I don't think this is a good pattern in a kernel API (as opposed to user-space interfaces where we validate everything). We expect a certain level of sanity from in-kernel users. Bart
On 10/7/2024 7:53 PM, Bartosz Golaszewski wrote: > On Mon, 7 Oct 2024 at 03:18, Bjorn Andersson <andersson@kernel.org> 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. >>> >> >> Unless there's good reason for the opposite, I rather see that we define >> the API to only accept valid pointers. Then if a client passes a NULL we >> get a oops with a nice callstack, which is easy to debug>> >> The alternative is that we return -EINVAL, which not unlikely is >> propagated to some application which may or may not result in a bug >> report from a user - without any tangible information about where things >> went wrong. Discussing with Dmitry as well on other thread over same point. Not all checks are needed but I believe some sanity is still needed to avoid crashes. > > Agreed, I don't think this is a good pattern in a kernel API (as > opposed to user-space interfaces where we validate everything). We > expect a certain level of sanity from in-kernel users. > > Bart -- Regards Kuldeep
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.
>
> Signed-off-by: Kuldeep Singh <quic_kuldsing@quicinc.com>
> ---
> drivers/firmware/qcom/qcom_tzmem.c | 17 ++++++++++++++++-
> 1 file changed, 16 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/qcom/qcom_tzmem.c b/drivers/firmware/qcom/qcom_tzmem.c
> index 92b365178235..2f2e1f2fa9fc 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 || !config->policy)
config can not be NULL
Ack for config->policy check.
> + return ERR_PTR(-EINVAL);
> +
> switch (config->policy) {
> case QCOM_TZMEM_POLICY_STATIC:
> if (!config->initial_size)
> @@ -316,6 +319,9 @@ devm_qcom_tzmem_pool_new(struct device *dev,
> struct qcom_tzmem_pool *pool;
> int ret;
>
> + if (!dev || !config)
> + return ERR_PTR(-EINVAL);
dev can not be NULL
config can not be NULL
> +
> pool = qcom_tzmem_pool_new(config);
> if (IS_ERR(pool))
> return pool;
> @@ -366,7 +372,7 @@ void *qcom_tzmem_alloc(struct qcom_tzmem_pool *pool, size_t size, gfp_t gfp)
> unsigned long vaddr;
> int ret;
>
> - if (!size)
> + if (!pool || !size)
Is it really possible to pass NULL as pool? Which code path leads to
this event?
> return NULL;
>
> size = PAGE_ALIGN(size);
> @@ -412,6 +418,9 @@ void qcom_tzmem_free(void *vaddr)
> {
> struct qcom_tzmem_chunk *chunk;
>
> + if (!vaddr)
> + return;
Ack, simplifies error handling and matches existing kfree-like functions.
> +
> scoped_guard(spinlock_irqsave, &qcom_tzmem_chunks_lock)
> chunk = radix_tree_delete_item(&qcom_tzmem_chunks,
> (unsigned long)vaddr, NULL);
> @@ -446,6 +455,9 @@ phys_addr_t qcom_tzmem_to_phys(void *vaddr)
> void __rcu **slot;
> phys_addr_t ret;
>
> + if (!vaddr)
Is it possible?
> + return 0;
> +
> guard(spinlock_irqsave)(&qcom_tzmem_chunks_lock);
>
> radix_tree_for_each_slot(slot, &qcom_tzmem_chunks, &iter, 0) {
> @@ -466,6 +478,9 @@ EXPORT_SYMBOL_GPL(qcom_tzmem_to_phys);
>
> int qcom_tzmem_enable(struct device *dev)
> {
> + if (!dev)
> + return -EINVAL;
Definitely not possible.
> +
> if (qcom_tzmem_dev)
> return -EBUSY;
>
> --
> 2.34.1
>
--
With best wishes
Dmitry
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.
>>
>> Signed-off-by: Kuldeep Singh <quic_kuldsing@quicinc.com>
>> ---
>> drivers/firmware/qcom/qcom_tzmem.c | 17 ++++++++++++++++-
>> 1 file changed, 16 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/firmware/qcom/qcom_tzmem.c b/drivers/firmware/qcom/qcom_tzmem.c
>> index 92b365178235..2f2e1f2fa9fc 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 || !config->policy)
>
> config can not be NULL
> Ack for config->policy check.
Considering a scenario where user doesn't fill config struct details and call devm_qcom_tzmem_pool_new.
config will be null in that case.
>
>> + return ERR_PTR(-EINVAL);
>> +
>> switch (config->policy) {
>> case QCOM_TZMEM_POLICY_STATIC:
>> if (!config->initial_size)
>> @@ -316,6 +319,9 @@ devm_qcom_tzmem_pool_new(struct device *dev,
>> struct qcom_tzmem_pool *pool;
>> int ret;
>>
>> + if (!dev || !config)
>> + return ERR_PTR(-EINVAL);
>
> dev can not be NULL
> config can not be NULL
dev may not be always __scm->dev.
For ex: qcom_qseecom_uefisecapp.c pass it's own dev.
If new calling driver pass dev as null, will lead to NPD.
>
>> +
>> pool = qcom_tzmem_pool_new(config);
>> if (IS_ERR(pool))
>> return pool;
>> @@ -366,7 +372,7 @@ void *qcom_tzmem_alloc(struct qcom_tzmem_pool *pool, size_t size, gfp_t gfp)
>> unsigned long vaddr;
>> int ret;
>>
>> - if (!size)
>> + if (!pool || !size)
>
> Is it really possible to pass NULL as pool? Which code path leads to
> this event?
qcom_tzmem_alloc/free need to be used once pool is already created with devm_qcom_tzmem_pool_new API.
If pool isn't created, then calling qcom_tzmem_alloc/free will be erroneus.
>
>> return NULL;
>>
>> size = PAGE_ALIGN(size);
>> @@ -412,6 +418,9 @@ void qcom_tzmem_free(void *vaddr)
>> {
>> struct qcom_tzmem_chunk *chunk;
>>
>> + if (!vaddr)
>> + return;
>
> Ack, simplifies error handling and matches existing kfree-like functions.
>
>> +
>> scoped_guard(spinlock_irqsave, &qcom_tzmem_chunks_lock)
>> chunk = radix_tree_delete_item(&qcom_tzmem_chunks,
>> (unsigned long)vaddr, NULL);
>> @@ -446,6 +455,9 @@ phys_addr_t qcom_tzmem_to_phys(void *vaddr)
>> void __rcu **slot;
>> phys_addr_t ret;
>>
>> + if (!vaddr)
>
> Is it possible?
Yes, A scenario where qcom_tzmem_alloc fails resulting vaddr as 0 followed by no null check.
Now, immediately passing vaddr to qcom_tzmem_to_phys will again cause NPD.
>
>> + return 0;
>> +
>> guard(spinlock_irqsave)(&qcom_tzmem_chunks_lock);
>>
>> radix_tree_for_each_slot(slot, &qcom_tzmem_chunks, &iter, 0) {
>> @@ -466,6 +478,9 @@ EXPORT_SYMBOL_GPL(qcom_tzmem_to_phys);
>>
>> int qcom_tzmem_enable(struct device *dev)
>> {
>> + if (!dev)
>> + return -EINVAL;
>
> Definitely not possible.
Ack, by this time __scm->dev will be initialised in qcom_scm driver and cannot be null.
If some other caller even try and qcom_tzmem_dev is already set hence, return -EBUSY.
Will drop the check.
--
Regards
Kuldeep
On Mon, 7 Oct 2024 at 21:17, Kuldeep Singh <quic_kuldsing@quicinc.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.
>
> >>
> >> Signed-off-by: Kuldeep Singh <quic_kuldsing@quicinc.com>
> >> ---
> >> drivers/firmware/qcom/qcom_tzmem.c | 17 ++++++++++++++++-
> >> 1 file changed, 16 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/firmware/qcom/qcom_tzmem.c b/drivers/firmware/qcom/qcom_tzmem.c
> >> index 92b365178235..2f2e1f2fa9fc 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 || !config->policy)
> >
> > config can not be NULL
> > Ack for config->policy check.
>
> Considering a scenario where user doesn't fill config struct details and call devm_qcom_tzmem_pool_new.
> config will be null in that case.
Likewise other driver (not the user!) can pass NULL to other
functions, crashing the kernel. This is not a way to go.
>
> >
> >> + return ERR_PTR(-EINVAL);
> >> +
> >> switch (config->policy) {
> >> case QCOM_TZMEM_POLICY_STATIC:
> >> if (!config->initial_size)
> >> @@ -316,6 +319,9 @@ devm_qcom_tzmem_pool_new(struct device *dev,
> >> struct qcom_tzmem_pool *pool;
> >> int ret;
> >>
> >> + if (!dev || !config)
> >> + return ERR_PTR(-EINVAL);
> >
> > dev can not be NULL
> > config can not be NULL
>
> dev may not be always __scm->dev.
> For ex: qcom_qseecom_uefisecapp.c pass it's own dev.
> If new calling driver pass dev as null, will lead to NPD.
Just don't. I don't see other devm_ functions checking the dev param,
because generally we believe in the sanity of driver authors.
>
> >
> >> +
> >> pool = qcom_tzmem_pool_new(config);
> >> if (IS_ERR(pool))
> >> return pool;
> >> @@ -366,7 +372,7 @@ void *qcom_tzmem_alloc(struct qcom_tzmem_pool *pool, size_t size, gfp_t gfp)
> >> unsigned long vaddr;
> >> int ret;
> >>
> >> - if (!size)
> >> + if (!pool || !size)
> >
> > Is it really possible to pass NULL as pool? Which code path leads to
> > this event?
>
> qcom_tzmem_alloc/free need to be used once pool is already created with devm_qcom_tzmem_pool_new API.
> If pool isn't created, then calling qcom_tzmem_alloc/free will be erroneus.
If your driver doesn't check pool_new() result, then it's broken.
>
> >
> >> return NULL;
> >>
> >> size = PAGE_ALIGN(size);
> >> @@ -412,6 +418,9 @@ void qcom_tzmem_free(void *vaddr)
> >> {
> >> struct qcom_tzmem_chunk *chunk;
> >>
> >> + if (!vaddr)
> >> + return;
> >
> > Ack, simplifies error handling and matches existing kfree-like functions.
> >
> >> +
> >> scoped_guard(spinlock_irqsave, &qcom_tzmem_chunks_lock)
> >> chunk = radix_tree_delete_item(&qcom_tzmem_chunks,
> >> (unsigned long)vaddr, NULL);
> >> @@ -446,6 +455,9 @@ phys_addr_t qcom_tzmem_to_phys(void *vaddr)
> >> void __rcu **slot;
> >> phys_addr_t ret;
> >>
> >> + if (!vaddr)
> >
> > Is it possible?
>
> Yes, A scenario where qcom_tzmem_alloc fails resulting vaddr as 0 followed by no null check.
> Now, immediately passing vaddr to qcom_tzmem_to_phys will again cause NPD.
Likewise. If you driver doesn't check qcom_tzmem_alloc(), it's broken
and must be fixed. Null pointer exception will help fix the driver.
Adding such band-aids will hide the issue.
>
> >
> >> + return 0;
> >> +
> >> guard(spinlock_irqsave)(&qcom_tzmem_chunks_lock);
> >>
> >> radix_tree_for_each_slot(slot, &qcom_tzmem_chunks, &iter, 0) {
> >> @@ -466,6 +478,9 @@ EXPORT_SYMBOL_GPL(qcom_tzmem_to_phys);
> >>
> >> int qcom_tzmem_enable(struct device *dev)
> >> {
> >> + if (!dev)
> >> + return -EINVAL;
> >
> > Definitely not possible.
>
> Ack, by this time __scm->dev will be initialised in qcom_scm driver and cannot be null.
> If some other caller even try and qcom_tzmem_dev is already set hence, return -EBUSY.
> Will drop the check.
--
With best wishes
Dmitry
On 10/8/2024 1:43 AM, Dmitry Baryshkov wrote: > On Mon, 7 Oct 2024 at 21:17, Kuldeep Singh <quic_kuldsing@quicinc.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. -- Regards Kuldeep
On Mon, 7 Oct 2024 at 22:44, Kuldeep Singh <quic_kuldsing@quicinc.com> wrote: > > > > On 10/8/2024 1:43 AM, Dmitry Baryshkov wrote: > > On Mon, 7 Oct 2024 at 21:17, Kuldeep Singh <quic_kuldsing@quicinc.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
© 2016 - 2024 Red Hat, Inc.