power: supply: sbs-battery: Fix the shared serial number buffer

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

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.

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

index 017ec06be766..9bdb6c599c5f 100644
@@ -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

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.

---
 drivers/power/supply/sbs-battery.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 9bdb6c599c5f..8f839b2cc353 100644
@@ -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

Applied, thanks!

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

      commit: 6027892925b8d19d2245c2d077e2ae35b49cc2b1
[2/2] power: supply: sbs-battery: Bound the serial number conversion to 16 bits
      commit: 645448becbd0b52216c6741897dd412f9ff1b925

Best regards,

Sebastian Reichel <[email protected]>