Re: [PATCH] staging: media: atomisp: Fix stack buffer overflow in gmin_get_var_int()

[Hans de Goede <hansg@kernel.org>]

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 <z3ptaa@gmail.com>
> Closes: https://lore.kernel.org/all/CAPBS6KoQyM7FMdPwOuXteXsOe44X4H3F8Fw+y_qWq6E+OdmxQA@mail.gmail.com
> Fixes: 38d4f74bc148 ("media: atomisp_gmin_platform: stop abusing efivar API")
> Signed-off-by: Kees Cook <kees@kernel.org>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <hansg@kernel.org>

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 <gregkh@linuxfoundation.org>
> Cc: Ard Biesheuvel <ardb@kernel.org>
> Cc: Andy Shevchenko <andy@kernel.org>
> Cc: Hans de Goede <hansg@kernel.org>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Cc: Sakari Ailus <sakari.ailus@linux.intel.com>
> Cc: <linux-media@vger.kernel.org>
> Cc: <linux-staging@lists.linux.dev>
> 
> 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)