Re: [PATCH] ata: pata_platform: Fix a NULL pointer dereference in __pata_platform_probe()

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

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@umn.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.

In any case, I will make sure to ignore patches from umn.edu for now.
Thanks.

-- 
Damien Le Moal
Western Digital Research