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 <[email protected]>
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(-)
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;
}
--
2.34.1
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 <[email protected]>
Thanks.
--
Dmitry
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 <[email protected]>:
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!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html
Hello:
This patch was applied to chrome-platform/linux.git (for-next)
by Tzung-Bi Shih <[email protected]>:
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!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html