[PATCH 0/5] hwmon: (pmbus/adm1266) buffer-bound and timestamp fixes

[PATCH 0/5] hwmon: (pmbus/adm1266) buffer-bound and timestamp fixes

[Abdurrahman Hussain]

This series fixes five pre-existing bugs in adm1266.c that were
surfaced by automated review of an in-flight feature series for the
same driver [1].  None of them are introduced by that feature work --
they are all reachable on the existing driver as it sits in mainline.
Sending them standalone first, with Fixes: tags and Cc: stable, so
the feature respin (v5) can rebase on top.

Patch 1 fixes a CLOCK_MONOTONIC vs CLOCK_REALTIME confusion in
adm1266_set_rtc(): the chip's SET_RTC register is documented to hold
wall-clock seconds, but the driver currently seeds it from
ktime_get_seconds(), giving blackbox records timestamps that reset
to small values on every host reboot.

Patches 2 and 3 fix two ways the blackbox-info path can be driven
out of bounds by a misbehaving slave: a 5-byte stack buffer that
i2c_smbus_read_block_data() will memcpy() up to 32 bytes into, and
a record_count loop bound taken directly from the device with no
upper clamp against the 32-record dev_mem allocation.

Patches 4 and 5 fix the two ways adm1266_pmbus_block_xfer() can
write past the end of a buffer: an off-by-one on the helper's own
read_buf (sized for the length+payload but missing the PEC byte the
i2c_msg length already accounts for), and a caller-side bug where
adm1266_nvmem_read_blackbox() advances its destination pointer in
64-byte strides while the helper is willing to write up to 255
bytes per call.

[1] https://lore.kernel.org/r/20260512-adm1266-v3-0-a81a479b0bb0@nexthop.ai

Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
Abdurrahman Hussain (5):
      hwmon: (pmbus/adm1266) seed timestamp from the real-time clock
      hwmon: (pmbus/adm1266) widen blackbox-info buffer to I2C_SMBUS_BLOCK_MAX
      hwmon: (pmbus/adm1266) reject implausible blackbox record_count
      hwmon: (pmbus/adm1266) include PEC byte in pmbus_block_xfer read buffer
      hwmon: (pmbus/adm1266) bounce blackbox records through a protocol-sized buffer

 drivers/hwmon/pmbus/adm1266.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)
---
base-commit: 1f63dd8ca0dc05a8272bb8155f643c691d29bb11
change-id: 20260514-adm1266-fixes-853003a0fad4

Best regards,
--  
Abdurrahman Hussain <abdurrahman@nexthop.ai>

[PATCH 1/5] hwmon: (pmbus/adm1266) seed timestamp from the real-time clock

[Abdurrahman Hussain]

adm1266_set_rtc() seeds the chip's SET_RTC register from
ktime_get_seconds(), which returns CLOCK_MONOTONIC -- i.e. seconds
since the host last booted, not seconds since the Unix epoch.

The chip stamps that value into every blackbox record it captures.
Userspace reading those timestamps back expects wall-clock seconds:
that's what the SET_RTC frame layout documents (datasheet Rev. D,
Table 84) and what every other consumer of "seconds since epoch"
assumes.  Seeding from CLOCK_MONOTONIC gives blackbox records a
timestamp that is only meaningful within a single boot of the host
and silently resets to small values on every reboot.

Switch to ktime_get_real_seconds() so the seed matches what the
register is documented to hold.

Fixes: 15609d189302 ("hwmon: (pmbus/adm1266) read blackbox")
Cc: stable@vger.kernel.org
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
 drivers/hwmon/pmbus/adm1266.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
index d90f8f80be8e..a86666c73a5e 100644
--- a/drivers/hwmon/pmbus/adm1266.c
+++ b/drivers/hwmon/pmbus/adm1266.c
@@ -432,7 +432,7 @@ static int adm1266_set_rtc(struct adm1266_data *data)
 	char write_buf[6];
 	int i;
 
-	kt = ktime_get_seconds();
+	kt = ktime_get_real_seconds();
 
 	memset(write_buf, 0, sizeof(write_buf));
 

-- 
2.53.0

Re: [PATCH 1/5] hwmon: (pmbus/adm1266) seed timestamp from the real-time clock

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Fetching wall-clock time during device probe yields an uninitialized value, defeating the purpose of the patch.
- [Medium] Switching from monotonic time to wall-clock time introduces a Year 2038 silent truncation bug.
--

commit a6888c73c1a25c1329fffa916ee8e1654b5eaa1a
Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>

hwmon: (pmbus/adm1266) seed timestamp from the real-time clock

This commit switches the adm1266 driver to use ktime_get_real_seconds()
instead of ktime_get_seconds() to seed the chip's SET_RTC register.
The intention is to provide wall-clock time for blackbox records.

> diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
> index d90f8f80be8e0..a86666c73a5e9 100644
> --- a/drivers/hwmon/pmbus/adm1266.c
> +++ b/drivers/hwmon/pmbus/adm1266.c
> @@ -432,7 +432,7 @@ static int adm1266_set_rtc(struct adm1266_data *data)
>  	char write_buf[6];
>  	int i;
>  
> -	kt = ktime_get_seconds();
> +	kt = ktime_get_real_seconds();

Since adm1266_set_rtc() is called during device probe, will the wall-clock
time be initialized at this point?

During early boot, before the system timekeeper is synchronized with a
hardware RTC or NTP, ktime_get_real_seconds() might return a near-zero
value. Would this effectively behave like a monotonic timestamp and
silently defeat the purpose of the patch?

Additionally, does the 4-byte serialization loop in this function silently
truncate the 64-bit kt value?

drivers/hwmon/pmbus/adm1266.c:adm1266_set_rtc() {
    ...
    for (i = 0; i < 4; i++)
        write_buf[2 + i] = (kt >> (i * 8)) & 0xFF;
    ...
}

Because kt is now a wall-clock timestamp instead of system uptime, a 4-byte
field will overflow in the year 2038.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515-adm1266-fixes-v1-0-1c1ea1349cfe@nexthop.ai?part=1

[PATCH 2/5] hwmon: (pmbus/adm1266) widen blackbox-info buffer to I2C_SMBUS_BLOCK_MAX

[Abdurrahman Hussain]

adm1266_nvmem_read_blackbox() declares a 5-byte stack buffer and
passes it to i2c_smbus_read_block_data() to retrieve the 4-byte
BLACKBOX_INFO response.  i2c_smbus_read_block_data() does not honour
caller buffer sizes -- it memcpy()s data.block[0] bytes from the
SMBus transaction (where data.block[0] is the length byte returned by
the slave device, up to I2C_SMBUS_BLOCK_MAX = 32):

	memcpy(values, &data.block[1], data.block[0]);

If the device returns any block length above 5, the call overflows
the caller's 5-byte stack buffer before the post-call

	if (ret != 4)
		return -EIO;

check has a chance to reject the response.

Widen the local buffer to I2C_SMBUS_BLOCK_MAX so the helper has room
for any well-formed SMBus block response, matching the convention used
by the other i2c_smbus_read_block_data() callers in this driver.

Fixes: 15609d189302 ("hwmon: (pmbus/adm1266) read blackbox")
Cc: stable@vger.kernel.org
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
 drivers/hwmon/pmbus/adm1266.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
index a86666c73a5e..94691dec1359 100644
--- a/drivers/hwmon/pmbus/adm1266.c
+++ b/drivers/hwmon/pmbus/adm1266.c
@@ -349,7 +349,7 @@ static int adm1266_nvmem_read_blackbox(struct adm1266_data *data, u8 *read_buff)
 {
 	int record_count;
 	char index;
-	u8 buf[5];
+	u8 buf[I2C_SMBUS_BLOCK_MAX];
 	int ret;
 
 	ret = i2c_smbus_read_block_data(data->client, ADM1266_BLACKBOX_INFO, buf);

-- 
2.53.0

Re: [PATCH 2/5] hwmon: (pmbus/adm1266) widen blackbox-info buffer to I2C_SMBUS_BLOCK_MAX

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unsynchronized concurrent access to the shared `data->dev_mem` buffer in NVMEM read callback violates subsystem locking guidelines.
--

commit ad17d73bcc6d34ca2a117a7fde443e8a2ce1dda7
Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>
Subject: hwmon: (pmbus/adm1266) widen blackbox-info buffer to I2C_SMBUS_BLOCK_MAX

adm1266_nvmem_read_blackbox() declares a 5-byte stack buffer and passes it
to i2c_smbus_read_block_data(). This patch widens the local buffer to
I2C_SMBUS_BLOCK_MAX so the helper has room for any well-formed SMBus block
response, avoiding stack buffer overflow. Note that other buffer overflow
issues are fixed in subsequent commits in this series.

> diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
> index a86666c73a5e9..94691dec13599 100644
> --- a/drivers/hwmon/pmbus/adm1266.c
> +++ b/drivers/hwmon/pmbus/adm1266.c
> @@ -349,7 +349,7 @@ static int adm1266_nvmem_read_blackbox(struct adm1266_data *data, u8 *read_buff)

This isn't a regression introduced by this patch, but does the nvmem read
callback that populates read_buff need locking?

The hardware monitoring subsystem guidelines state that drivers must implement
locking for attributes registered by other means (like nvmem) and should use
hwmon_lock() and hwmon_unlock().

Since the nvmem core does not serialize reg_read operations, can concurrent
reads from userspace via sysfs cause data races?

For example, in adm1266_nvmem_read():

	if (offset == 0) {
		memset(data->dev_mem, 0, data->nvmem_config.size);

		ret = adm1266_nvmem_read_blackbox(data, data->dev_mem);
	...
	}
	memcpy(val, data->dev_mem + offset, bytes);

If one thread executes memset() or multi-block I2C reads via
adm1266_nvmem_read_blackbox() while another thread copies partially written
data via memcpy(), could this result in memory corruption of the shared buffer
and incorrectly interlaced I2C hardware interactions?

>  {
>  	int record_count;
>  	char index;
> -	u8 buf[5];
> +	u8 buf[I2C_SMBUS_BLOCK_MAX];
>  	int ret;
>  
>  	ret = i2c_smbus_read_block_data(data->client, ADM1266_BLACKBOX_INFO, buf);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515-adm1266-fixes-v1-0-1c1ea1349cfe@nexthop.ai?part=2

[PATCH 3/5] hwmon: (pmbus/adm1266) reject implausible blackbox record_count

[Abdurrahman Hussain]

adm1266_nvmem_read_blackbox() loops over a record_count that comes
straight from byte 3 of the BLACKBOX_INFO response.  The destination
buffer is data->dev_mem, sized for the nvmem cell's declared 2048
bytes (ADM1266_BLACKBOX_MAX_RECORDS * ADM1266_BLACKBOX_SIZE = 32 * 64).
A device that reports a record_count greater than 32 -- whether due
to firmware bugs, bus corruption, or a non-responsive slave returning
0xff -- would walk read_buff past the end of the dev_mem allocation
on the trailing iterations.

Cap record_count at ADM1266_BLACKBOX_MAX_RECORDS (introduced here)
before entering the loop and return -EIO on any larger value, so a
malformed BLACKBOX_INFO response cannot drive the loop out of bounds.

Fixes: 15609d189302 ("hwmon: (pmbus/adm1266) read blackbox")
Cc: stable@vger.kernel.org
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
 drivers/hwmon/pmbus/adm1266.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
index 94691dec1359..43d9e7407795 100644
--- a/drivers/hwmon/pmbus/adm1266.c
+++ b/drivers/hwmon/pmbus/adm1266.c
@@ -46,6 +46,7 @@
 
 #define ADM1266_BLACKBOX_OFFSET		0
 #define ADM1266_BLACKBOX_SIZE		64
+#define ADM1266_BLACKBOX_MAX_RECORDS	32
 
 #define ADM1266_PMBUS_BLOCK_MAX		255
 
@@ -360,6 +361,8 @@ static int adm1266_nvmem_read_blackbox(struct adm1266_data *data, u8 *read_buff)
 		return -EIO;
 
 	record_count = buf[3];
+	if (record_count > ADM1266_BLACKBOX_MAX_RECORDS)
+		return -EIO;
 
 	for (index = 0; index < record_count; index++) {
 		ret = adm1266_pmbus_block_xfer(data, ADM1266_READ_BLACKBOX, 1, &index, read_buff);

-- 
2.53.0

Re: [PATCH 3/5] hwmon: (pmbus/adm1266) reject implausible blackbox record_count

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The driver fails to use `hwmon_lock()` to serialize access to the nvmem cell, violating hardware monitoring subsystem architectural rules.
--

commit 82b651c5cd300cad751f056cdcdf20bb2cbb18c7
Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>

hwmon: (pmbus/adm1266) reject implausible blackbox record_count

This commit adds a bounds check to adm1266_nvmem_read_blackbox() to ensure
the record_count does not exceed the maximum capacity of the device memory
buffer. It caps the record count at 32 and returns an error if exceeded.

> diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
> index 94691dec13599..43d9e74077954 100644
> --- a/drivers/hwmon/pmbus/adm1266.c
> +++ b/drivers/hwmon/pmbus/adm1266.c
[ ... ]
> @@ -360,6 +361,8 @@ static int adm1266_nvmem_read_blackbox(struct adm1266_data *data, u8 *read_buff)
>  		return -EIO;
>  
>  	record_count = buf[3];
> +	if (record_count > ADM1266_BLACKBOX_MAX_RECORDS)
> +		return -EIO;
>  
>  	for (index = 0; index < record_count; index++) {
>  		ret = adm1266_pmbus_block_xfer(data, ADM1266_READ_BLACKBOX, 1, &index, read_buff);

This isn't a bug introduced by this patch, but while reviewing the blackbox
read path, is there a missing lock to serialize access to the nvmem cell?

According to the hardware monitoring subsystem guidelines, drivers must
implement locking for attributes registered outside the hwmon core, and should
use hwmon_lock() and hwmon_unlock() for this purpose.

If multiple user space threads concurrently access the nvmem attribute, could
this lead to a race condition where one thread clears or populates the shared
buffer while another thread copies that partially modified buffer to userspace?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515-adm1266-fixes-v1-0-1c1ea1349cfe@nexthop.ai?part=3

[PATCH 4/5] hwmon: (pmbus/adm1266) include PEC byte in pmbus_block_xfer read buffer

[Abdurrahman Hussain]

adm1266_pmbus_block_xfer() sets up the read transaction with

	.buf = data->read_buf,
	.len = ADM1266_PMBUS_BLOCK_MAX + 2,

but read_buf in struct adm1266_data is declared as

	u8 read_buf[ADM1266_PMBUS_BLOCK_MAX + 1];

For a max-length block response (length byte = 255 + up to 1 PEC
byte), the i2c controller is told to write 257 bytes into a 256-byte
buffer, putting one byte past the end of read_buf.  The same response
also makes the subsequent PEC compare

	if (crc != msgs[1].buf[msgs[1].buf[0] + 1])

read a byte beyond the array.

Bump the read_buf declaration to ADM1266_PMBUS_BLOCK_MAX + 2 so the
buffer can hold the length byte, up to 255 payload bytes, and the PEC
byte the i2c_msg length already accounts for.

Fixes: 407dc802a9c0 ("hwmon: (pmbus/adm1266) Add Block process call")
Cc: stable@vger.kernel.org
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
 drivers/hwmon/pmbus/adm1266.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
index 43d9e7407795..5c68e3177f64 100644
--- a/drivers/hwmon/pmbus/adm1266.c
+++ b/drivers/hwmon/pmbus/adm1266.c
@@ -61,7 +61,7 @@ struct adm1266_data {
 	u8 *dev_mem;
 	struct mutex buf_mutex;
 	u8 write_buf[ADM1266_PMBUS_BLOCK_MAX + 1] ____cacheline_aligned;
-	u8 read_buf[ADM1266_PMBUS_BLOCK_MAX + 1] ____cacheline_aligned;
+	u8 read_buf[ADM1266_PMBUS_BLOCK_MAX + 2] ____cacheline_aligned;
 };
 
 static const struct nvmem_cell_info adm1266_nvmem_cells[] = {

-- 
2.53.0

Re: [PATCH 4/5] hwmon: (pmbus/adm1266) include PEC byte in pmbus_block_xfer read buffer

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Out-of-bounds memory accesses in `adm1266_gpio_get_multiple` due to incorrect macro usage.
- [High] Data race and subsystem guideline violation in `adm1266_nvmem_read` due to lack of locking.
--

commit 8a595dba5a711f725b08f3b2d988dc3dd5b53c2e
Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>

hwmon: (pmbus/adm1266) include PEC byte in pmbus_block_xfer read buffer

This commit increases the size of the read buffer in the adm1266 driver by
one byte. This allows it to hold the length byte, up to 255 payload bytes,
and the PEC byte, preventing an out-of-bounds write and a subsequent
out-of-bounds read during PEC comparison.

> diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
> index 43d9e74077954..5c68e3177f64e 100644
> --- a/drivers/hwmon/pmbus/adm1266.c
> +++ b/drivers/hwmon/pmbus/adm1266.c
> @@ -61,7 +61,7 @@ struct adm1266_data {
>  	u8 *dev_mem;
>  	struct mutex buf_mutex;
>  	u8 write_buf[ADM1266_PMBUS_BLOCK_MAX + 1] ____cacheline_aligned;
> -	u8 read_buf[ADM1266_PMBUS_BLOCK_MAX + 1] ____cacheline_aligned;
> +	u8 read_buf[ADM1266_PMBUS_BLOCK_MAX + 2] ____cacheline_aligned;
>  };
>  
>  static const struct nvmem_cell_info adm1266_nvmem_cells[] = {

While reviewing this code, I noticed a couple of other issues in the file.
This isn't a bug introduced by this patch, but does the loop in
adm1266_gpio_get_multiple() read past the end of the mask array?

drivers/hwmon/pmbus/adm1266.c:adm1266_gpio_get_multiple() {
...
	*bits = 0;
	for_each_set_bit_from(gpio_nr, mask, ADM1266_GPIO_NR + ADM1266_PDIO_STATUS) {
		if (test_bit(gpio_nr - ADM1266_GPIO_NR, &status))
			set_bit(gpio_nr, bits);
	}
...
}

Because ADM1266_PDIO_STATUS is a PMBus command constant (0xE9 or 233), the
size evaluated is 242 instead of the expected ADM1266_GPIO_NR +
ADM1266_PDIO_NR (25).

Since the mask and bits arrays supplied by gpiolib are sized only for ngpio
(25 bits), find_next_bit() will read up to 242 bits from mask.

If any of that memory has a bit set, could the loop execute
set_bit(gpio_nr, bits) and overflow the bits array?

Additionally, this isn't a bug introduced by this patch, but is there a
missing lock in adm1266_nvmem_read() that could cause a data race on
data->dev_mem?

drivers/hwmon/pmbus/adm1266.c:adm1266_nvmem_read() {
...
	if (offset == 0) {
		memset(data->dev_mem, 0, data->nvmem_config.size);

		ret = adm1266_nvmem_read_blackbox(data, data->dev_mem);
...
}

The hwmon subsystem guidelines require drivers to implement locking for
attributes registered by other means, such as the NVMEM core. However,
adm1266_nvmem_read() accesses and mutates the shared data->dev_mem buffer
without acquiring any driver-level lock.

When called with offset 0, it zeroes out the memory via memset() and then
repopulates it via multiple slow I2C reads.

Because the NVMEM subsystem does not serialize reg_read operations, will
concurrent reads via the sysfs NVMEM interface result in multiple threads
interleaving their memset operations and I2C block transactions, returning
corrupted or partially-populated data to userspace?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515-adm1266-fixes-v1-0-1c1ea1349cfe@nexthop.ai?part=4

[PATCH 5/5] hwmon: (pmbus/adm1266) bounce blackbox records through a protocol-sized buffer

[Abdurrahman Hussain]

adm1266_pmbus_block_xfer() copies the device-supplied block payload
into the caller-provided buffer using the device-supplied length:

	memcpy(data_r, &msgs[1].buf[1], msgs[1].buf[0]);

The helper does not know how large data_r is and trusts the device to
return at most one record's worth of bytes.  adm1266_nvmem_read_blackbox()
violates that contract: it advances read_buff inside data->dev_mem in
ADM1266_BLACKBOX_SIZE (64-byte) strides while the helper is willing to
write up to ADM1266_PMBUS_BLOCK_MAX (255) bytes.  A device that returns
more than 64 bytes on the trailing record (read_buff offset 1984 in
the 2048-byte dev_mem allocation) overflows dev_mem by up to 191 bytes
before the post-call

	if (ret != ADM1266_BLACKBOX_SIZE)
		return -EIO;

can reject the response.

Contain the fix in the caller without changing the helper signature:
read each record into a 255-byte local bounce buffer that matches the
helper's maximum output, validate the returned length, and only then
copy exactly ADM1266_BLACKBOX_SIZE bytes into the dev_mem slot.

Fixes: 407dc802a9c0 ("hwmon: (pmbus/adm1266) Add Block process call")
Cc: stable@vger.kernel.org
Signed-off-by: Abdurrahman Hussain <abdurrahman@nexthop.ai>
---
 drivers/hwmon/pmbus/adm1266.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
index 5c68e3177f64..ea1edb89d2a0 100644
--- a/drivers/hwmon/pmbus/adm1266.c
+++ b/drivers/hwmon/pmbus/adm1266.c
@@ -348,6 +348,7 @@ static void adm1266_init_debugfs(struct adm1266_data *data)
 
 static int adm1266_nvmem_read_blackbox(struct adm1266_data *data, u8 *read_buff)
 {
+	u8 record[ADM1266_PMBUS_BLOCK_MAX];
 	int record_count;
 	char index;
 	u8 buf[I2C_SMBUS_BLOCK_MAX];
@@ -365,13 +366,14 @@ static int adm1266_nvmem_read_blackbox(struct adm1266_data *data, u8 *read_buff)
 		return -EIO;
 
 	for (index = 0; index < record_count; index++) {
-		ret = adm1266_pmbus_block_xfer(data, ADM1266_READ_BLACKBOX, 1, &index, read_buff);
+		ret = adm1266_pmbus_block_xfer(data, ADM1266_READ_BLACKBOX, 1, &index, record);
 		if (ret < 0)
 			return ret;
 
 		if (ret != ADM1266_BLACKBOX_SIZE)
 			return -EIO;
 
+		memcpy(read_buff, record, ADM1266_BLACKBOX_SIZE);
 		read_buff += ADM1266_BLACKBOX_SIZE;
 	}
 

-- 
2.53.0

Re: [PATCH 5/5] hwmon: (pmbus/adm1266) bounce blackbox records through a protocol-sized buffer

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] This problem wasn't introduced by this patch, but the NVMEM read callback `adm1266_nvmem_read` lacks required locking, violating hwmon subsystem rules and causing a data race on `data->dev_mem`.
--

commit ec2121ec78bbb218045b0a4130c3ed16fa6c9a66
Author: Abdurrahman Hussain <abdurrahman@nexthop.ai>

hwmon: (pmbus/adm1266) bounce blackbox records through a protocol-sized buffer

This commit modifies adm1266_nvmem_read_blackbox() to copy records into a
local bounce buffer before writing them to the destination. This prevents
an overflow since the device can return up to 255 bytes while the helper
previously wrote directly into 64-byte strides of the buffer.

> diff --git a/drivers/hwmon/pmbus/adm1266.c b/drivers/hwmon/pmbus/adm1266.c
> index 5c68e3177f64e..ea1edb89d2a0c 100644
> --- a/drivers/hwmon/pmbus/adm1266.c
> +++ b/drivers/hwmon/pmbus/adm1266.c
> @@ -348,6 +348,7 @@ static void adm1266_init_debugfs(struct adm1266_data *data)
>  
>  static int adm1266_nvmem_read_blackbox(struct adm1266_data *data, u8 *read_buff)
>  {
> +	u8 record[ADM1266_PMBUS_BLOCK_MAX];
>  	int record_count;
>  	char index;
>  	u8 buf[I2C_SMBUS_BLOCK_MAX];
> @@ -365,13 +366,14 @@ static int adm1266_nvmem_read_blackbox(struct adm1266_data *data, u8 *read_buff)
>  		return -EIO;
>  
>  	for (index = 0; index < record_count; index++) {
> -		ret = adm1266_pmbus_block_xfer(data, ADM1266_READ_BLACKBOX, 1, &index, read_buff);
> +		ret = adm1266_pmbus_block_xfer(data, ADM1266_READ_BLACKBOX, 1, &index, record);
>  		if (ret < 0)
>  			return ret;
>  
>  		if (ret != ADM1266_BLACKBOX_SIZE)
>  			return -EIO;
>  
> +		memcpy(read_buff, record, ADM1266_BLACKBOX_SIZE);

This isn't a bug introduced by this patch, but does the NVMEM read callback
adm1266_nvmem_read() lack required locking around data->dev_mem?

The hwmon subsystem guidelines state that drivers must implement locking
for attributes registered outside the hwmon core, such as using hwmon_lock()
and hwmon_unlock().

Since the NVMEM core does not serialize reads, could concurrent user space
reads race in adm1266_nvmem_read():

static int adm1266_nvmem_read(void *priv, unsigned int offset, void *val, size_t bytes)
{
	...
	if (offset == 0) {
		memset(data->dev_mem, 0, data->nvmem_config.size);

		ret = adm1266_nvmem_read_blackbox(data, data->dev_mem);
		...
	}

	memcpy(val, data->dev_mem + offset, bytes);

	return 0;
}

If Thread A reads at offset == 0, it zeroes data->dev_mem and repopulates
it via adm1266_nvmem_read_blackbox().

If Thread B concurrently reads at a non-zero offset, it might execute the
memcpy() and return torn, zeroed, or corrupted data to user space.

Additionally, if both threads read at offset == 0, they could simultaneously
clear and write to the same buffer, destroying the integrity of the cached
blackbox data.

>  		read_buff += ADM1266_BLACKBOX_SIZE;
>  	}
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260515-adm1266-fixes-v1-0-1c1ea1349cfe@nexthop.ai?part=5