staging: gasket: interrupt: Clean interrupt_data after free

[PATCH 5.10] staging: gasket: interrupt: Clean interrupt_data after free

Posted by Esina Ekaterina 2 years, 8 months ago

Add interrupt_data = NULL after kfree(interrupt_data) in
gasket_interrupt_init. It is needed to avoid double free
in gasket_interrupt_cleanup, there is a check for NULL
before kfree(interrupt_data).

Found by Astra Linux on behalf of Linux Verification Center
(linuxtesting.org) with SVACE.

Fixes: 9a69f5087ccc ("drivers/staging: Gasket driver framework + Apex driver")
Signed-off-by: Esina Ekaterina <eesina@astralinux.ru>
---
 drivers/staging/gasket/gasket_interrupt.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/staging/gasket/gasket_interrupt.c b/drivers/staging/gasket/gasket_interrupt.c
index 864342acfd86..48b664b9134a 100644
--- a/drivers/staging/gasket/gasket_interrupt.c
+++ b/drivers/staging/gasket/gasket_interrupt.c
@@ -337,6 +337,7 @@ int gasket_interrupt_init(struct gasket_dev *gasket_dev)
 			sizeof(*interrupt_data->eventfd_ctxs), GFP_KERNEL);
 	if (!interrupt_data->eventfd_ctxs) {
 		kfree(interrupt_data);
+		interrupt_data = NULL;
 		return -ENOMEM;
 	}
 
@@ -346,6 +347,7 @@ int gasket_interrupt_init(struct gasket_dev *gasket_dev)
 	if (!interrupt_data->interrupt_counts) {
 		kfree(interrupt_data->eventfd_ctxs);
 		kfree(interrupt_data);
+		interrupt_data = NULL;
 		return -ENOMEM;
 	}
 
-- 
2.40.1

Re: [PATCH 5.10] staging: gasket: interrupt: Clean interrupt_data after free

Posted by Greg Kroah-Hartman 2 years, 8 months ago

On Mon, May 15, 2023 at 03:29:50PM +0300, Esina Ekaterina wrote:
> Add interrupt_data = NULL after kfree(interrupt_data) in
> gasket_interrupt_init. It is needed to avoid double free
> in gasket_interrupt_cleanup, there is a check for NULL
> before kfree(interrupt_data).
> 
> Found by Astra Linux on behalf of Linux Verification Center
> (linuxtesting.org) with SVACE.
> 
> Fixes: 9a69f5087ccc ("drivers/staging: Gasket driver framework + Apex driver")
> Signed-off-by: Esina Ekaterina <eesina@astralinux.ru>
> ---
>  drivers/staging/gasket/gasket_interrupt.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/staging/gasket/gasket_interrupt.c b/drivers/staging/gasket/gasket_interrupt.c
> index 864342acfd86..48b664b9134a 100644
> --- a/drivers/staging/gasket/gasket_interrupt.c
> +++ b/drivers/staging/gasket/gasket_interrupt.c
> @@ -337,6 +337,7 @@ int gasket_interrupt_init(struct gasket_dev *gasket_dev)
>  			sizeof(*interrupt_data->eventfd_ctxs), GFP_KERNEL);
>  	if (!interrupt_data->eventfd_ctxs) {
>  		kfree(interrupt_data);
> +		interrupt_data = NULL;
>  		return -ENOMEM;
>  	}
>  
> @@ -346,6 +347,7 @@ int gasket_interrupt_init(struct gasket_dev *gasket_dev)
>  	if (!interrupt_data->interrupt_counts) {
>  		kfree(interrupt_data->eventfd_ctxs);
>  		kfree(interrupt_data);
> +		interrupt_data = NULL;
>  		return -ENOMEM;
>  	}
>  
> -- 
> 2.40.1

Also, your tool is wrong, this is not a correct fix at all.

How did you test it?  Why is your tool spitting out incorrect code?  Why
are you not verifying it's output before asking others to do so?

Please don't do this anymore, it's wasteful on our side, right?  Please
take the time to review existing changes for problems, that would be
more useful overall to everyone.

thanks,

greg k-h

Re: [PATCH 5.10] staging: gasket: interrupt: Clean interrupt_data after free

Posted by Ekaterina Esina 2 years, 8 months ago


On 15.05.2023 16:02, Greg Kroah-Hartman wrote:
> On Mon, May 15, 2023 at 03:29:50PM +0300, Esina Ekaterina wrote:
>> Add interrupt_data = NULL after kfree(interrupt_data) in
>> gasket_interrupt_init. It is needed to avoid double free
>> in gasket_interrupt_cleanup, there is a check for NULL
>> before kfree(interrupt_data).
>>
>> Found by Astra Linux on behalf of Linux Verification Center
>> (linuxtesting.org) with SVACE.
>>
>> Fixes: 9a69f5087ccc ("drivers/staging: Gasket driver framework + Apex driver")
>> Signed-off-by: Esina Ekaterina <eesina@astralinux.ru>
>> ---
>>   drivers/staging/gasket/gasket_interrupt.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/staging/gasket/gasket_interrupt.c b/drivers/staging/gasket/gasket_interrupt.c
>> index 864342acfd86..48b664b9134a 100644
>> --- a/drivers/staging/gasket/gasket_interrupt.c
>> +++ b/drivers/staging/gasket/gasket_interrupt.c
>> @@ -337,6 +337,7 @@ int gasket_interrupt_init(struct gasket_dev *gasket_dev)
>>   			sizeof(*interrupt_data->eventfd_ctxs), GFP_KERNEL);
>>   	if (!interrupt_data->eventfd_ctxs) {
>>   		kfree(interrupt_data);
>> +		interrupt_data = NULL;
>>   		return -ENOMEM;
>>   	}
>>   
>> @@ -346,6 +347,7 @@ int gasket_interrupt_init(struct gasket_dev *gasket_dev)
>>   	if (!interrupt_data->interrupt_counts) {
>>   		kfree(interrupt_data->eventfd_ctxs);
>>   		kfree(interrupt_data);
>> +		interrupt_data = NULL;
>>   		return -ENOMEM;
>>   	}
>>   
>> -- 
>> 2.40.1
> 
> Also, your tool is wrong, this is not a correct fix at all.
> 
> How did you test it?  Why is your tool spitting out incorrect code?  Why
> are you not verifying it's output before asking others to do so?
> 
> Please don't do this anymore, it's wasteful on our side, right?  Please
> take the time to review existing changes for problems, that would be
> more useful overall to everyone.
> 
> thanks,
> 
> greg k-h

I apologize, there is a mistake. It should've been 
gasket_dev->interrupt_data = NULL istead of interrupt_data = NULL

Best regards,
Ekaterina Esina

Re: [PATCH 5.10] staging: gasket: interrupt: Clean interrupt_data after free

Posted by Greg Kroah-Hartman 2 years, 8 months ago

On Tue, May 16, 2023 at 01:44:06PM +0300, Ekaterina Esina wrote:
> 
> 
> On 15.05.2023 16:02, Greg Kroah-Hartman wrote:
> > On Mon, May 15, 2023 at 03:29:50PM +0300, Esina Ekaterina wrote:
> > > Add interrupt_data = NULL after kfree(interrupt_data) in
> > > gasket_interrupt_init. It is needed to avoid double free
> > > in gasket_interrupt_cleanup, there is a check for NULL
> > > before kfree(interrupt_data).
> > > 
> > > Found by Astra Linux on behalf of Linux Verification Center
> > > (linuxtesting.org) with SVACE.
> > > 
> > > Fixes: 9a69f5087ccc ("drivers/staging: Gasket driver framework + Apex driver")
> > > Signed-off-by: Esina Ekaterina <eesina@astralinux.ru>
> > > ---
> > >   drivers/staging/gasket/gasket_interrupt.c | 2 ++
> > >   1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/staging/gasket/gasket_interrupt.c b/drivers/staging/gasket/gasket_interrupt.c
> > > index 864342acfd86..48b664b9134a 100644
> > > --- a/drivers/staging/gasket/gasket_interrupt.c
> > > +++ b/drivers/staging/gasket/gasket_interrupt.c
> > > @@ -337,6 +337,7 @@ int gasket_interrupt_init(struct gasket_dev *gasket_dev)
> > >   			sizeof(*interrupt_data->eventfd_ctxs), GFP_KERNEL);
> > >   	if (!interrupt_data->eventfd_ctxs) {
> > >   		kfree(interrupt_data);
> > > +		interrupt_data = NULL;
> > >   		return -ENOMEM;
> > >   	}
> > > @@ -346,6 +347,7 @@ int gasket_interrupt_init(struct gasket_dev *gasket_dev)
> > >   	if (!interrupt_data->interrupt_counts) {
> > >   		kfree(interrupt_data->eventfd_ctxs);
> > >   		kfree(interrupt_data);
> > > +		interrupt_data = NULL;
> > >   		return -ENOMEM;
> > >   	}
> > > -- 
> > > 2.40.1
> > 
> > Also, your tool is wrong, this is not a correct fix at all.
> > 
> > How did you test it?  Why is your tool spitting out incorrect code?  Why
> > are you not verifying it's output before asking others to do so?
> > 
> > Please don't do this anymore, it's wasteful on our side, right?  Please
> > take the time to review existing changes for problems, that would be
> > more useful overall to everyone.
> > 
> > thanks,
> > 
> > greg k-h
> 
> I apologize, there is a mistake. It should've been
> gasket_dev->interrupt_data = NULL istead of interrupt_data = NULL

Which implies that your tools are broken, right?  Please do better
testing before sending anymore patches.

greg k-h

Re: [PATCH 5.10] staging: gasket: interrupt: Clean interrupt_data after free

Posted by Greg KH 2 years, 8 months ago

On Mon, May 15, 2023 at 03:29:50PM +0300, Esina Ekaterina wrote:
> Add interrupt_data = NULL after kfree(interrupt_data) in
> gasket_interrupt_init. It is needed to avoid double free
> in gasket_interrupt_cleanup, there is a check for NULL
> before kfree(interrupt_data).
> 
> Found by Astra Linux on behalf of Linux Verification Center
> (linuxtesting.org) with SVACE.
> 
> Fixes: 9a69f5087ccc ("drivers/staging: Gasket driver framework + Apex driver")
> Signed-off-by: Esina Ekaterina <eesina@astralinux.ru>
> ---
>  drivers/staging/gasket/gasket_interrupt.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/staging/gasket/gasket_interrupt.c b/drivers/staging/gasket/gasket_interrupt.c
> index 864342acfd86..48b664b9134a 100644
> --- a/drivers/staging/gasket/gasket_interrupt.c
> +++ b/drivers/staging/gasket/gasket_interrupt.c
> @@ -337,6 +337,7 @@ int gasket_interrupt_init(struct gasket_dev *gasket_dev)
>  			sizeof(*interrupt_data->eventfd_ctxs), GFP_KERNEL);
>  	if (!interrupt_data->eventfd_ctxs) {
>  		kfree(interrupt_data);
> +		interrupt_data = NULL;
>  		return -ENOMEM;
>  	}
>  
> @@ -346,6 +347,7 @@ int gasket_interrupt_init(struct gasket_dev *gasket_dev)
>  	if (!interrupt_data->interrupt_counts) {
>  		kfree(interrupt_data->eventfd_ctxs);
>  		kfree(interrupt_data);
> +		interrupt_data = NULL;
>  		return -ENOMEM;
>  	}
>  
> -- 
> 2.40.1
> 

<formletter>

This is not the correct way to submit patches for inclusion in the
stable kernel tree.  Please read:
    https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.

</formletter>