| Message ID | 20240903185410.21144-1-riyandhiman14@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | scsi: aacraid: Fix memory leak in open_getadapter_fib function | expand |
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.
>> 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
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.
>>> 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
>>> 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
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.
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; }
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(+)