linux-kernel - Re: [PATCH] ata: pata_platform: Fix a NULL pointer dereference in __pata_platform

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:   Sat, 29 Jan 2022 09:12:19 +0900
From:   Damien Le Moal <damien.lemoal@...nsource.wdc.com>
To:     Greg KH <gregkh@...uxfoundation.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     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 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.

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

-- 
Damien Le Moal
Western Digital Research