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

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 s=
ysfs if the board controller firmware returns fewer than 3 bytes of data.
- [Medium] The `mfd_cell` array `cgbc_devs` is missing the `const` qualifie=
r.
--

--- Patch [1]: [PATCH 1/4] mfd: cgbc: Fix use of negative error code as val=
id 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-v=
1-1-79274ad62b3a%40bootlin.com?part=3D1
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[] =3D {
>  	{ .name =3D "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.

[ ... ]

>  	{ .name =3D "cgbc-i2c", .id =3D 1 },
>  	{ .name =3D "cgbc-i2c", .id =3D 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 hand=
le
auto-indexing. The subsystem guidelines recommend using PLATFORM_DEVID_AUTO
to prevent driver load failures.

[ ... ]

>  static int cgbc_get_version(struct cgbc_device_data *cgbc)
>  {
>  	...
> 	ret =3D cgbc_command(cgbc, &cmd, 1, &data, sizeof(data), NULL);
> 	if (ret)
> 		return ret;
>=20
> 	cgbc->version.feature =3D data[0];
> 	cgbc->version.major =3D data[1];
> 	cgbc->version.minor =3D 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.

[ ... ]

>  static int cgbc_init_device(struct cgbc_device_data *cgbc)
>  {
>  	...
> 	ret =3D 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.

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260804-cgbc-i2c-s=
[email protected]?part=3D1