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

[Greg KH]

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@xxxxxxx>
> >>>> ---
> >>>
> >>> 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?

thanks,

greg k-h