From: Atul Gopinathan atulgopinathan@gmail.com
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@kernel.dk Cc: Peter Rosin peda@axentia.se Cc: stable stable@vger.kernel.org Signed-off-by: Atul Gopinathan atulgopinathan@gmail.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org --- 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);
Hi!
On 2021-05-03 13:56, Greg Kroah-Hartman wrote:
From: Atul Gopinathan atulgopinathan@gmail.com
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@kernel.dk Cc: Peter Rosin peda@axentia.se Cc: stable stable@vger.kernel.org Signed-off-by: Atul Gopinathan atulgopinathan@gmail.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
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
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@gmail.com
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@kernel.dk Cc: Peter Rosin peda@axentia.se Cc: stable stable@vger.kernel.org Signed-off-by: Atul Gopinathan atulgopinathan@gmail.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
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.
thanks,
greg k-h
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@gmail.com
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@kernel.dk Cc: Peter Rosin peda@axentia.se Cc: stable stable@vger.kernel.org Signed-off-by: Atul Gopinathan atulgopinathan@gmail.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
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.
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.
Without this patch, we are "safe" because gd.toc still points to the old thing which is leaked once a new gd.toc is allocated by the second probe.
Cheers, Peter
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@gmail.com
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@kernel.dk Cc: Peter Rosin peda@axentia.se Cc: stable stable@vger.kernel.org Signed-off-by: Atul Gopinathan atulgopinathan@gmail.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
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:
- init_gdrom() is called. gd.toc is NULL and is bogusly re-set to NULL.
- probe_gdrom() is called and succeeds. gd.toc is allocted.
- device is used, etc etc, whatever
- remove_gdrom() is called. gd.toc is freed (but not set to NULL).
- 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
From: Greg Kroah-Hartman gregkh@linuxfoundation.org
As Peter points out, if we were to disconnect and then reconnect this driver from a device, the "global" state of the device would contain odd values and could cause problems. Fix this up by just initializing the whole thing to 0 at probe() time.
Ideally this would be a per-device variable, but given the age and the total lack of users of it, that would require a lot of s/./->/g changes for really no good reason.
Reported-by: Peter Rosin peda@axentia.se Cc: Jens Axboe axboe@kernel.dk Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org ---
Note, this goes on top of my previous "gdrom" patch submitted here: https://lore.kernel.org/lkml/20210503115736.2104747-28-gregkh@linuxfoundatio...
And I'll just take it in the same series.
drivers/cdrom/gdrom.c | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/cdrom/gdrom.c b/drivers/cdrom/gdrom.c index 6c4f6139f853..c6d8c0f59722 100644 --- a/drivers/cdrom/gdrom.c +++ b/drivers/cdrom/gdrom.c @@ -744,6 +744,13 @@ static const struct blk_mq_ops gdrom_mq_ops = { static int probe_gdrom(struct platform_device *devptr) { int err; + + /* + * Ensure our "one" device is initialized properly in case of previous + * usages of it + */ + memset(&gd, 0, sizeof(gd)); + /* Start the device */ if (gdrom_execute_diagnostic() != 1) { pr_warn("ATA Probe for GDROM failed\n"); @@ -847,7 +854,7 @@ static struct platform_driver gdrom_driver = { static int __init init_gdrom(void) { int rc; - gd.toc = NULL; + rc = platform_driver_register(&gdrom_driver); if (rc) return rc;
Hi!
On 2021-05-06 16:00, Greg Kroah-Hartman wrote:
From: Greg Kroah-Hartman gregkh@linuxfoundation.org
As Peter points out, if we were to disconnect and then reconnect this driver from a device, the "global" state of the device would contain odd values and could cause problems. Fix this up by just initializing the whole thing to 0 at probe() time.
Ideally this would be a per-device variable, but given the age and the total lack of users of it, that would require a lot of s/./->/g changes for really no good reason.
Reported-by: Peter Rosin peda@axentia.se Cc: Jens Axboe axboe@kernel.dk Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
Looks good to me.
Reviewed-by: Peter Rosin peda@axentia.se
Thanks, Peter
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@gmail.com
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@kernel.dk Cc: Peter Rosin peda@axentia.se Cc: stable stable@vger.kernel.org Signed-off-by: Atul Gopinathan atulgopinathan@gmail.com Signed-off-by: Greg Kroah-Hartman gregkh@linuxfoundation.org
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:
- init_gdrom() is called. gd.toc is NULL and is bogusly re-set to NULL.
- probe_gdrom() is called and succeeds. gd.toc is allocted.
- device is used, etc etc, whatever
- remove_gdrom() is called. gd.toc is freed (but not set to NULL).
- probe_gdrom() is called again. Boom.
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 guess I'm late and it seems like a conclusion has already been reached, so this mail doesn't really add up to anything. I just had a doubt in my mind which I wanted to clarify:
as Peter said, probe_gdrom() calls "probe_gdrom_setupcd()" which defines the ops, this includes "gdrom_get_last_session()" which is the only function that uses the data of "gd.toc".
It then calls "register_cdrom()", I went through the function definition of this and found only one line which has anything to do with ".get_last_session":
int register_cdrom(struct gendisk *disk, struct cdrom_device_info *cdi) { static char banner_printed; const struct cdrom_device_ops *cdo = cdi->ops; . .<snipped> . -----> ENSURE(cdo, get_last_session, CDC_MULTI_SESSION); . }
The defintion of the ENSURE macro is this:
#define ENSURE(cdo, call, bits) \ do { \ if (cdo->call == NULL) \ WARN_ON_ONCE((cdo)->capability & (bits)); \ } while (0)
So here it is only checking if .get_last_session field is null or not, and not calling it.
Apart from this, I don't see gdrom_get_last_session() being called anywhere. But I could be missing something obvious too.
If you don't mind, could you point out where gd.toc is being used in probe_gdrom() before it is kzalloc-ed in the same function.
Thanks for the review! Atul
Hi!
On 2021-05-06 16:32, Atul Gopinathan wrote:
Apart from this, I don't see gdrom_get_last_session() being called anywhere. But I could be missing something obvious too.
If you don't mind, could you point out where gd.toc is being used in probe_gdrom() before it is kzalloc-ed in the same function.
You are very probably correct in your analysis, and I can't find it in me to spend the time to dig any further.
I simply thought it bad enough to hand off a pointer to a function that uses a stale pointer to some other driver. I never dug into that other module like you did. Relying on that other piece of code to not use the function that was just handed to it is way too subtle (for me at least). When you "register" with something else, you should be ready to get the calls.
This is true especially in the context of what we are fixing up here; broken shit related to people that are fond of weaknesses later to be activated by other innocuous commits.
Cheers, Peter
On Thu, May 06, 2021 at 05:43:14PM +0200, Peter Rosin wrote:
Hi!
On 2021-05-06 16:32, Atul Gopinathan wrote:
Apart from this, I don't see gdrom_get_last_session() being called anywhere. But I could be missing something obvious too.
If you don't mind, could you point out where gd.toc is being used in probe_gdrom() before it is kzalloc-ed in the same function.
You are very probably correct in your analysis, and I can't find it in me to spend the time to dig any further.
I simply thought it bad enough to hand off a pointer to a function that uses a stale pointer to some other driver. I never dug into that other module like you did. Relying on that other piece of code to not use the function that was just handed to it is way too subtle (for me at least). When you "register" with something else, you should be ready to get the calls.
This is true especially in the context of what we are fixing up here; broken shit related to people that are fond of weaknesses later to be activated by other innocuous commits.
Ah, I see, that makes sense. I just wanted to confirm if I was getting things right. Thanks for clarifying!
Regards, Atul
linux-stable-mirror@lists.linaro.org
-
Atul Gopinathan -
Greg Kroah-Hartman -
Peter Rosin