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

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

[Kees Cook]

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>
---
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)
-- 
2.34.1

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

[Hans de Goede]

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)

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

[Kees Cook]

On Sat, Jul 26, 2025 at 02:24:51PM +0200, Hans de Goede wrote:
> 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 ?

I apologize; this slipped through the cracks. Shall I take it for -rc2,
or do you want to snag it?

-Kees

-- 
Kees Cook

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

[Hans de Goede]

Hi Kees,

On 29-Jul-25 2:46 AM, Kees Cook wrote:
> On Sat, Jul 26, 2025 at 02:24:51PM +0200, Hans de Goede wrote:
>> 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 ?
> 
> I apologize; this slipped through the cracks. Shall I take it for -rc2,
> or do you want to snag it?

I'm just back from vacation and I see you've send this
to Linus already:

https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/staging/media/atomisp?id=ee4cf798202d285dcbe85e4467a094c44f5ed8e6

Which is great, thank you.

Regards,

Hans