[PATCH -next] drm/amdgpu: double free error and freeing uninitialized null pointer
Somalapuram, Amaranath
asomalap at amd.com
Fri Jul 15 08:18:56 UTC 2022
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/amd-gfx/attachments/20220715/d9b758de/attachment-0001.htm>
More information about the amd-gfx
mailing list