All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] media: ngene: Fix out-of-bounds bug in ngene_command_config_free_buf()
@ 2021-04-20  0:16 Gustavo A. R. Silva
  2021-04-21 17:40 ` Kees Cook
  0 siblings, 1 reply; 3+ messages in thread
From: Gustavo A. R. Silva @ 2021-04-20  0:16 UTC (permalink / raw)
  To: Mauro Carvalho Chehab, Ralph Metzler, Matthias Benesch, Oliver Endriss
  Cc: linux-media, linux-kernel, Gustavo A. R. Silva, linux-hardening,
	Kees Cook

Fix an 11-year old bug in ngene_command_config_free_buf() while
addressing the following warnings caught with -Warray-bounds:

arch/alpha/include/asm/string.h:22:16: warning: '__builtin_memcpy' offset [12, 16] from the object at 'com' is out of the bounds of referenced subobject 'config' with type 'unsigned char' at offset 10 [-Warray-bounds]
arch/x86/include/asm/string_32.h:182:25: warning: '__builtin_memcpy' offset [12, 16] from the object at 'com' is out of the bounds of referenced subobject 'config' with type 'unsigned char' at offset 10 [-Warray-bounds]

The problem is that the original code is trying to copy 6 bytes of
data into a one-byte size member _config_ of the wrong structue
FW_CONFIGURE_BUFFERS, in a single call to memcpy(). This causes a
legitimate compiler warning because memcpy() overruns the length
of &com.cmd.ConfigureBuffers.config. It seems that the right
structure is FW_CONFIGURE_FREE_BUFFERS, instead, because it contains
6 more members apart from the header _hdr_. Also, the name of
the function ngene_command_config_free_buf() suggests that the actual
intention is to ConfigureFreeBuffers, instead of ConfigureBuffers
(which configuration takes place in the function ngene_command_config_buf(),
above).

Fix this by enclosing those 6 members of struct FW_CONFIGURE_FREE_BUFFERS
into new struct config, and use &com.cmd.ConfigureFreeBuffers.config as
the destination address, instead of &com.cmd.ConfigureBuffers.config,
when calling memcpy().

This also helps with the ongoing efforts to globally enable
-Warray-bounds and get us closer to being able to tighten the
FORTIFY_SOURCE routines on memcpy().

Link: https://github.com/KSPP/linux/issues/109
Fixes: dae52d009fc9 ("V4L/DVB: ngene: Initial check-in")
Cc: stable@vger.kernel.org
Reported-by: kernel test robot <lkp@intel.com>
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
 drivers/media/pci/ngene/ngene-core.c |  2 +-
 drivers/media/pci/ngene/ngene.h      | 14 ++++++++------
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/media/pci/ngene/ngene-core.c b/drivers/media/pci/ngene/ngene-core.c
index 07f342db6701..7481f553f959 100644
--- a/drivers/media/pci/ngene/ngene-core.c
+++ b/drivers/media/pci/ngene/ngene-core.c
@@ -385,7 +385,7 @@ static int ngene_command_config_free_buf(struct ngene *dev, u8 *config)
 
 	com.cmd.hdr.Opcode = CMD_CONFIGURE_FREE_BUFFER;
 	com.cmd.hdr.Length = 6;
-	memcpy(&com.cmd.ConfigureBuffers.config, config, 6);
+	memcpy(&com.cmd.ConfigureFreeBuffers.config, config, 6);
 	com.in_len = 6;
 	com.out_len = 0;
 
diff --git a/drivers/media/pci/ngene/ngene.h b/drivers/media/pci/ngene/ngene.h
index 84f04e0e0cb9..3d296f1998a1 100644
--- a/drivers/media/pci/ngene/ngene.h
+++ b/drivers/media/pci/ngene/ngene.h
@@ -407,12 +407,14 @@ enum _BUFFER_CONFIGS {
 
 struct FW_CONFIGURE_FREE_BUFFERS {
 	struct FW_HEADER hdr;
-	u8   UVI1_BufferLength;
-	u8   UVI2_BufferLength;
-	u8   TVO_BufferLength;
-	u8   AUD1_BufferLength;
-	u8   AUD2_BufferLength;
-	u8   TVA_BufferLength;
+	struct {
+		u8   UVI1_BufferLength;
+		u8   UVI2_BufferLength;
+		u8   TVO_BufferLength;
+		u8   AUD1_BufferLength;
+		u8   AUD2_BufferLength;
+		u8   TVA_BufferLength;
+	} __packed config;
 } __attribute__ ((__packed__));
 
 struct FW_CONFIGURE_UART {
-- 
2.27.0


^ permalink raw reply related	[flat|nested] 3+ messages in thread
* Re: [PATCH] media: ngene: Fix out-of-bounds bug in ngene_command_config_free_buf()
  2021-04-20  0:16 [PATCH] media: ngene: Fix out-of-bounds bug in ngene_command_config_free_buf() Gustavo A. R. Silva
@ 2021-04-21 17:40 ` Kees Cook
  2021-07-20  0:36   ` Gustavo A. R. Silva
  0 siblings, 1 reply; 3+ messages in thread
From: Kees Cook @ 2021-04-21 17:40 UTC (permalink / raw)
  To: Gustavo A. R. Silva
  Cc: Mauro Carvalho Chehab, Ralph Metzler, Matthias Benesch,
	Oliver Endriss, linux-media, linux-kernel, linux-hardening

On Mon, Apr 19, 2021 at 07:16:31PM -0500, Gustavo A. R. Silva wrote:
> Fix an 11-year old bug in ngene_command_config_free_buf() while
> addressing the following warnings caught with -Warray-bounds:
> 
> arch/alpha/include/asm/string.h:22:16: warning: '__builtin_memcpy' offset [12, 16] from the object at 'com' is out of the bounds of referenced subobject 'config' with type 'unsigned char' at offset 10 [-Warray-bounds]
> arch/x86/include/asm/string_32.h:182:25: warning: '__builtin_memcpy' offset [12, 16] from the object at 'com' is out of the bounds of referenced subobject 'config' with type 'unsigned char' at offset 10 [-Warray-bounds]
> 
> The problem is that the original code is trying to copy 6 bytes of
> data into a one-byte size member _config_ of the wrong structue
> FW_CONFIGURE_BUFFERS, in a single call to memcpy(). This causes a
> legitimate compiler warning because memcpy() overruns the length
> of &com.cmd.ConfigureBuffers.config. It seems that the right
> structure is FW_CONFIGURE_FREE_BUFFERS, instead, because it contains
> 6 more members apart from the header _hdr_. Also, the name of
> the function ngene_command_config_free_buf() suggests that the actual
> intention is to ConfigureFreeBuffers, instead of ConfigureBuffers
> (which configuration takes place in the function ngene_command_config_buf(),
> above).
> 
> Fix this by enclosing those 6 members of struct FW_CONFIGURE_FREE_BUFFERS
> into new struct config, and use &com.cmd.ConfigureFreeBuffers.config as
> the destination address, instead of &com.cmd.ConfigureBuffers.config,
> when calling memcpy().
> 
> This also helps with the ongoing efforts to globally enable
> -Warray-bounds and get us closer to being able to tighten the
> FORTIFY_SOURCE routines on memcpy().
> 
> Link: https://github.com/KSPP/linux/issues/109
> Fixes: dae52d009fc9 ("V4L/DVB: ngene: Initial check-in")
> Cc: stable@vger.kernel.org
> Reported-by: kernel test robot <lkp@intel.com>
> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>

Nice find! Yeah, this looks like a copy/paste bug but it went unnoticed
because it's occupying the same memory via the union. Heh.

Reviewed-by: Kees Cook <keescook@chromium.org>

-Kees

> ---
>  drivers/media/pci/ngene/ngene-core.c |  2 +-
>  drivers/media/pci/ngene/ngene.h      | 14 ++++++++------
>  2 files changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/pci/ngene/ngene-core.c b/drivers/media/pci/ngene/ngene-core.c
> index 07f342db6701..7481f553f959 100644
> --- a/drivers/media/pci/ngene/ngene-core.c
> +++ b/drivers/media/pci/ngene/ngene-core.c
> @@ -385,7 +385,7 @@ static int ngene_command_config_free_buf(struct ngene *dev, u8 *config)
>  
>  	com.cmd.hdr.Opcode = CMD_CONFIGURE_FREE_BUFFER;
>  	com.cmd.hdr.Length = 6;
> -	memcpy(&com.cmd.ConfigureBuffers.config, config, 6);
> +	memcpy(&com.cmd.ConfigureFreeBuffers.config, config, 6);
>  	com.in_len = 6;
>  	com.out_len = 0;
>  
> diff --git a/drivers/media/pci/ngene/ngene.h b/drivers/media/pci/ngene/ngene.h
> index 84f04e0e0cb9..3d296f1998a1 100644
> --- a/drivers/media/pci/ngene/ngene.h
> +++ b/drivers/media/pci/ngene/ngene.h
> @@ -407,12 +407,14 @@ enum _BUFFER_CONFIGS {
>  
>  struct FW_CONFIGURE_FREE_BUFFERS {
>  	struct FW_HEADER hdr;
> -	u8   UVI1_BufferLength;
> -	u8   UVI2_BufferLength;
> -	u8   TVO_BufferLength;
> -	u8   AUD1_BufferLength;
> -	u8   AUD2_BufferLength;
> -	u8   TVA_BufferLength;
> +	struct {
> +		u8   UVI1_BufferLength;
> +		u8   UVI2_BufferLength;
> +		u8   TVO_BufferLength;
> +		u8   AUD1_BufferLength;
> +		u8   AUD2_BufferLength;
> +		u8   TVA_BufferLength;
> +	} __packed config;
>  } __attribute__ ((__packed__));
>  
>  struct FW_CONFIGURE_UART {
> -- 
> 2.27.0
> 

-- 
Kees Cook

^ permalink raw reply	[flat|nested] 3+ messages in thread
* Re: [PATCH] media: ngene: Fix out-of-bounds bug in ngene_command_config_free_buf()
  2021-04-21 17:40 ` Kees Cook
@ 2021-07-20  0:36   ` Gustavo A. R. Silva
  0 siblings, 0 replies; 3+ messages in thread
From: Gustavo A. R. Silva @ 2021-07-20  0:36 UTC (permalink / raw)
  To: Kees Cook, Gustavo A. R. Silva
  Cc: Mauro Carvalho Chehab, Ralph Metzler, Matthias Benesch,
	Oliver Endriss, linux-media, linux-kernel, linux-hardening

Hi all,

I'm taking this in my tree[1] for 5.14-rc3.

Thanks
--
Gustavo

[1] https://git.kernel.org/pub/scm/linux/kernel/git/gustavoars/linux.git/log/?h=for-next/array-bounds

On 4/21/21 12:40, Kees Cook wrote:
> On Mon, Apr 19, 2021 at 07:16:31PM -0500, Gustavo A. R. Silva wrote:
>> Fix an 11-year old bug in ngene_command_config_free_buf() while
>> addressing the following warnings caught with -Warray-bounds:
>>
>> arch/alpha/include/asm/string.h:22:16: warning: '__builtin_memcpy' offset [12, 16] from the object at 'com' is out of the bounds of referenced subobject 'config' with type 'unsigned char' at offset 10 [-Warray-bounds]
>> arch/x86/include/asm/string_32.h:182:25: warning: '__builtin_memcpy' offset [12, 16] from the object at 'com' is out of the bounds of referenced subobject 'config' with type 'unsigned char' at offset 10 [-Warray-bounds]
>>
>> The problem is that the original code is trying to copy 6 bytes of
>> data into a one-byte size member _config_ of the wrong structue
>> FW_CONFIGURE_BUFFERS, in a single call to memcpy(). This causes a
>> legitimate compiler warning because memcpy() overruns the length
>> of &com.cmd.ConfigureBuffers.config. It seems that the right
>> structure is FW_CONFIGURE_FREE_BUFFERS, instead, because it contains
>> 6 more members apart from the header _hdr_. Also, the name of
>> the function ngene_command_config_free_buf() suggests that the actual
>> intention is to ConfigureFreeBuffers, instead of ConfigureBuffers
>> (which configuration takes place in the function ngene_command_config_buf(),
>> above).
>>
>> Fix this by enclosing those 6 members of struct FW_CONFIGURE_FREE_BUFFERS
>> into new struct config, and use &com.cmd.ConfigureFreeBuffers.config as
>> the destination address, instead of &com.cmd.ConfigureBuffers.config,
>> when calling memcpy().
>>
>> This also helps with the ongoing efforts to globally enable
>> -Warray-bounds and get us closer to being able to tighten the
>> FORTIFY_SOURCE routines on memcpy().
>>
>> Link: https://github.com/KSPP/linux/issues/109
>> Fixes: dae52d009fc9 ("V4L/DVB: ngene: Initial check-in")
>> Cc: stable@vger.kernel.org
>> Reported-by: kernel test robot <lkp@intel.com>
>> Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
> 
> Nice find! Yeah, this looks like a copy/paste bug but it went unnoticed
> because it's occupying the same memory via the union. Heh.
> 
> Reviewed-by: Kees Cook <keescook@chromium.org>
> 
> -Kees
> 
>> ---
>>  drivers/media/pci/ngene/ngene-core.c |  2 +-
>>  drivers/media/pci/ngene/ngene.h      | 14 ++++++++------
>>  2 files changed, 9 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/media/pci/ngene/ngene-core.c b/drivers/media/pci/ngene/ngene-core.c
>> index 07f342db6701..7481f553f959 100644
>> --- a/drivers/media/pci/ngene/ngene-core.c
>> +++ b/drivers/media/pci/ngene/ngene-core.c
>> @@ -385,7 +385,7 @@ static int ngene_command_config_free_buf(struct ngene *dev, u8 *config)
>>  
>>  	com.cmd.hdr.Opcode = CMD_CONFIGURE_FREE_BUFFER;
>>  	com.cmd.hdr.Length = 6;
>> -	memcpy(&com.cmd.ConfigureBuffers.config, config, 6);
>> +	memcpy(&com.cmd.ConfigureFreeBuffers.config, config, 6);
>>  	com.in_len = 6;
>>  	com.out_len = 0;
>>  
>> diff --git a/drivers/media/pci/ngene/ngene.h b/drivers/media/pci/ngene/ngene.h
>> index 84f04e0e0cb9..3d296f1998a1 100644
>> --- a/drivers/media/pci/ngene/ngene.h
>> +++ b/drivers/media/pci/ngene/ngene.h
>> @@ -407,12 +407,14 @@ enum _BUFFER_CONFIGS {
>>  
>>  struct FW_CONFIGURE_FREE_BUFFERS {
>>  	struct FW_HEADER hdr;
>> -	u8   UVI1_BufferLength;
>> -	u8   UVI2_BufferLength;
>> -	u8   TVO_BufferLength;
>> -	u8   AUD1_BufferLength;
>> -	u8   AUD2_BufferLength;
>> -	u8   TVA_BufferLength;
>> +	struct {
>> +		u8   UVI1_BufferLength;
>> +		u8   UVI2_BufferLength;
>> +		u8   TVO_BufferLength;
>> +		u8   AUD1_BufferLength;
>> +		u8   AUD2_BufferLength;
>> +		u8   TVA_BufferLength;
>> +	} __packed config;
>>  } __attribute__ ((__packed__));
>>  
>>  struct FW_CONFIGURE_UART {
>> -- 
>> 2.27.0
>>
> 

^ permalink raw reply	[flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-07-20  0:53 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-20  0:16 [PATCH] media: ngene: Fix out-of-bounds bug in ngene_command_config_free_buf() Gustavo A. R. Silva
2021-04-21 17:40 ` Kees Cook
2021-07-20  0:36   ` Gustavo A. R. Silva

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.