Re: [PATCH] mfd: cgbc: Fix teardown ordering in cgbc_remove()

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Truncation of error code from `cgbc_session_command` masks timeout errors.
- [High] Uninitialized kernel stack memory disclosure in `cgbc_get_version`.
--

--- Patch [1]: [PATCH] mfd: cgbc: Fix teardown ordering in cgbc_remove() ---
commit 59b97a15c94c66064a1c8f30345e941eb2fcf28f
Author: Thomas Richard <[email protected]>

mfd: cgbc: Fix teardown ordering in cgbc_remove()

Release Board Controller session once children are removed by the core.

Closes: https://sashiko.dev/#/patchset/cover.1783507945.git.u.kleine-koenig%40baylibre.com?part=19

> diff --git a/drivers/mfd/cgbc-core.c b/drivers/mfd/cgbc-core.c
> index 10bb4b414c34d..2becaf797646d 100644
> --- a/drivers/mfd/cgbc-core.c
> +++ b/drivers/mfd/cgbc-core.c
> @@ -364,9 +364,9 @@ static void cgbc_remove(struct platform_device *pdev)
>  {
>  	struct cgbc_device_data *cgbc = platform_get_drvdata(pdev);
>  
> -	cgbc_session_release(cgbc);
> -
>  	mfd_remove_devices(&pdev->dev);
> +
> +	cgbc_session_release(cgbc);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does the truncation of the error
code in cgbc_session_request() mask timeout errors?

In drivers/mfd/cgbc-core.c:cgbc_session_request():

    cgbc->session = cgbc_session_command(cgbc, CGBC_SESSION_CMD_REQUEST);

If cgbc_session_command() returns -ETIMEDOUT (-110) during device polling,
assigning this negative int to the u8 cgbc->session truncates it to 146.

The subsequent bounds check evaluates to false for 146, completely masking the
timeout error and allowing initialization to proceed with an invalid session:

    if (cgbc->session < CGBC_SESSION_VALID_MIN || cgbc->session > CGBC_SESSION_VALID_MAX)
        return dev_err_probe(cgbc->dev, -ECONNREFUSED, ...);

[Severity: High]
This is a pre-existing issue, but does cgbc_get_version() leak uninitialized
stack memory to userspace?

In drivers/mfd/cgbc-core.c:cgbc_get_version(), the data array is allocated on
the stack without initialization:

    u8 data[4];
    ...
    ret = cgbc_command(cgbc, &cmd, 1, &data, sizeof(data), NULL);

If the hardware returns a successful status but with a payload length less
than 3 bytes, cgbc_command() will write only those bytes into data and
return 0.

Then, cgbc_get_version() unconditionally copies the bytes:

    cgbc->version.feature = data[0];
    cgbc->version.major = data[1];
    cgbc->version.minor = data[2];

This copies potentially uninitialized kernel stack memory into the device
version structure, which is subsequently exposed to userspace.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1