Re: [PATCH 1/4] mfd: cgbc: Fix use of negative error code as valid session handle

On 8/4/26 5:29 PM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Hardcoded `.id` values are used in the `mfd_cell` array `cgbc_devs` instead of relying on `PLATFORM_DEVID_AUTO`.
> - [High] Uninitialized kernel stack memory can be leaked to userspace via sysfs if the board controller firmware returns fewer than 3 bytes of data.
> - [Medium] The `mfd_cell` array `cgbc_devs` is missing the `const` qualifier.
> --
> 
> --- Patch [1]: [PATCH 1/4] mfd: cgbc: Fix use of negative error code as valid session handle ---
> commit d37d718d69c4fd9448146a18eac17c7d57dcefc2
> Author: Thomas Richard (congatec GmbH) <[email protected]>
> 
> mfd: cgbc: Fix use of negative error code as valid session handle
> 
> The cgbc_session_command() return value was directly cast to u8 and used
> as session handle without error checking. Casting a negative error to u8
> produces a valid-looking session handle. So check if return value is
> positive before to cast and use it.
> 
> Cc: [email protected]
> Reported-by: Sashiko <[email protected]>
> Closes: https://sashiko.dev/#/patchset/20260713-cgbc-core-fix-cgbc-remove-v1-1-79274ad62b3a%40bootlin.com?part=1
> Fixes: 6f1067cfbee7 ("mfd: Add Congatec Board Controller driver")
> 
>> diff --git a/drivers/mfd/cgbc-core.c b/drivers/mfd/cgbc-core.c
>> index 2becaf797646d..4a409234e66c8 100644
>> --- a/drivers/mfd/cgbc-core.c
>> +++ b/drivers/mfd/cgbc-core.c
> 
> [ ... ]
> 
>>  static struct mfd_cell cgbc_devs[] = {
>>  	{ .name = "cgbc-wdt"	},
> 
> [Severity: Medium]
> This is a pre-existing issue, but should cgbc_devs be declared with the
> const qualifier?
> 
> The MFD subsystem guidelines require mfd_cell arrays to be static const
> so they are placed in .rodata rather than mutable memory.

yes indeed. Not a big deal, I can add a patch to fix this in v2.

> 
> [ ... ]
> 
>>  	{ .name = "cgbc-i2c", .id = 1 },
>>  	{ .name = "cgbc-i2c", .id = 2 },
> 
> [Severity: High]
> This is a pre-existing issue, but could this cause device naming collisions
> in sysfs?
> 
> The cells use hardcoded .id values instead of allowing the MFD core to handle
> auto-indexing. The subsystem guidelines recommend using PLATFORM_DEVID_AUTO
> to prevent driver load failures.

PLATFORM_DEVID_NONE (-1) is used and id starts from 1.
The first I2C bus receives a platform_id of 0 (cgbc-i2c.0, cgbc-i2c.1),
so no collision. While this design is maybe not ideal, but it works for
now. Also the plan is to add support for additional boards and I2C
busses in the future. And since some boards won't support all I2C
busses, so this scheme will probably be reworked.

> 
> [ ... ]
> 
>>  static int cgbc_get_version(struct cgbc_device_data *cgbc)
>>  {
>>  	...
>> 	ret = cgbc_command(cgbc, &cmd, 1, &data, sizeof(data), NULL);
>> 	if (ret)
>> 		return ret;
>>
>> 	cgbc->version.feature = data[0];
>> 	cgbc->version.major = data[1];
>> 	cgbc->version.minor = data[2];
> 
> [Severity: High]
> This is a pre-existing issue, but can this leak uninitialized stack memory
> to userspace?
> 
> If the board controller hardware returns fewer than 3 bytes during the
> CGBC_CMD_GET_FW_REV command, cgbc_command() might still successfully verify
> the truncated response and return 0.
> 
> The code then unconditionally reads data[0], data[1], and data[2], storing
> potentially uninitialized stack bytes into cgbc->version, which is later
> exposed via the cgbc_version sysfs attribute.

No. If CGBC_CMD_GET_FW_REV command succeeds (and we check cgbc_command's
return code), it returns exactly 4 bytes. So there is no way to get
uninitialized values in data buffer.

Also to prevent buffer overflow, cgbc_command verifies that the buffer
is large enough to hold all received data.

> 
> [ ... ]
> 
>>  static int cgbc_init_device(struct cgbc_device_data *cgbc)
>>  {
>>  	...
>> 	ret = mfd_add_devices(cgbc->dev, -1, cgbc_devs, ARRAY_SIZE(cgbc_devs),
>> 			      NULL, 0, NULL);
> 
> [Severity: High]
> This is a pre-existing issue, but should this pass PLATFORM_DEVID_AUTO
> instead of -1 (PLATFORM_DEVID_NONE)?
> 
> Using PLATFORM_DEVID_NONE combined with hardcoded .id values in the mfd_cell
> array violates MFD guidelines.

Ditto (see above)

-- 
Thomas Richard, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com