Re: [PATCH v2] regulator: da9063: fix null pointer deref with partial DT config

[Benjamin Bara <bbara93@gmail.com>]

Hi,

On Thu, 22 Jun 2023 at 17:11, Martin Fuzzey <martin.fuzzey@flowbird.group> wrote:
> When some of the da9063 regulators do not have corresponding DT nodes a null
> pointer dereference occurs on boot:
> 
> [    1.559034] 8<--- cut here ---
> [    1.564014] Unable to handle kernel NULL pointer dereference at virtual address
> 00000098 when read
> [    1.578055] [00000098] *pgd=00000000
> [    1.593575] Internal error: Oops: 5 [#1] SMP ARM
> [    1.634870] PC is at da9063_regulator_probe+0x35c/0x788
> [    1.647934] LR is at da9063_regulator_probe+0x2e8/0x788
> [    2.073626]  da9063_regulator_probe from platform_probe+0x58/0xb8
> [    2.079759]  platform_probe from really_probe+0xd8/0x3c0
> [    2.085092]  really_probe from __driver_probe_device+0x94/0x1e8
> [    2.091026]  __driver_probe_device from driver_probe_device+0x2c/0xd0
> [    2.097479]  driver_probe_device from __device_attach_driver+0xa4/0x11c
> [    2.104107]  __device_attach_driver from bus_for_each_drv+0x84/0xdc
> [    2.110402]  bus_for_each_drv from __device_attach_async_helper+0xb0/0x110
> [    2.117295]  __device_attach_async_helper from async_run_entry_fn+0x3c/0x158
> [    2.124369]  async_run_entry_fn from process_one_work+0x1d4/0x3e4
> [    2.130485]  process_one_work from worker_thread+0x30/0x520
> [    2.136070]  worker_thread from kthread+0xdc/0xfc
> 
> This is because such regulators have no init_data causing the pointers calculated in
> da9063_check_xvp_constraints() to be invalid.
> 
> Do not dereference them in this case.
> 
> Fixes: b8717a80e6ee ("regulator: da9063: implement setter for voltage monitoring")
> Signed-off-by: Martin Fuzzey <martin.fuzzey@flowbird.group>
> ---
> 
> Changes since V1:
>  -  Following review by Mark Brown avoid previous dereferences too.
>     With the GCC versions I tried this didn't cause problems
>     because it only takes the address ie
>         &config->init_data->constraints
>     doesn't fault if config->init_data is NULL (it would without the &)
>     But this behaviour isn't guaranteed and other compilers or compiler
>     versions could behave differently so completely avoid calling the
>     function if config->init_data is NULL.
> 
>  drivers/regulator/da9063-regulator.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/regulator/da9063-regulator.c b/drivers/regulator/da9063-regulator.c
> index c5dd77be558b..a0621665a6d2 100644
> --- a/drivers/regulator/da9063-regulator.c
> +++ b/drivers/regulator/da9063-regulator.c
> @@ -1028,9 +1028,12 @@ static int da9063_regulator_probe(struct platform_device
> *pdev)
>                         config.of_node = da9063_reg_matches[id].of_node;
>                 config.regmap = da9063->regmap;
> 
> -               ret = da9063_check_xvp_constraints(&config);
> -               if (ret)
> -                       return ret;
> +               /* Checking constraints requires init_data from DT. */
> +               if (config.init_data) {
> +                       ret = da9063_check_xvp_constraints(&config);
> +                       if (ret)
> +                               return ret;
> +               }
> 
>                 regl->rdev = devm_regulator_register(&pdev->dev, &regl->desc,
>                                                      &config);
> --
> 2.25.1

Thank you!

As this is the same as I did in my patch[1], which I tested by removing some
LDO DT nodes, feel free to add:
Tested-by: Benjamin Bara <benjamin.bara@skidata.com>

br,
Benjamin

[1] https://lore.kernel.org/lkml/20230419-dynamic-vmon-v4-1-4d3734e62ada@skidata.com/