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

[Greg KH <gregkh@linuxfoundation.org>]

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".

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

thanks,

greg k-h