Re: [PATCH 27/69] cdrom: gdrom: deallocate struct gdrom_unit fields in remove_gdrom

[Greg Kroah-Hartman]

On Thu, May 06, 2021 at 03:08:08PM +0200, Peter Rosin wrote:
> Hi!
> 
> On 2021-05-06 12:24, Greg Kroah-Hartman wrote:
> > On Mon, May 03, 2021 at 04:13:18PM +0200, Peter Rosin wrote:
> >> Hi!
> >>
> >> On 2021-05-03 13:56, Greg Kroah-Hartman wrote:
> >>> From: Atul Gopinathan <atulgopinathan@xxxxxxxxx>
> >>>
> >>> The fields, "toc" and "cd_info", of "struct gdrom_unit gd" are allocated
> >>> in "probe_gdrom()". Prevent a memory leak by making sure "gd.cd_info" is
> >>> deallocated in the "remove_gdrom()" function.
> >>>
> >>> Also prevent double free of the field "gd.toc" by moving it from the
> >>> module's exit function to "remove_gdrom()". This is because, in
> >>> "probe_gdrom()", the function makes sure to deallocate "gd.toc" in case
> >>> of any errors, so the exit function invoked later would again free
> >>> "gd.toc".
> >>>
> >>> The patch also maintains consistency by deallocating the above mentioned
> >>> fields in "remove_gdrom()" along with another memory allocated field
> >>> "gd.disk".
> >>>
> >>> Suggested-by: Jens Axboe <axboe@xxxxxxxxx>
> >>> Cc: Peter Rosin <peda@xxxxxxxxxx>
> >>> Cc: stable <stable@xxxxxxxxxxxxxxx>
> >>> Signed-off-by: Atul Gopinathan <atulgopinathan@xxxxxxxxx>
> >>> Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>
> >>> ---
> >>>  drivers/cdrom/gdrom.c | 3 ++-
> >>>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>>
> >>> diff --git a/drivers/cdrom/gdrom.c b/drivers/cdrom/gdrom.c
> >>> index 7f681320c7d3..6c4f6139f853 100644
> >>> --- a/drivers/cdrom/gdrom.c
> >>> +++ b/drivers/cdrom/gdrom.c
> >>> @@ -830,6 +830,8 @@ static int remove_gdrom(struct platform_device *devptr)
> >>>  	if (gdrom_major)
> >>>  		unregister_blkdev(gdrom_major, GDROM_DEV_NAME);
> >>>  	unregister_cdrom(gd.cd_info);
> >>> +	kfree(gd.cd_info);
> >>> +	kfree(gd.toc);
> >>>  
> >>>  	return 0;
> >>>  }
> >>> @@ -861,7 +863,6 @@ static void __exit exit_gdrom(void)
> >>>  {
> >>>  	platform_device_unregister(pd);
> >>>  	platform_driver_unregister(&gdrom_driver);
> >>> -	kfree(gd.toc);
> >>>  }
> >>>  
> >>>  module_init(init_gdrom);
> >>>
> >>
> >> I worry about the gd.toc = NULL; statement in init_gdrom(). It sets off
> >> all kinds of warnings with me. It looks completely bogus, but the fact
> >> that it's there at all makes me go hmmmm.
> > 
> > Yeah, that's bogus.
> > 
> >> probe_gdrom_setupcd() will arrange for gdrom_ops to be used, including
> >> .get_last_session pointing to gdrom_get_last_session() 
> >>
> >> gdrom_get_last_session() will use gd.toc, if it is non-NULL.
> >>
> >> The above will all be registered externally to the driver with the call
> >> to register_cdrom() in probe_gdrom(), before a possible stale gd.toc is
> >> overwritten with a new one at the end of probe_gdrom().
> > 
> > But can that really happen given that it hasn't ever happened before in
> > a real system?  :)
> > 
> >> Side note, .get_last_session is an interesting name in this context, but
> >> I have no idea if it might be called in the "bad" window (but relying on
> >> that to not be the case would be ... subtle).
> >>
> >> So, by simply freeing gd.toc in remove_gdrom() without also setting
> >> it to NULL, it looks like a potential use after free of gd.toc is
> >> introduced, replacing a potential leak. Not good.
> > 
> > So should we set it to NULL after freeing it?  Is that really going to
> > help here given that the probe failed?  Nothing can use it after
> > remove_gdrom() is called because unregiser_* is called already.
> > 
> > I don't see the race here, sorry.
> > 
> >> The same is not true for gd.cd_info as far as I can tell, but it's a bit
> >> subtle. gdrom_probe() calls gdrom_execute_diagnostics() before the stale
> >> gd.cd_info is overwritten, and gdrom_execute_diagnostic() passes the
> >> stale pointer to gdrom_hardreset(), which luckily doesn't use it. But
> >> this is - as hinted - a bit too subtle for me. I would prefer to have
> >> remove_gdrom() also clear out the gd.cd_info pointer.
> > 
> > Ok, but again, how can that be used after remove_gdrom() is called?
> > 
> >> In addition to adding these clears of gd.toc and gd.cd_info to
> >> remove_gdrom(), they also need to be cleared in case probe fails.
> >>
> >> Or instead, maybe add a big fat
> >> 	memset(&gd, 0, sizeof(gd));
> >> at the top of probe?
> > 
> > Really, that's what is happening today as there is only 1 device here,
> > and the whole structure was zeroed out already.  So that would be a
> > no-op.
> > 
> >> Or maybe the struct gdrom_unit should simply be kzalloc:ed? But that
> >> triggers some . to -> churn...
> > 
> > Yes, ideally that would be the correct change, but given that you can
> > only have 1 device in the system at a time of this type, it's not going
> > to make much difference at all here.
> > 
> >> Anyway, the patch as proposed gets a NACK from me.
> > 
> > Why?  It fixes the obvious memory leak, right?  Worst case you are
> > saying we should also set to NULL these pointers, but I can not see how
> > they are accessed as we have already torn everything down.
> 
> I'm thinking this:
> 
> 1. init_gdrom() is called. gd.toc is NULL and is bogusly re-set to NULL.
> 2. probe_gdrom() is called and succeeds. gd.toc is allocted.
> 3. device is used, etc etc, whatever
> 4. remove_gdrom() is called. gd.toc is freed (but not set to NULL).
> 5. probe_gdrom() is called again. Boom.

Ah.  Well, adding/removing platform devices is a hard thing, and if you
do it, you deserve the pieces you get :)

It would be trivial to fix this by setting all of &gd to 0 as you
mention above, so yes, that would be good.  But that's an add-on patch
and not relevant to this "fix" here.

> In 5, gd.toc is not NULL, and is pointing to whatever. It is
> potentially used by probe_gdrom() before it is (re-)allocated.
> 
> I suppose the above can only happen if the module is compiled in.

You can add/remove platform devices through sysfs if the code is a
module as well.

I'll go make a new commit that zeros everything at probe_gdrom() that
goes on top of this one.

thanks,

greg k-h