Re: [PATCH] hwmon: (spd5118) Select page 0 unconditionally during probe

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unsafe unconditional I2C write during probe before device verification risks permanent data corruption on unrelated devices.
- [Medium] The I2C write to select page 0 unintentionally clears bits 7:4 of the legacy mode register due to an incorrect bitmask.
--

commit 270ea09e9966102e26a3fc6a4bba86ec060d0e37
Author: Armin Wolf <W_Armin@gmx.de>
hwmon: (spd5118) Select page 0 unconditionally during probe

This commit modifies the probe sequence of the spd5118 driver to
unconditionally select page 0 before attempting to read the device type
register. This change is intended to bypass BIOS-level restrictions on I2C
writes to the SPD device and properly handle chips that restrict access to
volatile registers when a non-zero page is selected.

> diff --git a/drivers/hwmon/spd5118.c b/drivers/hwmon/spd5118.c
> index 9724cf70b61d4..65a0d5852ce86 100644
> --- a/drivers/hwmon/spd5118.c
> +++ b/drivers/hwmon/spd5118.c
[ ... ]
> @@ -637,45 +637,32 @@ static int spd5118_i2c_init(struct i2c_client *client)
[ ... ]
> +	mode = i2c_smbus_read_byte_data(client, SPD5118_REG_I2C_LEGACY_MODE);
> +	if (mode < 0)
> +		return mode;
> +
> +	err = i2c_smbus_write_byte_data(client, SPD5118_REG_I2C_LEGACY_MODE,
> +					mode & SPD5118_LEGACY_MODE_ADDR);

[Severity: High]
Does this unconditional write execute before any device identity checks,
creating a risk of data corruption for incorrectly bound devices?

If this driver binds to an incorrect I2C device, such as a standard DDR4 SPD
EEPROM residing at the same 0x50-0x57 address range, it will blindly overwrite
byte 0x0B of the device before verifying its identity.

[Severity: Medium]
Does this bitwise AND inadvertently zero out bits 7:4 of the legacy mode
register?

The masking operation mode & SPD5118_LEGACY_MODE_ADDR effectively masks out
all bits except bit 3, forcing bits 7:4 (and bits 2:0) to zero. In the
previous implementation, the driver explicitly verified that bits 7:4 were
zero before applying this mask. With that check removed, any device with bits
7:4 set will have them silently wiped out.

To safely clear only the page bits (2:0), should the mask logically be
mode & ~SPD5118_LEGACY_PAGE_MASK?

> +	if (err < 0)
> +		return err;
> +
> +	regval = i2c_smbus_read_word_swapped(client, SPD5118_REG_TYPE);
> +	if (regval < 0)
> +		return regval;
> +
> +	if (regval != 0x5118)
> +		return -ENODEV;

[Severity: High]
If the type mismatch occurs here, the probe aborts without rolling back the
write to SPD5118_REG_I2C_LEGACY_MODE. Does this leak the state modification
to misidentified devices?

>  
>  	/* We are reasonably sure that this is really a SPD5118 hub controller */
>  	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260830231036.203719-1-W_Armin@gmx.de?part=1