linux-kernel - Re: [PATCH] power: supply: max17042_battery: fix some usage of uninitialized variables

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Mon, 14 Oct 2019 05:23:25 +0200
From:   Sebastian Reichel <sre@...nel.org>
To:     Yizhuo <yzhai003@....edu>
Cc:     csong@...ucr.edu, zhiyunq@...ucr.edu, linux-pm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] power: supply: max17042_battery: fix some usage of
 uninitialized variables

Hi,

On Thu, Oct 03, 2019 at 10:29:48AM -0700, Yizhuo wrote:
> Several functions in this file are trying to use regmap_read() to
> initialize the specific variable, however, if regmap_read() fails,
> the variable could be uninitialized but used directly, which is
> potentially unsafe. The return value of regmap_read() should be
> checked and handled. The same case also happens in function
> max17042_thread_handler() but it needs more effort to patch.
> 
> Signed-off-by: Yizhuo <yzhai003@....edu>
> ---

This does not improve the situation much. You should change the
functions from 'void' to 'int' return code and propagate the error
code, otherwise the following code assumes everything to be correct.
Also the Signed-off-by name and the patch author's name seems to be
incomplete.

Thanks,

-- Sebastian

>  drivers/power/supply/max17042_battery.c | 23 +++++++++++++++++++----
>  1 file changed, 19 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/power/supply/max17042_battery.c b/drivers/power/supply/max17042_battery.c
> index e6a2dacaa730..b897776a2749 100644
> --- a/drivers/power/supply/max17042_battery.c
> +++ b/drivers/power/supply/max17042_battery.c
> @@ -675,8 +675,12 @@ static void max17042_reset_vfsoc0_reg(struct max17042_chip *chip)
>  {
>  	unsigned int vfSoc;
>  	struct regmap *map = chip->regmap;
> +	int ret;
> +
> +	ret = regmap_read(map, MAX17042_VFSOC, &vfSoc);
> +	if (ret)
> +		return;
>  
> -	regmap_read(map, MAX17042_VFSOC, &vfSoc);
>  	regmap_write(map, MAX17042_VFSOC0Enable, VFSOC0_UNLOCK);
>  	max17042_write_verify_reg(map, MAX17042_VFSOC0, vfSoc);
>  	regmap_write(map, MAX17042_VFSOC0Enable, VFSOC0_LOCK);
> @@ -686,12 +690,18 @@ static void max17042_load_new_capacity_params(struct max17042_chip *chip)
>  {
>  	u32 full_cap0, rep_cap, dq_acc, vfSoc;
>  	u32 rem_cap;
> +	int ret;
>  
>  	struct max17042_config_data *config = chip->pdata->config_data;
>  	struct regmap *map = chip->regmap;
>  
> -	regmap_read(map, MAX17042_FullCAP0, &full_cap0);
> -	regmap_read(map, MAX17042_VFSOC, &vfSoc);
> +	ret = regmap_read(map, MAX17042_FullCAP0, &full_cap0);
> +	if (ret)
> +		return;
> +
> +	ret = regmap_read(map, MAX17042_VFSOC, &vfSoc);
> +	if (ret)
> +		return;
>  
>  	/* fg_vfSoc needs to shifted by 8 bits to get the
>  	 * perc in 1% accuracy, to get the right rem_cap multiply
> @@ -1108,7 +1118,12 @@ static int max17042_probe(struct i2c_client *client,
>  	if (!client->irq)
>  		regmap_write(chip->regmap, MAX17042_SALRT_Th, 0xff00);
>  
> -	regmap_read(chip->regmap, MAX17042_STATUS, &val);
> +	ret = regmap_read(chip->regmap, MAX17042_STATUS, &val);
> +	if (ret) {
> +		dev_err(&client->dev, "Failed to get MAX17042_STATUS.\n");
> +		return ret;
> +	}
> +
>  	if (val & STATUS_POR_BIT) {
>  		INIT_WORK(&chip->work, max17042_init_worker);
>  		ret = devm_add_action(&client->dev, max17042_stop_work, chip);
> -- 
> 2.17.1
> 

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)