| Message ID | 20220813220843.2373004-1-subkhankulov@ispras.ru (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 6ad4194d6a1e1d11b285989cd648ef695b4a93c0 |
| Headers | show |
| Series | platform/chrome: fix double-free in chromeos_laptop_prepare() | expand |
On Sun, Aug 14, 2022 at 01:08:43AM +0300, Rustam Subkhankulov wrote: > If chromeos_laptop_prepare_i2c_peripherals() fails after allocating memory > for 'cros_laptop->i2c_peripherals', this memory is freed at 'err_out' label > and nonzero value is returned. Then chromeos_laptop_destroy() is called, > resulting in double-free error. Alternatively, I would prefer to fix the double-free by setting `i2c_peripherals` to NULL after [1]. [1]: https://elixir.bootlin.com/linux/v5.19/source/drivers/platform/chrome/chromeos_laptop.c#L787 > Found by Linux Verification Center (linuxtesting.org) with SVACE. After a quick glance, I found an invalid memory access at [2] if `i2c_peripherals` is NULL (see [3]). Do you have a real machine to perform some module load/unload tests? Or was the double-free issue discovered by some static analysis? [2]: https://elixir.bootlin.com/linux/v5.19/source/drivers/platform/chrome/chromeos_laptop.c#L860 [3]: https://elixir.bootlin.com/linux/v5.19/source/drivers/platform/chrome/chromeos_laptop.c#L756
On Mon, 2022-08-15 at 05:00 +0000, Tzung-Bi Shih wrote: > Alternatively, I would prefer to fix the double-free by setting > `i2c_peripherals` to NULL after [1]. Since 'cros_laptop->num_i2c_peripherals' is assigned with nonzero value (otherwise the code on 'err_out' is not executed), setting 'i2c_peripherals' to NULL after [1] will cause dereferencing of NULL pointer in chromeos_laptop_destroy() at [2]. [1]: https://elixir.bootlin.com/linux/v5.19/source/drivers/platform/chrome/chromeos_laptop.c#L787 [2]: https://elixir.bootlin.com/linux/v5.19/source/drivers/platform/chrome/chromeos_laptop.c#L860 > After a quick glance, I found an invalid memory access at [2] if > `i2c_peripherals` is NULL (see [3]). After applying the patch, there will be no invalid memory access at [2] if 'i2c_peripherals' is NULL, because in this situation 'cros_laptop->num_i2c_peripherals' is zero and there is no single iteration of the loop. > Or was the double-free issue > discovered by > some static analysis? Yes, it was discovered by SVACE static analysis tool.
On Sat, Aug 20, 2022 at 08:05:13PM +0300, Rustam Subkhankulov wrote: > On Mon, 2022-08-15 at 05:00 +0000, Tzung-Bi Shih wrote: > > Alternatively, I would prefer to fix the double-free by setting > > `i2c_peripherals` to NULL after [1]. > > Since 'cros_laptop->num_i2c_peripherals' is assigned with nonzero value > (otherwise the code on 'err_out' is not executed), setting > 'i2c_peripherals' to NULL after [1] will cause dereferencing of > NULL pointer in chromeos_laptop_destroy() at [2]. > > [1]: > https://elixir.bootlin.com/linux/v5.19/source/drivers/platform/chrome/chromeos_laptop.c#L787 > [2]: > https://elixir.bootlin.com/linux/v5.19/source/drivers/platform/chrome/chromeos_laptop.c#L860 > > > After a quick glance, I found an invalid memory access at [2] if > > `i2c_peripherals` is NULL (see [3]). > > After applying the patch, there will be no invalid memory access at [2] > if 'i2c_peripherals' is NULL, because in this situation > 'cros_laptop->num_i2c_peripherals' is zero and there is no single > iteration of the loop. Yes, we should either reset both cros_laptop->i2c_peripherals and cros_laptop->num_i2c_peripherals on error, or avoid setting them until we are sure that we are not getting an error. I think prefer the latter. Reviewed-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> Thanks.
On Sat, Aug 20, 2022 at 08:05:13PM +0300, Rustam Subkhankulov wrote: > On Mon, 2022-08-15 at 05:00 +0000, Tzung-Bi Shih wrote: > > Alternatively, I would prefer to fix the double-free by setting > > `i2c_peripherals` to NULL after [1]. > > Since 'cros_laptop->num_i2c_peripherals' is assigned with nonzero value > (otherwise the code on 'err_out' is not executed), setting > 'i2c_peripherals' to NULL after [1] will cause dereferencing of > NULL pointer in chromeos_laptop_destroy() at [2]. > > [1]: > https://elixir.bootlin.com/linux/v5.19/source/drivers/platform/chrome/chromeos_laptop.c#L787 > [2]: > https://elixir.bootlin.com/linux/v5.19/source/drivers/platform/chrome/chromeos_laptop.c#L860 > > > After a quick glance, I found an invalid memory access at [2] if > > `i2c_peripherals` is NULL (see [3]). > > After applying the patch, there will be no invalid memory access at [2] > if 'i2c_peripherals' is NULL, because in this situation > 'cros_laptop->num_i2c_peripherals' is zero and there is no single > iteration of the loop. Thanks, I see. I overlooked the `num_i2c_peripherals`.
Hello: This patch was applied to chrome-platform/linux.git (for-kernelci) by Tzung-Bi Shih <tzungbi@kernel.org>: On Sun, 14 Aug 2022 01:08:43 +0300 you wrote: > If chromeos_laptop_prepare_i2c_peripherals() fails after allocating memory > for 'cros_laptop->i2c_peripherals', this memory is freed at 'err_out' label > and nonzero value is returned. Then chromeos_laptop_destroy() is called, > resulting in double-free error. > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > [...] Here is the summary with links: - platform/chrome: fix double-free in chromeos_laptop_prepare() https://git.kernel.org/chrome-platform/c/6ad4194d6a1e You are awesome, thank you!
Hello: This patch was applied to chrome-platform/linux.git (for-next) by Tzung-Bi Shih <tzungbi@kernel.org>: On Sun, 14 Aug 2022 01:08:43 +0300 you wrote: > If chromeos_laptop_prepare_i2c_peripherals() fails after allocating memory > for 'cros_laptop->i2c_peripherals', this memory is freed at 'err_out' label > and nonzero value is returned. Then chromeos_laptop_destroy() is called, > resulting in double-free error. > > Found by Linux Verification Center (linuxtesting.org) with SVACE. > > [...] Here is the summary with links: - platform/chrome: fix double-free in chromeos_laptop_prepare() https://git.kernel.org/chrome-platform/c/6ad4194d6a1e You are awesome, thank you!
diff --git a/drivers/platform/chrome/chromeos_laptop.c b/drivers/platform/chrome/chromeos_laptop.c index 4e14b4d6635d..a2cdbfbaeae6 100644 --- a/drivers/platform/chrome/chromeos_laptop.c +++ b/drivers/platform/chrome/chromeos_laptop.c @@ -740,6 +740,7 @@ static int __init chromeos_laptop_prepare_i2c_peripherals(struct chromeos_laptop *cros_laptop, const struct chromeos_laptop *src) { + struct i2c_peripheral *i2c_peripherals; struct i2c_peripheral *i2c_dev; struct i2c_board_info *info; int i; @@ -748,17 +749,15 @@ chromeos_laptop_prepare_i2c_peripherals(struct chromeos_laptop *cros_laptop, if (!src->num_i2c_peripherals) return 0; - cros_laptop->i2c_peripherals = kmemdup(src->i2c_peripherals, - src->num_i2c_peripherals * - sizeof(*src->i2c_peripherals), - GFP_KERNEL); - if (!cros_laptop->i2c_peripherals) + i2c_peripherals = kmemdup(src->i2c_peripherals, + src->num_i2c_peripherals * + sizeof(*src->i2c_peripherals), + GFP_KERNEL); + if (!i2c_peripherals) return -ENOMEM; - cros_laptop->num_i2c_peripherals = src->num_i2c_peripherals; - - for (i = 0; i < cros_laptop->num_i2c_peripherals; i++) { - i2c_dev = &cros_laptop->i2c_peripherals[i]; + for (i = 0; i < src->num_i2c_peripherals; i++) { + i2c_dev = &i2c_peripherals[i]; info = &i2c_dev->board_info; error = chromeos_laptop_setup_irq(i2c_dev); @@ -775,16 +774,19 @@ chromeos_laptop_prepare_i2c_peripherals(struct chromeos_laptop *cros_laptop, } } + cros_laptop->i2c_peripherals = i2c_peripherals; + cros_laptop->num_i2c_peripherals = src->num_i2c_peripherals; + return 0; err_out: while (--i >= 0) { - i2c_dev = &cros_laptop->i2c_peripherals[i]; + i2c_dev = &i2c_peripherals[i]; info = &i2c_dev->board_info; if (!IS_ERR_OR_NULL(info->fwnode)) fwnode_remove_software_node(info->fwnode); } - kfree(cros_laptop->i2c_peripherals); + kfree(i2c_peripherals); return error; }
If chromeos_laptop_prepare_i2c_peripherals() fails after allocating memory for 'cros_laptop->i2c_peripherals', this memory is freed at 'err_out' label and nonzero value is returned. Then chromeos_laptop_destroy() is called, resulting in double-free error. Found by Linux Verification Center (linuxtesting.org) with SVACE. Signed-off-by: Rustam Subkhankulov <subkhankulov@ispras.ru> Fixes: 5020cd29d8bf ("platform/chrome: chromeos_laptop - supply properties for ACPI devices") --- drivers/platform/chrome/chromeos_laptop.c | 24 ++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-)