[167/190] Revert "gdrom: fix a memory leak bug"
diff mbox series

Message ID 20210421130105.1226686-168-gregkh@linuxfoundation.org
State New, archived
Headers show
Series
  • Revertion of all of the umn.edu commits
Related show

Commit Message

Greg Kroah-Hartman April 21, 2021, 1 p.m. UTC
This reverts commit 093c48213ee37c3c3ff1cf5ac1aa2a9d8bc66017.

Commits from @umn.edu addresses have been found to be submitted in "bad
faith" to try to test the kernel community's ability to review "known
malicious" changes.  The result of these submissions can be found in a
paper published at the 42nd IEEE Symposium on Security and Privacy
entitled, "Open Source Insecurity: Stealthily Introducing
Vulnerabilities via Hypocrite Commits" written by Qiushi Wu (University
of Minnesota) and Kangjie Lu (University of Minnesota).

Because of this, all submissions from this group must be reverted from
the kernel tree and will need to be re-reviewed again to determine if
they actually are a valid fix.  Until that work is complete, remove this
change to ensure that no problems are being introduced into the
codebase.

Cc: Wenwen Wang <wang6495@umn.edu>
Cc: Jens Axboe <axboe@kernel.dk>
Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
---
 drivers/cdrom/gdrom.c | 1 -
 1 file changed, 1 deletion(-)

Comments

Peter Rosin April 22, 2021, 9:29 p.m. UTC | #1
> This reverts commit 093c48213ee37c3c3ff1cf5ac1aa2a9d8bc66017.

The reverted patch looks fishy.

gc.cd_info is kzalloc:ed on probe. In case probe fails after this allocation, the
memory is kfree:d but the variable is NOT zeroed out.

AFAICT, the above leads to a double-free on exit by the added line.

I believe gd.cd_info should be kfree:d on remove instead.

However, might not gc.toc also be kfree:d twice for similar reasons?

I could easily be mistaken.

Cheers,
Peter
Jens Axboe April 23, 2021, 2:20 p.m. UTC | #2
On 4/22/21 3:29 PM, Peter Rosin wrote:
>> This reverts commit 093c48213ee37c3c3ff1cf5ac1aa2a9d8bc66017.
> 
> The reverted patch looks fishy.
> 
> gc.cd_info is kzalloc:ed on probe. In case probe fails after this allocation, the
> memory is kfree:d but the variable is NOT zeroed out.
> 
> AFAICT, the above leads to a double-free on exit by the added line.
> 
> I believe gd.cd_info should be kfree:d on remove instead.
> 
> However, might not gc.toc also be kfree:d twice for similar reasons?
> 
> I could easily be mistaken.

From taking a quick look the other day, that's my conclusion too. I
don't think the patch is correct, but I don't think the surrounding code
is correct right now either.
Greg Kroah-Hartman April 27, 2021, 1:01 p.m. UTC | #3
On Fri, Apr 23, 2021 at 08:20:30AM -0600, Jens Axboe wrote:
> On 4/22/21 3:29 PM, Peter Rosin wrote:
> >> This reverts commit 093c48213ee37c3c3ff1cf5ac1aa2a9d8bc66017.
> > 
> > The reverted patch looks fishy.
> > 
> > gc.cd_info is kzalloc:ed on probe. In case probe fails after this allocation, the
> > memory is kfree:d but the variable is NOT zeroed out.
> > 
> > AFAICT, the above leads to a double-free on exit by the added line.
> > 
> > I believe gd.cd_info should be kfree:d on remove instead.
> > 
> > However, might not gc.toc also be kfree:d twice for similar reasons?
> > 
> > I could easily be mistaken.
> 
> >From taking a quick look the other day, that's my conclusion too. I
> don't think the patch is correct, but I don't think the surrounding code
> is correct right now either.

Thanks for the review from both of you, I'll keep this commit in the
tree.

greg k-h
Peter Rosin April 27, 2021, 2:03 p.m. UTC | #4
On 2021-04-27 15:01, Greg KH wrote:
> On Fri, Apr 23, 2021 at 08:20:30AM -0600, Jens Axboe wrote:
>> On 4/22/21 3:29 PM, Peter Rosin wrote:
>>>> This reverts commit 093c48213ee37c3c3ff1cf5ac1aa2a9d8bc66017.
>>>
>>> The reverted patch looks fishy.
>>>
>>> gc.cd_info is kzalloc:ed on probe. In case probe fails after this allocation, the
>>> memory is kfree:d but the variable is NOT zeroed out.
>>>
>>> AFAICT, the above leads to a double-free on exit by the added line.
>>>
>>> I believe gd.cd_info should be kfree:d on remove instead.
>>>
>>> However, might not gc.toc also be kfree:d twice for similar reasons?
>>>
>>> I could easily be mistaken.
>>
>> >From taking a quick look the other day, that's my conclusion too. I
>> don't think the patch is correct, but I don't think the surrounding code
>> is correct right now either.
> 
> Thanks for the review from both of you, I'll keep this commit in the
> tree.
Err, which commit is "this" and what tree are you keeping it in? I
think you mean that you are keeping the revert in your tree with
reverts, and not that you mean that we should keep the original
commit in Linus' tree.

In any case, I'd think that the original memory leak is somewhat
better than the introduced double-free and therefore the revert
should be done.

Cheers,
Peter
Jens Axboe April 27, 2021, 2:39 p.m. UTC | #5
On 4/27/21 8:03 AM, Peter Rosin wrote:
> On 2021-04-27 15:01, Greg KH wrote:
>> On Fri, Apr 23, 2021 at 08:20:30AM -0600, Jens Axboe wrote:
>>> On 4/22/21 3:29 PM, Peter Rosin wrote:
>>>>> This reverts commit 093c48213ee37c3c3ff1cf5ac1aa2a9d8bc66017.
>>>>
>>>> The reverted patch looks fishy.
>>>>
>>>> gc.cd_info is kzalloc:ed on probe. In case probe fails after this allocation, the
>>>> memory is kfree:d but the variable is NOT zeroed out.
>>>>
>>>> AFAICT, the above leads to a double-free on exit by the added line.
>>>>
>>>> I believe gd.cd_info should be kfree:d on remove instead.
>>>>
>>>> However, might not gc.toc also be kfree:d twice for similar reasons?
>>>>
>>>> I could easily be mistaken.
>>>
>>> >From taking a quick look the other day, that's my conclusion too. I
>>> don't think the patch is correct, but I don't think the surrounding code
>>> is correct right now either.
>>
>> Thanks for the review from both of you, I'll keep this commit in the
>> tree.
> Err, which commit is "this" and what tree are you keeping it in? I
> think you mean that you are keeping the revert in your tree with
> reverts, and not that you mean that we should keep the original
> commit in Linus' tree.
> 
> In any case, I'd think that the original memory leak is somewhat
> better than the introduced double-free and therefore the revert
> should be done.

It should probably look like the below, though I doubt it matters
since only one device is supported anyway. As long as the free
happens post unregister, it likely won't make a difference. But
it is cleaner and easier to verify, and should double device support
ever be introduced, the existing code is buggy.

But given that, I don't think we should keep the revert patch.

diff --git a/drivers/cdrom/gdrom.c b/drivers/cdrom/gdrom.c
index 9874fc1c815b..02d369881165 100644
--- a/drivers/cdrom/gdrom.c
+++ b/drivers/cdrom/gdrom.c
@@ -831,6 +831,8 @@ static int remove_gdrom(struct platform_device *devptr)
 	if (gdrom_major)
 		unregister_blkdev(gdrom_major, GDROM_DEV_NAME);
 	unregister_cdrom(gd.cd_info);
+	kfree(gd.toc);
+	kfree(gd.cd_info);
 
 	return 0;
 }
@@ -862,8 +864,6 @@ static void __exit exit_gdrom(void)
 {
 	platform_device_unregister(pd);
 	platform_driver_unregister(&gdrom_driver);
-	kfree(gd.toc);
-	kfree(gd.cd_info);
 }
 
 module_init(init_gdrom);
Greg Kroah-Hartman April 27, 2021, 4:11 p.m. UTC | #6
On Tue, Apr 27, 2021 at 04:03:01PM +0200, Peter Rosin wrote:
> On 2021-04-27 15:01, Greg KH wrote:
> > On Fri, Apr 23, 2021 at 08:20:30AM -0600, Jens Axboe wrote:
> >> On 4/22/21 3:29 PM, Peter Rosin wrote:
> >>>> This reverts commit 093c48213ee37c3c3ff1cf5ac1aa2a9d8bc66017.
> >>>
> >>> The reverted patch looks fishy.
> >>>
> >>> gc.cd_info is kzalloc:ed on probe. In case probe fails after this allocation, the
> >>> memory is kfree:d but the variable is NOT zeroed out.
> >>>
> >>> AFAICT, the above leads to a double-free on exit by the added line.
> >>>
> >>> I believe gd.cd_info should be kfree:d on remove instead.
> >>>
> >>> However, might not gc.toc also be kfree:d twice for similar reasons?
> >>>
> >>> I could easily be mistaken.
> >>
> >> >From taking a quick look the other day, that's my conclusion too. I
> >> don't think the patch is correct, but I don't think the surrounding code
> >> is correct right now either.
> > 
> > Thanks for the review from both of you, I'll keep this commit in the
> > tree.
> Err, which commit is "this" and what tree are you keeping it in? I
> think you mean that you are keeping the revert in your tree with
> reverts, and not that you mean that we should keep the original
> commit in Linus' tree.

That is correct, I will be keeping this revert in my tree.

> In any case, I'd think that the original memory leak is somewhat
> better than the introduced double-free and therefore the revert
> should be done.

Will do that.

thanks,

greg k-h
Greg Kroah-Hartman April 27, 2021, 4:12 p.m. UTC | #7
On Tue, Apr 27, 2021 at 08:39:15AM -0600, Jens Axboe wrote:
> On 4/27/21 8:03 AM, Peter Rosin wrote:
> > On 2021-04-27 15:01, Greg KH wrote:
> >> On Fri, Apr 23, 2021 at 08:20:30AM -0600, Jens Axboe wrote:
> >>> On 4/22/21 3:29 PM, Peter Rosin wrote:
> >>>>> This reverts commit 093c48213ee37c3c3ff1cf5ac1aa2a9d8bc66017.
> >>>>
> >>>> The reverted patch looks fishy.
> >>>>
> >>>> gc.cd_info is kzalloc:ed on probe. In case probe fails after this allocation, the
> >>>> memory is kfree:d but the variable is NOT zeroed out.
> >>>>
> >>>> AFAICT, the above leads to a double-free on exit by the added line.
> >>>>
> >>>> I believe gd.cd_info should be kfree:d on remove instead.
> >>>>
> >>>> However, might not gc.toc also be kfree:d twice for similar reasons?
> >>>>
> >>>> I could easily be mistaken.
> >>>
> >>> >From taking a quick look the other day, that's my conclusion too. I
> >>> don't think the patch is correct, but I don't think the surrounding code
> >>> is correct right now either.
> >>
> >> Thanks for the review from both of you, I'll keep this commit in the
> >> tree.
> > Err, which commit is "this" and what tree are you keeping it in? I
> > think you mean that you are keeping the revert in your tree with
> > reverts, and not that you mean that we should keep the original
> > commit in Linus' tree.
> > 
> > In any case, I'd think that the original memory leak is somewhat
> > better than the introduced double-free and therefore the revert
> > should be done.
> 
> It should probably look like the below, though I doubt it matters
> since only one device is supported anyway. As long as the free
> happens post unregister, it likely won't make a difference. But
> it is cleaner and easier to verify, and should double device support
> ever be introduced, the existing code is buggy.
> 
> But given that, I don't think we should keep the revert patch.
> 
> diff --git a/drivers/cdrom/gdrom.c b/drivers/cdrom/gdrom.c
> index 9874fc1c815b..02d369881165 100644
> --- a/drivers/cdrom/gdrom.c
> +++ b/drivers/cdrom/gdrom.c
> @@ -831,6 +831,8 @@ static int remove_gdrom(struct platform_device *devptr)
>  	if (gdrom_major)
>  		unregister_blkdev(gdrom_major, GDROM_DEV_NAME);
>  	unregister_cdrom(gd.cd_info);
> +	kfree(gd.toc);
> +	kfree(gd.cd_info);
>  
>  	return 0;
>  }
> @@ -862,8 +864,6 @@ static void __exit exit_gdrom(void)
>  {
>  	platform_device_unregister(pd);
>  	platform_driver_unregister(&gdrom_driver);
> -	kfree(gd.toc);
> -	kfree(gd.cd_info);
>  }
>  
>  module_init(init_gdrom);
> 
> -- 
> Jens Axboe
> 

I'll add this fix to the tree after the revert, and give you the credit
for the fix :)

thanks,

greg k-h
Jens Axboe April 27, 2021, 5:01 p.m. UTC | #8
On 4/27/21 10:12 AM, Greg KH wrote:
> On Tue, Apr 27, 2021 at 08:39:15AM -0600, Jens Axboe wrote:
>> On 4/27/21 8:03 AM, Peter Rosin wrote:
>>> On 2021-04-27 15:01, Greg KH wrote:
>>>> On Fri, Apr 23, 2021 at 08:20:30AM -0600, Jens Axboe wrote:
>>>>> On 4/22/21 3:29 PM, Peter Rosin wrote:
>>>>>>> This reverts commit 093c48213ee37c3c3ff1cf5ac1aa2a9d8bc66017.
>>>>>>
>>>>>> The reverted patch looks fishy.
>>>>>>
>>>>>> gc.cd_info is kzalloc:ed on probe. In case probe fails after this allocation, the
>>>>>> memory is kfree:d but the variable is NOT zeroed out.
>>>>>>
>>>>>> AFAICT, the above leads to a double-free on exit by the added line.
>>>>>>
>>>>>> I believe gd.cd_info should be kfree:d on remove instead.
>>>>>>
>>>>>> However, might not gc.toc also be kfree:d twice for similar reasons?
>>>>>>
>>>>>> I could easily be mistaken.
>>>>>
>>>>> >From taking a quick look the other day, that's my conclusion too. I
>>>>> don't think the patch is correct, but I don't think the surrounding code
>>>>> is correct right now either.
>>>>
>>>> Thanks for the review from both of you, I'll keep this commit in the
>>>> tree.
>>> Err, which commit is "this" and what tree are you keeping it in? I
>>> think you mean that you are keeping the revert in your tree with
>>> reverts, and not that you mean that we should keep the original
>>> commit in Linus' tree.
>>>
>>> In any case, I'd think that the original memory leak is somewhat
>>> better than the introduced double-free and therefore the revert
>>> should be done.
>>
>> It should probably look like the below, though I doubt it matters
>> since only one device is supported anyway. As long as the free
>> happens post unregister, it likely won't make a difference. But
>> it is cleaner and easier to verify, and should double device support
>> ever be introduced, the existing code is buggy.
>>
>> But given that, I don't think we should keep the revert patch.
>>
>> diff --git a/drivers/cdrom/gdrom.c b/drivers/cdrom/gdrom.c
>> index 9874fc1c815b..02d369881165 100644
>> --- a/drivers/cdrom/gdrom.c
>> +++ b/drivers/cdrom/gdrom.c
>> @@ -831,6 +831,8 @@ static int remove_gdrom(struct platform_device *devptr)
>>  	if (gdrom_major)
>>  		unregister_blkdev(gdrom_major, GDROM_DEV_NAME);
>>  	unregister_cdrom(gd.cd_info);
>> +	kfree(gd.toc);
>> +	kfree(gd.cd_info);
>>  
>>  	return 0;
>>  }
>> @@ -862,8 +864,6 @@ static void __exit exit_gdrom(void)
>>  {
>>  	platform_device_unregister(pd);
>>  	platform_driver_unregister(&gdrom_driver);
>> -	kfree(gd.toc);
>> -	kfree(gd.cd_info);
>>  }
>>  
>>  module_init(init_gdrom);
>>
>> -- 
>> Jens Axboe
>>
> 
> I'll add this fix to the tree after the revert, and give you the credit
> for the fix :)

Sounds good, thanks Greg.

Patch
diff mbox series

diff --git a/drivers/cdrom/gdrom.c b/drivers/cdrom/gdrom.c
index 9874fc1c815b..466fc3eee8bb 100644
--- a/drivers/cdrom/gdrom.c
+++ b/drivers/cdrom/gdrom.c
@@ -863,7 +863,6 @@  static void __exit exit_gdrom(void)
 	platform_device_unregister(pd);
 	platform_driver_unregister(&gdrom_driver);
 	kfree(gd.toc);
-	kfree(gd.cd_info);
 }
 
 module_init(init_gdrom);