| From 8d4abca95ecc82fc8c41912fa0085281f19cc29f Mon Sep 17 00:00:00 2001 |
| From: "Gustavo A. R. Silva" <gustavoars@kernel.org> |
| Date: Mon, 19 Apr 2021 18:43:32 -0500 |
| Subject: media: ngene: Fix out-of-bounds bug in ngene_command_config_free_buf() |
| |
| From: Gustavo A. R. Silva <gustavoars@kernel.org> |
| |
| commit 8d4abca95ecc82fc8c41912fa0085281f19cc29f upstream. |
| |
| 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 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> |
| Reviewed-by: Kees Cook <keescook@chromium.org> |
| Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> |
| Link: https://lore.kernel.org/linux-hardening/20210420001631.GA45456@embeddedor/ |
| Signed-off-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org> |
| --- |
| drivers/media/pci/ngene/ngene-core.c | 2 +- |
| drivers/media/pci/ngene/ngene.h | 14 ++++++++------ |
| 2 files changed, 9 insertions(+), 7 deletions(-) |
| |
| --- 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 |
| |
| 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; |
| |
| --- 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 { |