[PATCH 0/2] power: supply: sbs-battery: Fix the shared serial number buffer

[PATCH 0/2] power: supply: sbs-battery: Fix the shared serial number buffer

[Babanpreet Singh]

The serial number of every sbs-battery instance is formatted into one
file-scope buffer, so concurrent reads on two batteries can make one
battery report the other's serial number. Patch 1 gives each device its
own buffer.

While rewriting that sprintf() call it became apparent that the
conversion is also the only W=1 warning in this driver: the value is
carried in an int and only its negative half is rejected, so gcc cannot
prove that four hex digits fit the five-byte buffer. Patch 2 casts to
the register's actual width. It is a separate patch because it is a
distinct (and purely cosmetic) change - please just drop it if you would
rather not carry it.

Both patches are compile-tested only; I have no SBS hardware. The race
in patch 1 was derived from the code, not observed on a running system.

Detail on the multi-instance premise for patch 1, since it is the part
that matters: sbs-battery binds per I2C client and sbs-manager registers
one muxed I2C channel per supported battery precisely so that the smart
battery driver can be bound to each of them (MANAGER_SBS Kconfig help
says as much), so more than one instance is a supported configuration
rather than a hypothetical one.

Babanpreet Singh (2):
  power: supply: sbs-battery: Use a per-device serial number buffer
  power: supply: sbs-battery: Bound the serial number conversion to 16
    bits

 drivers/power/supply/sbs-battery.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

-- 
2.43.0

[PATCH 1/2] power: supply: sbs-battery: Use a per-device serial number buffer

[Babanpreet Singh]

sbs_get_battery_serial_number() formats the battery serial number into
sbs_serial[], a single file-scope buffer shared by every sbs-battery
instance, and points val->strval at it.

Nothing restricts this driver to one instance. It binds per I2C client,
and sbs-manager registers one muxed I2C channel per supported battery
specifically so that the smart battery driver can be bound to each of
them, so several sbs-battery instances on one system is a supported
configuration.

The power supply core reads strval after the driver's get_property()
callback has returned: power_supply_show_property() fills a local
union power_supply_propval, then formats it with sysfs_emit(). Two
concurrent POWER_SUPPLY_PROP_SERIAL_NUMBER reads on different batteries
therefore race for the shared buffer - battery B's sprintf() can land
between battery A filling the buffer and the core reading it, and
battery A then reports battery B's serial number.

Move the buffer into struct sbs_info so that each battery formats into
its own storage. It is deliberately not added to the chip->strings[]
array: those entries hold the cached constant strings that
sbs_invalidate_cached_props() clears on presence changes, whereas the
serial number is re-read from its word register on every access.

Fixes: d3ab61ecbab2 ("bq20z75: Add support for more power supply properties")
Assisted-by: Claude:claude-opus-5
Signed-off-by: Babanpreet Singh <bbnpreetsingh@gmail.com>
---
 drivers/power/supply/sbs-battery.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
index 017ec06be766..9bdb6c599c5f 100644
--- a/drivers/power/supply/sbs-battery.c
+++ b/drivers/power/supply/sbs-battery.c
@@ -217,6 +217,7 @@ struct sbs_info {
 	u32				flags;
 	int				technology;
 	char				strings[NR_STRING_BUFFERS][I2C_SMBUS_BLOCK_MAX + 1];
+	char				serial[5];
 };
 
 static char *sbs_get_string_buf(struct sbs_info *chip,
@@ -821,18 +822,18 @@ static int sbs_get_battery_capacity(struct i2c_client *client,
 	return 0;
 }
 
-static char sbs_serial[5];
 static int sbs_get_battery_serial_number(struct i2c_client *client,
 	union power_supply_propval *val)
 {
+	struct sbs_info *chip = i2c_get_clientdata(client);
 	int ret;
 
 	ret = sbs_read_word_data(client, sbs_data[REG_SERIAL_NUMBER].addr);
 	if (ret < 0)
 		return ret;
 
-	sprintf(sbs_serial, "%04x", ret);
-	val->strval = sbs_serial;
+	sprintf(chip->serial, "%04x", ret);
+	val->strval = chip->serial;
 
 	return 0;
 }
-- 
2.43.0

[PATCH 2/2] power: supply: sbs-battery: Bound the serial number conversion to 16 bits

[Babanpreet Singh]

The SBS SerialNumber register is a 16-bit word and chip->serial[] is
sized for its four hex digits plus the NUL terminator. The value is
carried in an int, though, and only the negative half of that range is
rejected before the conversion, so the compiler has to assume
[0, INT_MAX] - up to eight digits:

drivers/power/supply/sbs-battery.c:835:32: warning: '%04x' directive writing between 4 and 8 bytes into a region of size 5 [-Wformat-overflow=]
drivers/power/supply/sbs-battery.c:835:31: note: directive argument in the range [0, 2147483647]
drivers/power/supply/sbs-battery.c:835:9: note: 'sprintf' output between 5 and 9 bytes into a destination of size 5

The overflow is not reachable: sbs_read_word_data() returns the result of
i2c_smbus_read_word_data(), which yields at most 0xffff on success, and
negative returns are rejected just above. Cast to u16 to state the
register width at the point of use, which also lets the compiler prove
the buffer is large enough. No functional change.

This is the only W=1 warning in this driver.

Assisted-by: Claude:claude-opus-5
Signed-off-by: Babanpreet Singh <bbnpreetsingh@gmail.com>
---
 drivers/power/supply/sbs-battery.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
index 9bdb6c599c5f..8f839b2cc353 100644
--- a/drivers/power/supply/sbs-battery.c
+++ b/drivers/power/supply/sbs-battery.c
@@ -832,7 +832,7 @@ static int sbs_get_battery_serial_number(struct i2c_client *client,
 	if (ret < 0)
 		return ret;
 
-	sprintf(chip->serial, "%04x", ret);
+	sprintf(chip->serial, "%04x", (u16)ret);
 	val->strval = chip->serial;
 
 	return 0;
-- 
2.43.0