Date:   Mon, 31 Jan 2022 09:11:44 +0900
From:   Damien Le Moal <damien.lemoal@...nsource.wdc.com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     Linus Torvalds <torvalds@...ux-foundation.org>,
        Zhou Qingyang <zhou1615@....edu>, kjlu@....edu,
        Alexander Shiyan <shc_work@...l.ru>,
        Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
        Jens Axboe <axboe@...nel.dk>, linux-ide@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ata: pata_platform: Fix a NULL pointer dereference in
 __pata_platform_probe()

On 2022/01/29 16:06, Greg KH wrote:
> On Sat, Jan 29, 2022 at 09:12:19AM +0900, Damien Le Moal wrote:
>> On 1/29/22 00:57, Greg KH wrote:
>>> On Fri, Jan 28, 2022 at 08:50:04PM +0900, Damien Le Moal wrote:
>>>> On 1/28/22 19:11, Greg KH wrote:
>>>>> On Tue, Jan 25, 2022 at 12:45:25AM +0800, Zhou Qingyang wrote:
>>>>>> In __pata_platform_probe(), devm_kzalloc() is assigned to ap->ops and
>>>>>> there is a dereference of it right after that, which could introduce a
>>>>>> NULL pointer dereference bug.
>>>>>>
>>>>>> Fix this by adding a NULL check of ap->ops.
>>>>>>
>>>>>> This bug was found by a static analyzer.
>>>>>>
>>>>>> Builds with 'make allyesconfig' show no new warnings,
>>>>>> and our static analyzer no longer warns about this code.
>>>>>>
>>>>>> Fixes: f3d5e4f18dba ("ata: pata_of_platform: Allow to use 16-bit wide data transfer")
>>>>>> Signed-off-by: Zhou Qingyang <zhou1615@....edu>
>>>>>> ---
>>>>>
>>>>> As stated in the past, please do not make contributions to the Linux
>>>>> kernel until umn.edu has properly resolved its development issues.
>>>>
>>>> Aouch. My apologies. I forgot about this. Thank you for the reminder.
>>>>
>>>>>
>>>>>> The analysis employs differential checking to identify inconsistent 
>>>>>> security operations (e.g., checks or kfrees) between two code paths 
>>>>>> and confirms that the inconsistent operations are not recovered in the
>>>>>> current function or the callers, so they constitute bugs. 
>>>>>>
>>>>>> Note that, as a bug found by static analysis, it can be a false
>>>>>> positive or hard to trigger. Multiple researchers have cross-reviewed
>>>>>> the bug.
>>>>>>
>>>>>>  drivers/ata/pata_platform.c | 2 ++
>>>>>>  1 file changed, 2 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/ata/pata_platform.c b/drivers/ata/pata_platform.c
>>>>>> index 028329428b75..021ef9cbcbc1 100644
>>>>>> --- a/drivers/ata/pata_platform.c
>>>>>> +++ b/drivers/ata/pata_platform.c
>>>>>> @@ -128,6 +128,8 @@ int __pata_platform_probe(struct device *dev, struct resource *io_res,
>>>>>>  	ap = host->ports[0];
>>>>>>  
>>>>>>  	ap->ops = devm_kzalloc(dev, sizeof(*ap->ops), GFP_KERNEL);
>>>>>> +	if (ap->ops)
>>>>>> +		return -ENOMEM;
>>>>>
>>>>> This change seems to leak memory.  Damien, please revert it.
>>>>
>>>> I fixed the patch when applying, so there is no leak.
>>>
>>> Really?  What happened to the memory that ata_host_alloc() created above
>>> this call?  How is that freed?
>>>
>>>> This is a genuine (potential) bug fix.
>>>
>>> As I tell others, how can kmalloc() ever fail here, so odd of this being
>>> a real bugfix are so low it's not funny.  So take these types of
>>> cleanups as a last-resort only after you have strongly validated that
>>> they are correct.  The current group of people trying to do these fixes
>>> have a horrible track-record and are getting things wrong way more than
>>> they should be.  And so it is worse having code that "looks" correct vs.
>>> something that is "obviously we need to handle this some day".
>>
>> I completely agree that this is not fixing any real bug reported in the
>> field. And as you say, an error here is more than unlikely. I accepted
>> the patch only on the ground of code correctness.
>>
>>>
>>>> Must I revert ?
>>>
>>> If it's buggy you should, see my above question about ata_host_alloc(),
>>> is there a cleanup path somewhere that I am missing?
>>
>> The resources allocated by ata_host_alloc() are attached to the device
>> (devres and drv_data) so they will be freed by ata_devres_release() when
>> the dev is dropped due to the probe error. I think the return that the
>> patch introduces is fine as is.
>>
>> If I am misunderstanding the devres handling, please let me know.
> 
> Where does the data allocated in ata_host_alloc() on this line:
> 	host = kzalloc(sz, GFP_KERNEL);
> 
> Get attached to a devres structure?
> 
> It's a kref-managed data structure (see the call to kref_init() a few
> lines down), and the memory will be freed if you release the last
> reference on the kref, but that has nothing to do with devres.
> 
> There's also the ports memory attached to the host structure as well,
> that is controlled by the lifetime of the kref, not a devres reference
> that I can see.
> 
> Or am I missing some link somewhere here?

Checking again, the path is not super obvious, but it is there:
ata_devres_release() calls ata_host_put(), which drops the kref on the ata_host
memory and will thus free it when the last ref on the dev is dropped.
ata_host_release() is used for that and this function does free everything,
including the port memory within the ata_host structure.

So in essence, the ata_host and ata_port resources are like devres. I wonder if
actually making them real devres would make the code cleaner and simpler.

I will dig into this more to make sure there is no memory leak. As Sasha pointed
out, it seems that the last ref on the dev is actually never dropped. So there
may be an actual memory leak on error but it was there already.


-- 
Damien Le Moal
Western Digital Research

Powered by blists - more mailing lists