linux-kernel - Re: [PATCH 167/190] Revert "gdrom: fix a memory leak bug"

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 27 Apr 2021 16:03:01 +0200
From:   Peter Rosin <peda@...ntia.se>
To:     Greg KH <gregkh@...uxfoundation.org>, Jens Axboe <axboe@...nel.dk>
Cc:     linux-kernel@...r.kernel.org
Subject: Re: [PATCH 167/190] Revert "gdrom: fix a memory leak bug"

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