RE: [PATCH v2 2/2] hwmon: (pmbus/max20830) add driver for max20830

> 
> Sashiko AI review found 2 potential issue(s):
> - [High] Missing validation for short reads from
> `i2c_smbus_read_i2c_block_data()` can lead to uninitialized stack memory
> access and information leak.
> - [Low] Off-by-one error when null-terminating the device ID string truncates
> the last character of the payload.
> --
> 
> commit 479738352902d3197058c3172cee31960249a6a5
> Author: Alexis Czezar Torreno <[email protected]>
> 
> hwmon: (pmbus/max20830) add driver for max20830
> 
> This adds support for the MAX20830 step-down DC-DC switching regulator
> with a PMBus interface, allowing monitoring of input/output voltages, output
> current, and temperature.
> 
> > diff --git a/drivers/hwmon/pmbus/max20830.c
> > b/drivers/hwmon/pmbus/max20830.c new file mode 100644 index
> > 0000000000000..21ea8b59150cb
> > --- /dev/null
> > +++ b/drivers/hwmon/pmbus/max20830.c
> 
> [ ... ]
> 
> > +	ret = i2c_smbus_read_i2c_block_data(client, PMBUS_IC_DEVICE_ID,
> > +					    I2C_SMBUS_BLOCK_MAX, buf);
> > +	if (ret < 0)
> > +		return dev_err_probe(&client->dev, ret,
> > +				     "Failed to read IC_DEVICE_ID\n");
> 
> i2c_smbus_read_i2c_block_data() returns the number of bytes read on
> success.
> If the I2C controller performs a short read, returning fewer bytes than
> expected, buf will contain uninitialized stack memory for the remaining bytes.
> 
> If buf[0] coincidentally contains 9, the len != 9 check below will pass. Could this
> lead to uninitialized stack memory being accessed and printed by
> dev_err_probe() on a mismatch? Should we verify the number of bytes read
> (e.g., ret >= 10) instead of just ret < 0?

It was suggested to me to just initialize the buffer to avoid this issue.

> 
> > +
> > +	/* First byte is the block length. */
> > +	len = buf[0];
> > +	if (len != 9)
> > +		return dev_err_probe(&client->dev, -ENODEV,
> > +				     "Unexpected IC_DEVICE_ID response\n");
> > +
> > +	buf[len] = '\0';
> 
> Since the payload of length len occupies buf[1] through buf[len], doesn't setting
> buf[len] to '\0' overwrite the final byte of the device ID payload?
> 
> While this 9th byte is ignored by the 8-byte strncmp() call below, it will cause
> the dev_err_probe() error log to print a truncated string. Should the null
> terminator be placed at buf[len + 1] instead?

When literally reading the ID from the device the device replies with:
 { 0x09, M, A, X, 2, 0, 8, 3, 0} - nothing follows not even terminator.

buf[0] is 9, so len = 9, it apparently includes itself in the length/number of bytes.
Actual data exist from buf[1] to buf[len-1], hence buf[len] to '\0' is actually correct.

I can add comments on the code to clarify this.

> 
> > +	if (strncmp(buf + 1, "MAX20830", 8))
> > +		return dev_err_probe(&client->dev, -ENODEV,
> > +				     "Unsupported device: '%s'\n", buf + 1);
> 
> --
> Sashiko AI review *
> https://urldefense.com/v3/__https://sashiko.dev/*/patchset/20260416-
> dev_max20830-v2-0-
> [email protected]?part=2__;Iw!!A3Ni8CS0y2Y!6tnYi0B1txw5D1iHlO
> wMF5xTB08wkOaqxNEZnQLyd615sTaced0WcqjVwJpIAN6vuhsxvyz81g-
> FcmRT1xWRgDSmicnZcYI$