Hi Kees, On 24-Jul-25 10:08 AM, Kees Cook wrote: > When gmin_get_config_var() calls efi.get_variable() and the EFI variable > is larger than the expected buffer size, two behaviors combine to create > a stack buffer overflow: > > 1. gmin_get_config_var() does not return the proper error code when > efi.get_variable() fails. It returns the stale 'ret' value from > earlier operations instead of indicating the EFI failure. > > 2. When efi.get_variable() returns EFI_BUFFER_TOO_SMALL, it updates > *out_len to the required buffer size but writes no data to the output > buffer. However, due to bug #1, gmin_get_var_int() believes the call > succeeded. > > The caller gmin_get_var_int() then performs: > - Allocates val[CFG_VAR_NAME_MAX + 1] (65 bytes) on stack > - Calls gmin_get_config_var(dev, is_gmin, var, val, &len) with len=64 > - If EFI variable is >64 bytes, efi.get_variable() sets len=required_size > - Due to bug #1, thinks call succeeded with len=required_size > - Executes val[len] = 0, writing past end of 65-byte stack buffer > > This creates a stack buffer overflow when EFI variables are larger than > 64 bytes. Since EFI variables can be controlled by firmware or system > configuration, this could potentially be exploited for code execution. > > Fix the bug by returning proper error codes from gmin_get_config_var() > based on EFI status instead of stale 'ret' value. > > The gmin_get_var_int() function is called during device initialization > for camera sensor configuration on Intel Bay Trail and Cherry Trail > platforms using the atomisp camera stack. > > Reported-by: zepta <[email protected]> > Closes: > https://lore.kernel.org/all/capbs6koqym7fmdpwouxtexsoe44x4h3f8fw+y_qwq6e+odm...@mail.gmail.com > Fixes: 38d4f74bc148 ("media: atomisp_gmin_platform: stop abusing efivar API") > Signed-off-by: Kees Cook <[email protected]>
Thanks, patch looks good to me: Reviewed-by: Hans de Goede <[email protected]> I've already send an atomisp pull-request for 6.17 out and this is already in media-committers/next now and the media subsystem is typically not good in merging fixes just before the merge window. Kees, the file touched here is unchanged in media-committers/next vs Linus' latest master, can you send this fix to Linus yourself ? Regards, Hans > --- > Cc: Greg Kroah-Hartman <[email protected]> > Cc: Ard Biesheuvel <[email protected]> > Cc: Andy Shevchenko <[email protected]> > Cc: Hans de Goede <[email protected]> > Cc: Mauro Carvalho Chehab <[email protected]> > Cc: Sakari Ailus <[email protected]> > Cc: <[email protected]> > Cc: <[email protected]> > > Note that as an exercise this fix and the commit log body (I wrote the > tags) was entirely written by an LLM (and reviewed by me), though I really > had to help it focus on where to be looking. Here were my prompts: > > Is there a buffer overflow problem associated with gmin_get_config_var()'s > use of efi.get_variable()? > > What does efi.get_variable() do to out_len if it fails? > > Does the function return an error when efi.get_variable fails? > > What would the caller do when it sees a success but when efi.get_variable > changed out_len? > > Propose fixes and write a commit message with all of the details you > just gave me about reachability, impact, etc. > > Since the EFI_SUCCESS test ends with "return 0" you don't need an explicit > "else" block for the error path code. > --- > .../staging/media/atomisp/pci/atomisp_gmin_platform.c | 9 +++++---- > 1 file changed, 5 insertions(+), 4 deletions(-) > > diff --git a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c > b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c > index 5f59519ac8e2..964cc3bcc0ac 100644 > --- a/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c > +++ b/drivers/staging/media/atomisp/pci/atomisp_gmin_platform.c > @@ -1272,14 +1272,15 @@ static int gmin_get_config_var(struct device *maindev, > if (efi_rt_services_supported(EFI_RT_SUPPORTED_GET_VARIABLE)) > status = efi.get_variable(var16, &GMIN_CFG_VAR_EFI_GUID, NULL, > (unsigned long *)out_len, out); > - if (status == EFI_SUCCESS) > + if (status == EFI_SUCCESS) { > dev_info(maindev, "found EFI entry for '%s'\n", var8); > - else if (is_gmin) > + return 0; > + } > + if (is_gmin) > dev_info(maindev, "Failed to find EFI gmin variable %s\n", > var8); > else > dev_info(maindev, "Failed to find EFI variable %s\n", var8); > - > - return ret; > + return -ENOENT; > } > > int gmin_get_var_int(struct device *dev, bool is_gmin, const char *var, int > def)
