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

[Peter Rosin]

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.

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().

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.

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.

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?
Or maybe the struct gdrom_unit should simply be kzalloc:ed? But that
triggers some . to -> churn...

Anyway, the patch as proposed gets a NACK from me.

Cheers,
Peter