Re: [PATCH 09/28] mfd: cgbc: Zero-initialise version buffer in cgbc_get_version()

[Thomas Richard (congatec GmbH)]

On 2026-10-21 18:38:18+02:00, Thomas Richard (congatec GmbH) wrote:
> > If the Board Controller returns a short response to the firmware
> > revision query in cgbc_get_version(), cgbc_command() can succeed
> > without populating all bytes of the stack-allocated data buffer.
> > Those uninitialised stack bytes are then copied into cgbc->version
> > and exposed to userspace via the cgbc_version sysfs attribute.
> > 
> > Zero-initialise the data buffer on the stack to prevent leaking
> > uninitialised kernel stack memory.
> > 
> > Fixes: 6f1067cfbee7 ("mfd: Add Congatec Board Controller driver")
> > Closes: https://sashiko.dev/#/bug/linux-8c473713-527e-4619-8404-689f28c7654d
> > Cc: stable@xxxxxxxxxxxxxxx
> > Signed-off-by: Lee Jones <lee@xxxxxxxxxx>
> >
> > diff --git a/drivers/mfd/cgbc-core.c b/drivers/mfd/cgbc-core.c
> > index edfac2b60718..7513ba42eb96 100644
> > --- a/drivers/mfd/cgbc-core.c
> > +++ b/drivers/mfd/cgbc-core.c
> > @@ -461,7 +461,7 @@ ATTRIBUTE_GROUPS(cgbc);
> >  static int cgbc_get_version(struct cgbc_device_data *cgbc)
> >  {
> >  	u8 cmd = CGBC_CMD_GET_FW_REV;
> > -	u8 data[4];
> > +	u8 data[4] = {};
> >  	int ret;
> >  
> >  	ret = cgbc_command(cgbc, &cmd, 1, &data, sizeof(data), NULL);
> 
> Yes Sashiko complained multiple times even if I already explained it cannot
> happen if cgbc_command succeeds. Anyway I agree that things can be improved
> but I would prefer in a generic way:
> - add macros for cmd resp size (eg CGBC_CMD_GET_FW_REV_RESP_SIZE) in all cgbc
>   drivers
> - cgbc_command() should return an error if data buffer size doesn't match
>   the number of bytes returned by the hardware, it is not the case today [1].

I mean return an error if data buffer size is smaller than the number of
bytes returned by the hardware. Also cgbc_command() should initialize data
buffer first (in case of it does not populate all bytes after), so no need
to it from the callers. I also noticed we use cmd and data pointers, but we
did not check if they are NULL.

I'll send a series to address all of these issues. So you can drop this
patch.

Best Regards,
Thomas