[PATCH] scsi: aacraid: Fix memory leak in open_getadapter_fib function

[PATCH] scsi: aacraid: Fix memory leak in open_getadapter_fib function

[Riyan Dhiman]

In the open_getadapter_fib() function, memory allocated for the fibctx structure
was not freed when copy_to_user() failed. This can lead to memory leaks as the 
allocated memory remains unreferenced and cannot be reclaimed.

This patch ensures that the allocated memory for fibctx is properly
freed if copy_to_user() fails, thereby preventing potential memory leaks.

Changes:
- Added kfree(fibctx); to release memory when copy_to_user() fails.

Signed-off-by: Riyan Dhiman <riyandhiman14@gmail.com>
---
 drivers/scsi/aacraid/commctrl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/scsi/aacraid/commctrl.c b/drivers/scsi/aacraid/commctrl.c
index e7cc927ed952..80838c84b444 100644
--- a/drivers/scsi/aacraid/commctrl.c
+++ b/drivers/scsi/aacraid/commctrl.c
@@ -220,6 +220,7 @@ static int open_getadapter_fib(struct aac_dev * dev, void __user *arg)
 		if (copy_to_user(arg, &fibctx->unique,
 						sizeof(fibctx->unique))) {
 			status = -EFAULT;
+			kfree(fibctx);
 		} else {
 			status = 0;
 		}
-- 
2.46.0

Re: [PATCH] scsi: aacraid: Fix memory leak in open_getadapter_fib function

[Bart Van Assche]

On 9/3/24 11:54 AM, Riyan Dhiman wrote:
> In the open_getadapter_fib() function, memory allocated for the fibctx structure
> was not freed when copy_to_user() failed. This can lead to memory leaks as the
> allocated memory remains unreferenced and cannot be reclaimed.
> 
> This patch ensures that the allocated memory for fibctx is properly
> freed if copy_to_user() fails, thereby preventing potential memory leaks.

What made you analyze the code modified by this patch?

How has this patch been tested?

> Changes:
> - Added kfree(fibctx); to release memory when copy_to_user() fails.

Changes compared to what? I don't see a version number in the email
subject.

> @@ -220,6 +220,7 @@ static int open_getadapter_fib(struct aac_dev * dev, void __user *arg)
>   		if (copy_to_user(arg, &fibctx->unique,
>   						sizeof(fibctx->unique))) {
>   			status = -EFAULT;
> +			kfree(fibctx);
>   		} else {
>   			status = 0;
>   		}

Just above the copy_to_user() call there is the following statement:

	list_add_tail(&fibctx->next, &dev->fib_list);

Does that mean that the above kfree() will cause list corruption?

Bart.

[PATCH] scsi: aacraid: Fix memory leak in open_getadapter_fib function

[Riyan Dhiman]

>> This patch ensures that the allocated memory for fibctx is properly
>> freed if copy_to_user() fails, thereby preventing potential memory leaks.
>
> What made you analyze the code modified by this patch?

If copy_to_user() fails and returns an -EFAULT error, the memory allocated 
for fibctx was not being freed, which could lead to memory leaks.

> How has this patch been tested?

I have compiled tested the patch. I realize I should have specified "compile tested"
 in the commit message and written "preventing potential memory leaks" instead.  

>> Changes:
>> - Added kfree(fibctx); to release memory when copy_to_user() fails.
>
> Changes compared to what? I don't see a version number in the email
> subject.

I included the "Changes" section to indicate what was modified in the patch. I will 
remove this section in the updated message, as there is no version number to reference.

> Just above the copy_to_user() call there is the following statement:
> 
> 	list_add_tail(&fibctx->next, &dev->fib_list);
>
> Does that mean that the above kfree() will cause list corruption?

Yes, you are correct. I overlooked that fibctx is part of a list, and freeing the 
memory without removing the list entry would corrupt the list. 
The list entry should be deleted before freeing the memory if copy_to_user() fails.

Regards,
Riyan Dhiman

Re: [PATCH] scsi: aacraid: Fix memory leak in open_getadapter_fib function

[Bart Van Assche]

On 9/3/24 1:30 PM, Riyan Dhiman wrote:
>> Just above the copy_to_user() call there is the following statement:
>>
>> 	list_add_tail(&fibctx->next, &dev->fib_list);
>>
>> Does that mean that the above kfree() will cause list corruption?
> 
> Yes, you are correct. I overlooked that fibctx is part of a list, and freeing the
> memory without removing the list entry would corrupt the list.
> The list entry should be deleted before freeing the memory if copy_to_user() fails.

Are you sure that this is what the code should do?

Thanks,

Bart.

[PATCH] scsi: aacraid: Fix memory leak in open_getadapter_fib function

[Riyan Dhiman]

>>> Just above the copy_to_user() call there is the following statement:
>>>
>>> 	list_add_tail(&fibctx->next, &dev->fib_list);
>>>
>>> Does that mean that the above kfree() will cause list corruption?
>> 
>> Yes, you are correct. I overlooked that fibctx is part of a list, and freeing the
>> memory without removing the list entry would corrupt the list.
>> The list entry should be deleted before freeing the memory if copy_to_user() fails.
>
> Are you sure that this is what the code should do?

Yes, removing the list entry before freeing the memory is necessary to maintain list 
integrity and prevent corruption. If there are any other methods, additional checks, 
or potential issues with this approach that I should consider, please let me know, 
and I'll make the necessary adjustments promptly.

Regards,
Riyan Dhiman

[PATCH] scsi: aacraid: Fix memory leak in open_getadapter_fib function

[Riyan Dhiman]

>>> Just above the copy_to_user() call there is the following statement:
>>>
>>> 	list_add_tail(&fibctx->next, &dev->fib_list);
>>>
>>> Does that mean that the above kfree() will cause list corruption?
>> 
>> Yes, you are correct. I overlooked that fibctx is part of a list, and freeing the
>> memory without removing the list entry would corrupt the list.
>> The list entry should be deleted before freeing the memory if copy_to_user() fails.
>
> Are you sure that this is what the code should do?

If copy_to_user function fails that means data was not copied to args successfully, which can leads to 
issue as args might remain unchanged or in an uninteded state. Since we are returning an -EFAULT error, 
we should free fibctx and remove the list entry in the case of an error. If there are any other methods, 
additional checks, or potential issues with this approach that I should consider, 
please let me know, and I'll make the necessary adjustments promptly.

Regards,
Riyan Dhiman

Re: [PATCH] scsi: aacraid: Fix memory leak in open_getadapter_fib function

[Bart Van Assche]

On 9/3/24 9:58 PM, Riyan Dhiman wrote:
> If there are any other methods, additional checks, or potential
> issues with this approach that I should consider, please let me know,
> and I'll make the necessary adjustments promptly.
I'm not familiar with the aacraid driver. I will let someone who is
familiar with this driver answer your question.

Thanks,

Bart.