[PATCH -next] drm/amdgpu: double free error and freeing uninitialized null pointer

[Somalapuram, Amaranath]

On 7/14/2022 9:13 PM, André Almeida wrote:
> Às 12:06 de 14/07/22, Sebin Sebastian escreveu:
>> On Tue, Jul 12, 2022 at 12:14:27PM -0300, André Almeida wrote:
>>> Hi Sebin,
>>>
>>> Às 10:29 de 10/07/22, Sebin Sebastian escreveu:
>>>> Fix two coverity warning's double free and and an uninitialized pointer
>>>> read. Both tmp and new are pointing at same address and both are freed
>>>> which leads to double free. Freeing tmp in the condition after new is
>>>> assigned with new address fixes the double free issue. new is not
>>>> initialized to null which also leads to a free on an uninitialized
>>>> pointer.
>>>> Coverity issue: 1518665 (uninitialized pointer read)
>>>> 		1518679 (double free)
>>> What are those numbers?
>>>
>> These numbers are the issue ID's for the errors that are being reported
>> by the coverity static analyzer tool.
>>
> I see, but I don't know which tool was used, so those seem like random
> number to me. I would just remove this part of your commit message, but
> if you want to keep it, you need to at least mention what's the tool.

new variable is not needed to initialize.

The only condition double free happens is:

tmp = new;
                 if (sscanf(reg_offset, "%X %n", &tmp[i], &ret) != 1) {
                         ret = -EINVAL;
                         goto error_free; *//    if it hits this*
                 }/
/

and can be avoided like:

  error_free:
-       kfree(tmp);
+       if (tmp != new)
+               kfree(tmp);
         kfree(new);
         return ret;
  }


Regards,

S.Amarnath

>>>> Signed-off-by: Sebin Sebastian<mailmesebin00 at gmail.com>
>>>> ---
>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c | 8 +++++---
>>>>   1 file changed, 5 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>> index f3b3c688e4e7..d82fe0e1b06b 100644
>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_debugfs.c
>>>> @@ -1660,7 +1660,7 @@ static ssize_t amdgpu_reset_dump_register_list_write(struct file *f,
>>>>   {
>>>>   	struct amdgpu_device *adev = (struct amdgpu_device *)file_inode(f)->i_private;
>>>>   	char reg_offset[11];
>>>> -	uint32_t *new, *tmp = NULL;
>>>> +	uint32_t *new = NULL, *tmp = NULL;
>>>>   	int ret, i = 0, len = 0;
>>>>   
>>>>   	do {
>>>> @@ -1692,17 +1692,19 @@ static ssize_t amdgpu_reset_dump_register_list_write(struct file *f,
>>>>   		goto error_free;
>>>>   	}
>>> If the `if (!new) {` above this line is true, will be tmp freed?
>>>
>> Yes, It doesn't seem to free tmp here. Should I free tmp immediately
>> after the do while loop and remove `kfree(tmp)` from the `if (ret)`
>> block? Thanks for pointing out the errors.
> If you free immediately after the while loop, then you would risk a use
> after free here:
>
> 	swap(adev->reset_dump_reg_list, tmp);
>
> So this isn't the solution either.
>
>>>>   	ret = down_write_killable(&adev->reset_domain->sem);
>>>> -	if (ret)
>>>> +	if (ret) {
>>>> +		kfree(tmp);
>>>>   		goto error_free;
>>>> +	}
>>>>   
>>>>   	swap(adev->reset_dump_reg_list, tmp);
>>>>   	swap(adev->reset_dump_reg_value, new);
>>>>   	adev->num_regs = i;
>>>>   	up_write(&adev->reset_domain->sem);
>>>> +	kfree(tmp);
>>>>   	ret = size;
>>>>   
>>>>   error_free:
>>>> -	kfree(tmp);
>>>>   	kfree(new);
>>>>   	return ret;
>>>>   }
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20220715/d9b758de/attachment-0001.htm>