All of lore.kernel.org
 help / color / mirror / Atom feed
From: Peter Rosin <peda@axentia.se>
To: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: linux-kernel@vger.kernel.org,
	Atul Gopinathan <atulgopinathan@gmail.com>,
	Jens Axboe <axboe@kernel.dk>, stable <stable@vger.kernel.org>
Subject: Re: [PATCH 27/69] cdrom: gdrom: deallocate struct gdrom_unit fields in remove_gdrom
Date: Thu, 6 May 2021 15:08:08 +0200	[thread overview]
Message-ID: <dd716d04-b9fa-986a-50dd-5c385ea745b2@axentia.se> (raw)
In-Reply-To: <YJPDzqAAnP0jDRDF@kroah.com>

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

  reply	other threads:[~2021-05-06 13:08 UTC|newest]

Thread overview: 97+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-03 11:56 [PATCH 00/69] "Revert and fix properly" patch series based on umn.edu re-review Greg Kroah-Hartman
2021-05-03 11:56 ` [PATCH 01/69] Revert "crypto: cavium/nitrox - add an error message to explain the failure of pci_request_mem_regions" Greg Kroah-Hartman
2021-05-03 11:56 ` [PATCH 02/69] Revert "ACPI: custom_method: fix memory leaks" Greg Kroah-Hartman
2021-05-03 11:56 ` [PATCH 03/69] Revert "media: rcar_drif: fix a memory disclosure" Greg Kroah-Hartman
2021-05-03 11:56 ` [PATCH 04/69] Revert "hwmon: (lm80) fix a missing check of bus read in lm80 probe" Greg Kroah-Hartman
2021-05-03 11:56 ` [PATCH 05/69] Revert "serial: mvebu-uart: Fix to avoid a potential NULL pointer dereference" Greg Kroah-Hartman
2021-05-03 11:56 ` [PATCH 06/69] Revert "media: usb: gspca: add a missed check for goto_low_power" Greg Kroah-Hartman
2021-05-03 11:56 ` [PATCH 07/69] Revert "ALSA: sb: fix a missing check of snd_ctl_add" Greg Kroah-Hartman
2021-05-03 11:56 ` [PATCH 08/69] Revert "leds: lp5523: fix a missing check of return value of lp55xx_read" Greg Kroah-Hartman
2021-05-03 11:56 ` [PATCH 09/69] leds: lp5523: check return value of lp5xx_read and jump to cleanup code Greg Kroah-Hartman
2021-05-03 19:36   ` Jacek Anaszewski
2021-05-13 15:25     ` Greg Kroah-Hartman
2021-05-03 11:56 ` [PATCH 10/69] Revert "serial: max310x: pass return value of spi_register_driver" Greg Kroah-Hartman
2021-05-03 11:56 ` [PATCH 11/69] serial: max310x: unregister uart driver in case of failure and abort Greg Kroah-Hartman
2021-05-03 11:56 ` [PATCH 12/69] Revert "rtlwifi: fix a potential NULL pointer dereference" Greg Kroah-Hartman
2021-05-03 11:56 ` [PATCH 13/69] net: rtlwifi: properly check for alloc_workqueue() failure Greg Kroah-Hartman
2021-05-03 11:56 ` [PATCH 14/69] Revert "net: fujitsu: fix a potential NULL pointer dereference" Greg Kroah-Hartman
2021-05-03 11:56 ` [PATCH 15/69] net: fujitsu: fix potential null-ptr-deref Greg Kroah-Hartman
2021-05-03 11:56 ` [PATCH 16/69] Revert "net/smc: fix a NULL pointer dereference" Greg Kroah-Hartman
2021-05-03 11:56 ` [PATCH 17/69] net/smc: properly handle workqueue allocation failure Greg Kroah-Hartman
2021-05-03 11:56 ` [PATCH 18/69] Revert "net: caif: replace BUG_ON with recovery code" Greg Kroah-Hartman
2021-05-03 11:56 ` [PATCH 19/69] net: caif: remove BUG_ON(dev == NULL) in caif_xmit Greg Kroah-Hartman
2021-05-03 11:56 ` [PATCH 20/69] Revert "net: stmicro: fix a missing check of clk_prepare" Greg Kroah-Hartman
2021-05-03 11:56 ` [PATCH 21/69] net: stmicro: handle clk_prepare() failure during init Greg Kroah-Hartman
2021-05-03 11:56 ` [PATCH 22/69] Revert "niu: fix missing checks of niu_pci_eeprom_read" Greg Kroah-Hartman
2021-05-03 11:56 ` [PATCH 23/69] ethernet: sun: niu: fix missing checks of niu_pci_eeprom_read() Greg Kroah-Hartman
2021-05-03 11:56 ` [PATCH 24/69] Revert "qlcnic: Avoid potential NULL pointer dereference" Greg Kroah-Hartman
2021-05-03 11:56 ` [PATCH 25/69] qlcnic: Add null check after calling netdev_alloc_skb Greg Kroah-Hartman
2021-05-03 11:56 ` [PATCH 26/69] Revert "gdrom: fix a memory leak bug" Greg Kroah-Hartman
2021-05-03 11:56 ` [PATCH 27/69] cdrom: gdrom: deallocate struct gdrom_unit fields in remove_gdrom Greg Kroah-Hartman
2021-05-03 14:13   ` Peter Rosin
2021-05-06 10:24     ` Greg Kroah-Hartman
2021-05-06 13:08       ` Peter Rosin [this message]
2021-05-06 13:43         ` Greg Kroah-Hartman
2021-05-06 14:00           ` [PATCH] cdrom: gdrom: initialize global variable at init time Greg Kroah-Hartman
2021-05-06 15:47             ` Peter Rosin
2021-05-06 14:32         ` [PATCH 27/69] cdrom: gdrom: deallocate struct gdrom_unit fields in remove_gdrom Atul Gopinathan
2021-05-06 15:43           ` Peter Rosin
2021-05-06 16:40             ` Atul Gopinathan
2021-05-03 11:56 ` [PATCH 28/69] Revert "char: hpet: fix a missing check of ioremap" Greg Kroah-Hartman
2021-05-03 11:56 ` [PATCH 29/69] char: hpet: add checks after calling ioremap Greg Kroah-Hartman
2021-05-03 11:56 ` [PATCH 30/69] Revert "scsi: ufs: fix a missing check of devm_reset_control_get" Greg Kroah-Hartman
2021-05-03 11:56 ` [PATCH 31/69] scsi: ufs: handle cleanup correctly on devm_reset_control_get error Greg Kroah-Hartman
2021-05-03 11:56 ` [PATCH 32/69] Revert "ALSA: gus: add a check of the status of snd_ctl_add" Greg Kroah-Hartman
2021-05-03 11:57 ` [PATCH 33/69] ALSA: gus: properly handle snd_ctl_add() error Greg Kroah-Hartman
2021-05-03 12:28   ` Takashi Iwai
2021-05-03 16:55     ` Greg Kroah-Hartman
2021-05-03 11:57 ` [PATCH 34/69] Revert "ALSA: sb8: add a check for request_region" Greg Kroah-Hartman
2021-05-03 11:57 ` [PATCH 35/69] ALSA: sb8: Add a comment note regarding an unused pointer Greg Kroah-Hartman
2021-05-03 11:57 ` [PATCH 36/69] Revert "ALSA: usx2y: Fix potential NULL pointer dereference" Greg Kroah-Hartman
2021-05-03 11:57 ` [PATCH 37/69] ALSA: usx2y: check for failure of usb_alloc_urb() Greg Kroah-Hartman
2021-05-03 20:33   ` Jaroslav Kysela
2021-05-04  8:27     ` Takashi Iwai
2021-05-04 16:31       ` Greg Kroah-Hartman
2021-05-03 11:57 ` [PATCH 38/69] Revert "video: hgafb: fix potential NULL pointer dereference" Greg Kroah-Hartman
2021-05-03 11:57 ` [PATCH 39/69] video: hgafb: fix potential NULL pointer dereference Greg Kroah-Hartman
2021-05-03 11:57 ` [PATCH 40/69] Revert "isdn: mISDNinfineon: fix potential NULL pointer dereference" Greg Kroah-Hartman
2021-05-03 11:57 ` [PATCH 41/69] isdn: mISDNinfineon: check/cleanup ioremap failure correctly in setup_io Greg Kroah-Hartman
2021-05-03 11:57 ` [PATCH 42/69] Revert "ath6kl: return error code in ath6kl_wmi_set_roam_lrssi_cmd()" Greg Kroah-Hartman
2021-05-03 11:57 ` [PATCH 43/69] ath6kl: return error code in ath6kl_wmi_set_roam_lrssi_cmd() Greg Kroah-Hartman
2021-05-03 11:57 ` [PATCH 44/69] Revert "rapidio: fix a NULL pointer dereference when create_workqueue() fails" Greg Kroah-Hartman
2021-05-03 11:57 ` [PATCH 45/69] rapidio: handle create_workqueue() failure Greg Kroah-Hartman
2021-05-03 11:57 ` [PATCH 46/69] Revert "isdn: mISDN: Fix potential NULL pointer dereference of kzalloc" Greg Kroah-Hartman
2021-05-03 11:57 ` [PATCH 47/69] isdn: mISDN: correctly handle ph_info allocation failure in hfcsusb_ph_info Greg Kroah-Hartman
2021-05-03 11:57 ` [PATCH 48/69] Revert "ecryptfs: replace BUG_ON with error handling code" Greg Kroah-Hartman
2021-05-03 11:57 ` [PATCH 49/69] fs: ecryptfs: remove BUG_ON from crypt_scatterlist Greg Kroah-Hartman
2021-05-03 11:57 ` [PATCH 50/69] Revert "dmaengine: qcom_hidma: Check for driver register failure" Greg Kroah-Hartman
2021-05-03 12:57   ` Sinan Kaya
2021-05-03 13:31   ` Vinod Koul
2021-05-03 11:57 ` [PATCH 51/69] dmaengine: qcom_hidma: comment platform_driver_register call Greg Kroah-Hartman
2021-05-03 12:57   ` Sinan Kaya
2021-05-03 13:31   ` Vinod Koul
2021-05-03 11:57 ` [PATCH 52/69] Revert "libertas: add checks for the return value of sysfs_create_group" Greg Kroah-Hartman
2021-05-03 11:57 ` [PATCH 53/69] libertas: register sysfs groups properly Greg Kroah-Hartman
2021-05-03 11:57 ` [PATCH 54/69] Revert "ASoC: rt5645: fix a NULL pointer dereference" Greg Kroah-Hartman
2021-05-03 11:57 ` [PATCH 55/69] ASoC: rt5645: add error checking to rt5645_probe function Greg Kroah-Hartman
2021-05-25 21:38   ` Mark Brown
2021-05-25 22:02     ` Phillip Potter
2021-05-27 16:31       ` Mark Brown
2021-05-30  8:58         ` Phillip Potter
2021-05-03 11:57 ` [PATCH 56/69] Revert "ASoC: cs43130: fix a NULL pointer dereference" Greg Kroah-Hartman
2021-05-03 11:57 ` [PATCH 57/69] ASoC: cs43130: handle errors in cs43130_probe() properly Greg Kroah-Hartman
2021-05-03 11:57 ` [PATCH 58/69] Revert "media: dvb: Add check on sp8870_readreg" Greg Kroah-Hartman
2021-05-03 11:57 ` [PATCH 59/69] media: dvb: Add check on sp8870_readreg return Greg Kroah-Hartman
2021-05-03 11:57 ` [PATCH 60/69] Revert "media: gspca: mt9m111: Check write_bridge for timeout" Greg Kroah-Hartman
2021-05-03 11:57 ` [PATCH 61/69] media: gspca: mt9m111: Check write_bridge for timeout Greg Kroah-Hartman
2021-05-03 11:57 ` [PATCH 62/69] Revert "media: gspca: Check the return value of write_bridge for timeout" Greg Kroah-Hartman
2021-05-03 11:57 ` [PATCH 63/69] media: gspca: properly check for errors in po1030_probe() Greg Kroah-Hartman
2021-05-03 11:57 ` [PATCH 64/69] Revert "net: liquidio: fix a NULL pointer dereference" Greg Kroah-Hartman
2021-05-03 11:57 ` [PATCH 65/69] net: liquidio: Add missing null pointer checks Greg Kroah-Hartman
2021-05-03 11:57 ` [PATCH 66/69] Revert "video: imsttfb: fix potential NULL pointer dereferences" Greg Kroah-Hartman
2021-05-03 13:41   ` Rob Herring
2021-05-03 11:57 ` [PATCH 67/69] video: imsttfb: check for ioremap() failures Greg Kroah-Hartman
2021-05-03 13:40   ` Rob Herring
2021-05-03 11:57 ` [PATCH 68/69] Revert "brcmfmac: add a check for the status of usb_register" Greg Kroah-Hartman
2021-05-03 11:57 ` [PATCH 69/69] brcmfmac: properly check for bus register errors Greg Kroah-Hartman
2021-05-13 16:59 ` [PATCH 00/69] "Revert and fix properly" patch series based on umn.edu re-review Greg Kroah-Hartman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=dd716d04-b9fa-986a-50dd-5c385ea745b2@axentia.se \
    --to=peda@axentia.se \
    --cc=atulgopinathan@gmail.com \
    --cc=axboe@kernel.dk \
    --cc=gregkh@linuxfoundation.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=stable@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.