Re: [PATCH V2 2/6] power: supply: sgm41542: Add SG Micro sgm41542 charger

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [Critical] Uninitialized pointer dereference causes kernel panic on missing battery info
- [High] Missing `.remove` callback leads to delayed work timer Use-After-Free
- [High] Race condition exposing uninitialized structures to userspace
- [High] Input current limit configuration truncates 3.8A to 600mA
- [High] Input voltage limit calculation wrap-around incorrectly applies low voltage bounds
- [High] Charge voltage calculation maps 4.336V request to 4.352V, risking battery overcharge
- [Medium] Watchdog timer logic applies incorrect bitmask instead of integer duration
- [Low] Ignored devm_regulator_register return value masks initialization failures
--

commit 974520d45c64c2d48e4b5e84a0082620a313d9d4
Author: Chris Morgan <macromorgan@hotmail.com>

power: supply: sgm41542: Add SG Micro sgm41542 charger

Add support for the SG Micro SGM41542 charger/boost converter.
Driver was adapted from Rockchip BSP driver and confirmed
with vendor datasheet.

Link: https://github.com/rockchip-linux/kernel/blob/develop-6.6/drivers/power/supply/sgm41542_charger.c
Link: https://www.sg-micro.de/rect/assets/1e8de70b-657e-4156-be68-a64fdbe8e418/SGM41541_SGM41542.pdf

> diff --git a/drivers/power/supply/sgm41542_charger.c b/drivers/power/supply/sgm41542_charger.c
> new file mode 100644
> index 0000000000000..1bc2eab554065
> --- /dev/null
> +++ b/drivers/power/supply/sgm41542_charger.c

[ ... ]

> +static int sgm4154x_set_chrg_volt(struct sgm4154x_device *sgm, int chrg_volt)
> +{
> +	int reg_val;
> +	int ret;
> +
> +	/*
> +	 * Note that the value of 0x01111 represents a "special value"
> +	 * corresponding to 4352000uV instead of the expected 4336000uV,
> +	 * per the datasheet. All other values are as expected.
> +	 */
> +	chrg_volt = clamp(chrg_volt, SGM4154X_VREG_V_MIN_UV, sgm->init_data.max_vreg);
> +	reg_val = (chrg_volt - SGM4154X_VREG_V_MIN_UV) / SGM4154X_VREG_V_STEP_UV;
> +	reg_val = reg_val << 3;

Could this apply a charge voltage higher than requested?

If chrg_volt is exactly 4336000, reg_val becomes 15 (0x0f). According to the
comment and sgm4154x_get_chrg_volt(), 0x0f maps to 4352000uV. Might this
overcharge the battery by 16mV?

[ ... ]

> +static int sgm4154x_set_input_volt_lim(struct sgm4154x_device *sgm,
> +				       unsigned int vindpm)
> +{
> +	enum SGM4154X_VINDPM_OS os_val;
> +	unsigned int offset;
> +	u8 reg_val;
> +	int ret;
> +
> +
> +	if (vindpm < SGM4154X_VINDPM_V_MIN_UV ||
> +	    vindpm > SGM4154X_VINDPM_V_MAX_UV) {
> +		dev_err(sgm->dev, "input voltage limit %u outside range\n", vindpm);
> +		return -EINVAL;
> +	}
> +
> +	if (vindpm < 5900000) {
> +		os_val = VINDPM_OS_3900MV;
> +		offset = 3900000;
> +	} else if (vindpm >= 5900000 && vindpm < 7500000) {
> +		os_val = VINDPM_OS_5900MV;
> +		offset = 5900000;
> +	} else if (vindpm >= 7500000 && vindpm < 10500000) {
> +		os_val = VINDPM_OS_7500MV;
> +		offset = 7500000;
> +	} else {
> +		os_val = VINDPM_OS_10500MV;
> +		offset = 10500000;
> +	}
> +
> +	ret = regmap_update_bits(sgm->regmap,
> +				 SGM4154X_CHRG_CTRL_F,
> +				 SGM4154X_VINDPM_OS_MASK,
> +				 os_val);
> +	if (ret) {
> +		dev_err(sgm->dev, "set vin dpm error!\n");
> +		return ret;
> +	}
> +
> +	reg_val = (vindpm - offset) / SGM4154X_VINDPM_STEP_UV;
> +
> +	ret = regmap_update_bits(sgm->regmap, SGM4154X_CHRG_CTRL_6,
> +				 SGM4154X_VINDPM_V_MASK, reg_val);

Is it possible for reg_val to wrap around the register mask here?

If vindpm is 5800000, it falls into the first range (vindpm < 5900000). The
calculation yields a reg_val of 19 (0x13). Since SGM4154X_VINDPM_V_MASK is
only 4 bits (GENMASK(3, 0)), updating the bits will truncate it to 3. This
would set the limit to 4.2V instead of clamping it to the highest supported
value in this range.

[ ... ]

> +static int sgm4154x_set_input_curr_lim(struct sgm4154x_device *sgm, int iindpm)
> +{
> +	int reg_val;
> +	int ret;
> +
> +	if (iindpm < SGM4154X_IINDPM_I_MIN_UA)
> +		return -EINVAL;
> +
> +	/*
> +	 * Per the datasheet, values between 100000uA and 3100000uA work
> +	 * as expected with the register defined as having a step of
> +	 * 100000 and a min/max of 100000 (0x00) through 3100000 (0x1e).
> +	 * The register value of 0x1f however corresponds to 3800000uA not
> +	 * 3200000uA as one would expect.
> +	 */
> +
> +	if (iindpm > 3100000 && iindpm < SGM4154X_IINDPM_I_MAX_UA)
> +		iindpm = 3100000;
> +	if ((iindpm > SGM4154X_IINDPM_I_MAX_UA) || (iindpm > sgm->init_data.ilim))
> +		iindpm = min(SGM4154X_IINDPM_I_MAX_UA, sgm->init_data.ilim);
> +
> +	reg_val = (iindpm - SGM4154X_IINDPM_I_MIN_UA) / SGM4154X_IINDPM_STEP_UA;
> +
> +	ret = regmap_update_bits(sgm->regmap,
> +				 SGM4154X_CHRG_CTRL_0,
> +				 SGM4154X_IINDPM_I_MASK,
> +				 reg_val);

Could this calculation truncate for the 3.8A setting?

If iindpm is exactly 3800000, reg_val evaluates to 37 (0x25). Since
SGM4154X_IINDPM_I_MASK is GENMASK(4, 0) (0x1f), this truncates to 0x05.
Might this configure the charger to 600mA instead of the intended 3.8A?

[ ... ]

> +static int sgm4154x_set_watchdog_timer(struct sgm4154x_device *sgm, int time)
> +{
> +	u8 reg_val;
> +	int ret;
> +
> +	if (time == 0)
> +		reg_val = SGM4154X_WDT_TIMER_DISABLE;
> +	else if (time == 40)
> +		reg_val = SGM4154X_WDT_TIMER_40S;
> +	else if (time == 80)
> +		reg_val = SGM4154X_WDT_TIMER_80S;
> +	else
> +		reg_val = SGM4154X_WDT_TIMER_160S;

[ ... ]

> +static int sgm4154x_charger_set_property(struct power_supply *psy,
> +					 enum power_supply_property prop,
> +					 const union power_supply_propval *val)
> +{
> +	struct sgm4154x_device *sgm = power_supply_get_drvdata(psy);
> +	int ret = -EINVAL;
> +
> +	switch (prop) {
> +	case POWER_SUPPLY_PROP_ONLINE:
> +		if (val->intval) {
> +			ret = sgm4154x_enable_charger(sgm);
> +			sgm4154x_set_watchdog_timer(sgm, SGM4154X_WDT_TIMER_40S);

Will this pass the correct duration to sgm4154x_set_watchdog_timer()?

SGM4154X_WDT_TIMER_40S is defined as a bitmask (BIT(4), which is 16).
Since the check inside sgm4154x_set_watchdog_timer() looks for exactly 40,
might passing 16 fall through to the else branch and unintentionally set
a 160-second watchdog?

[ ... ]

> +static int sgm4154x_hw_init(struct sgm4154x_device *sgm)
> +{
> +	struct power_supply_battery_info *bat_info;
> +	int ret;
> +	u32 val;
> +
> +	/*
> +	 * If unable to read devicetree info, use default/reset
> +	 * values from hardware.
> +	 */
> +	sgm->init_data.iprechg = SGM4154X_PRECHRG_I_DEF_UA;
> +	sgm->init_data.iterm = SGM4154X_TERMCHRG_I_DEF_UA;
> +	sgm->init_data.max_ichg = SGM4154X_ICHRG_I_DEF_UA;
> +	sgm->init_data.max_vreg = SGM4154X_VREG_V_DEF_UV;
> +	sgm->init_data.vlim = SGM4154X_VINDPM_DEF_UV;
> +	sgm->init_data.ilim = SGM4154X_IINDPM_DEF_UA;
> +
> +	ret = power_supply_get_battery_info(sgm->charger, &bat_info);
> +	if (ret)
> +		dev_warn(sgm->dev, "sgm4154x: cannot read battery info\n");
> +	else {
> +		if (bat_info->constant_charge_current_max_ua)
> +			sgm->init_data.max_ichg = bat_info->constant_charge_current_max_ua;
> +		if (bat_info->constant_charge_voltage_max_uv)
> +			sgm->init_data.max_vreg = bat_info->constant_charge_voltage_max_uv;
> +		if (bat_info->charge_term_current_ua)
> +			sgm->init_data.iterm = bat_info->charge_term_current_ua;
> +		if (bat_info->precharge_current_ua)
> +			sgm->init_data.iprechg = bat_info->precharge_current_ua;
> +	}
> +
> +	power_supply_put_battery_info(sgm->charger, bat_info);

Does this code dereference an uninitialized pointer?

If power_supply_get_battery_info() returns an error, bat_info remains
uninitialized. Calling power_supply_put_battery_info() with that pointer
outside of the success block will try to free its internal tables using
garbage data.

[ ... ]

> +static int sgm4154x_probe(struct i2c_client *client)
> +{
> +	const struct i2c_device_id *id = i2c_client_get_device_id(client);
> +	struct device *dev = &client->dev;
> +	struct sgm4154x_device *sgm;
> +	int ret;
> +
> +	sgm = devm_kzalloc(dev, sizeof(*sgm), GFP_KERNEL);
> +	if (!sgm)
> +		return -ENOMEM;
> +
> +	sgm->client = client;
> +	sgm->dev = dev;
> +
> +	strscpy(sgm->model_name, id->name, I2C_NAME_SIZE);
> +
> +	sgm->regmap = devm_regmap_init_i2c(client, &sgm4154x_regmap_config);
> +	if (IS_ERR(sgm->regmap))
> +		return dev_err_probe(dev, PTR_ERR(sgm->regmap),
> +				     "Failed to allocate register map\n");
> +
> +	i2c_set_clientdata(client, sgm);
> +
> +	ret = sgm4154x_hw_chipid_detect(sgm);
> +	if (ret)
> +		dev_err_probe(dev, ret, "Unable to read HW ID\n");
> +
> +	device_init_wakeup(dev, 1);
> +
> +	ret = sgm4154x_power_supply_init(sgm, dev);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Failed to register power supply\n");
> +
> +	ret = sgm4154x_hw_init(sgm);
> +	if (ret)
> +		return dev_err_probe(dev, ret, "Cannot initialize the chip.\n");
> +
> +	if (client->irq) {
> +		ret = devm_request_threaded_irq(dev, client->irq, NULL,
> +						sgm4154x_irq_handler_thread,
> +						IRQF_TRIGGER_FALLING |
> +						IRQF_ONESHOT,
> +						"sgm41542-irq", sgm);
> +		if (ret)
> +			return ret;
> +		enable_irq_wake(client->irq);
> +	}
> +
> +	sgm->sgm_monitor_wq = devm_alloc_ordered_workqueue(dev, "%s",
> +			WQ_MEM_RECLAIM | WQ_FREEZABLE, "sgm-monitor-wq");

Could this result in a race condition exposing uninitialized structures?

sgm4154x_power_supply_init() makes the sysfs interface available before the
workqueue or sgm_delay_work are initialized here. If userspace writes to the
ONLINE property in this small window, it triggers sgm4154x_set_watchdog_timer()
which queues the uninitialized work to a NULL workqueue.

[ ... ]

> +	sgm4154x_vbus_regulator_register(sgm);
> +
> +	return 0;
> +}

Does this silently ignore regulator registration failures?

If sgm4154x_vbus_regulator_register() fails and returns an error code
(like -EPROBE_DEFER), the return value is dropped and the function returns 0.

[ ... ]

> +static struct i2c_driver sgm4154x_driver = {
> +	.driver = {
> +		.name = "sgm4154x-charger",
> +		.of_match_table = sgm4154x_of_match,
> +	},
> +	.probe = sgm4154x_probe,
> +	.shutdown = sgm4154x_charger_shutdown,
> +	.id_table = sgm4154x_i2c_ids,
> +};

Is it possible for the delayed work timer to cause a Use-After-Free?

Since there is no .remove callback and the driver uses devm allocation for
struct sgm4154x_device and the workqueue, devres will automatically free
them when the module unbinds. However, the driver does not explicitly cancel
the delayed work item (sgm->sgm_delay_work). If the timer fires after the
driver structures are freed, it will dereference freed memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515221947.299229-1-macroalpha82@gmail.com?part=2