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

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

[Sebin Sebastian]

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)

Signed-off-by: Sebin Sebastian <mailmesebin00@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;
 	}
 	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;
 }
-- 
2.34.1

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

[André Almeida]

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?

> 
> Signed-off-by: Sebin Sebastian <mailmesebin00@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?

>  	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;
>  }

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

[Sebin Sebastian]

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.

> > 
> > Signed-off-by: Sebin Sebastian <mailmesebin00@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.

> >  	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;
> >  }

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

[André Almeida]

À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.

>>>
>>> Signed-off-by: Sebin Sebastian <mailmesebin00@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;
>>>  }

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

[Sebin Sebastian]

On Fri, Jul 15, 2022 at 01:48:56PM +0530, Somalapuram, Amaranath wrote:
> 
> 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.
>
But if new is not initialized to null, won't it trigger a free on an
uninitialized pointer in the first if block inside the do while loop?

> 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
>
This seem's like the best way to avoid the double free. Thanks for the
suggestions.

> > > > > Signed-off-by: Sebin Sebastian<mailmesebin00@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;
> > > > >   }