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: <2eb53ea6-848a-48bb-8c31-83a118bc5a73@quicinc.com>
Date: Tue, 8 Oct 2024 00:47:02 +0530
From: Kuldeep Singh <quic_kuldsing@...cinc.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
CC: Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio
	<konradybcio@...nel.org>,
        Bartosz Golaszewski
	<bartosz.golaszewski@...aro.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 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@...cinc.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