Re: [PATCH v2] EDAC/ghes: Fix buffer overflow in ghes_edac_register()

[Robert Richter]

On 22.07.22 15:20:50, Borislav Petkov wrote:
> + fix Robert's address.
> 
> On Thu, Jul 21, 2022 at 12:05:03PM -0600, Toshi Kani wrote:
> > The following buffer overflow BUG was observed on an HPE system.
> > ghes_edac_register() called strlen() on an uninitialized label,
> > which had non-zero values from krealloc_array().
> 
> I ended up massaging it into this:
> 
> ---
> From: Toshi Kani <toshi.kani@xxxxxxx>
> Date: Thu, 21 Jul 2022 12:05:03 -0600
> Subject: [PATCH] EDAC/ghes: Set the DIMM label unconditionally
> 
> The commit in Fixes enforced that both the bank and device strings
> passed to dimm_setup_label() are not NULL.
> 
> However, there are BIOSes, for example on a
> 
>   HPE ProLiant DL360 Gen10/ProLiant DL360 Gen10, BIOS U32 03/15/2019
> 
> which don't populate both strings:
> 
>   Handle 0x0020, DMI type 17, 84 bytes
>   Memory Device
>           Array Handle: 0x0013
>           Error Information Handle: Not Provided
>           Total Width: 72 bits
>           Data Width: 64 bits
>           Size: 32 GB
>           Form Factor: DIMM
>           Set: None
>           Locator: PROC 1 DIMM 1        <===== device
>           Bank Locator: Not Specified   <===== bank
> 
> This results in a buffer overflow because ghes_edac_register() calls
> strlen() on an uninitialized label, which had non-zero values left over
> from krealloc_array():
> 
>   detected buffer overflow in __fortify_strlen
>    ------------[ cut here ]------------
>    kernel BUG at lib/string_helpers.c:983!
>    invalid opcode: 0000 [#1] PREEMPT SMP NOPTI
>    CPU: 1 PID: 1 Comm: swapper/0 Tainted: G          I       5.18.6-200.fc36.x86_64 #1
>    Hardware name: HPE ProLiant DL360 Gen10/ProLiant DL360 Gen10, BIOS U32 03/15/2019
>    RIP: 0010:fortify_panic
>    ...
>    Call Trace:
>     <TASK>
>     ghes_edac_register.cold
>     ghes_probe
>     platform_probe
>     really_probe
>     __driver_probe_device
>     driver_probe_device
>     __driver_attach
>     ? __device_attach_driver
>     bus_for_each_dev
>     bus_add_driver
>     driver_register
>     acpi_ghes_init
>     acpi_init
>     ? acpi_sleep_proc_init
>     do_one_initcall
> 
> Change dimm_setup_label() to always initialize the label and use
> "N/A" in case bank or device is null.
> 
>   [ bp: Rewrite commit message. ]
> 
> Fixes: cb51a371d08e ("EDAC/ghes: Setup DIMM label from DMI and use it in error reports")
> Signed-off-by: Toshi Kani <toshi.kani@xxxxxxx>
> Signed-off-by: Borislav Petkov <bp@xxxxxxx>
> Tested-by: Robert Elliott <elliott@xxxxxxx>
> Link: https://lore.kernel.org/r/20220719220124.760359-1-toshi.kani@xxxxxxx
> ---
>  drivers/edac/ghes_edac.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
> index 59b0bedc9c24..fcab10e26b43 100644
> --- a/drivers/edac/ghes_edac.c
> +++ b/drivers/edac/ghes_edac.c
> @@ -103,9 +103,9 @@ static void dimm_setup_label(struct dimm_info *dimm, u16 handle)
>  
>  	dmi_memdev_name(handle, &bank, &device);
>  
> -	/* both strings must be non-zero */
> -	if (bank && *bank && device && *device)
> -		snprintf(dimm->label, sizeof(dimm->label), "%s %s", bank, device);
> +	snprintf(dimm->label, sizeof(dimm->label), "%s %s",
> +		 (bank && *bank) ? bank : "N/A",
> +		 (device && *device) ? device : "N/A");

The dimm->label buffer must be pre-initialized zero. This broke with:

 b9cae27728d1 EDAC/ghes: Scan the system once on driver init

since krealloc/krealloc_array() does not zero out the new allocated
buffer for struct dimm_info in enumerate_dimms(). This uninitialized
broken struct is then copied in ghes_edac_register() to the final
dimm_info destination. Originally, before b9cae27728d1, the struct was
zero initialized with kzalloc'ed by edac_mc_alloc() and directly used.
Now, that a copy is created first, this copy must be also zero
initialized.

IMO this is the proper fix:

diff --git a/drivers/edac/ghes_edac.c b/drivers/edac/ghes_edac.c
index 59b0bedc9c24..3b5bf6502141 100644
--- a/drivers/edac/ghes_edac.c
+++ b/drivers/edac/ghes_edac.c
@@ -220,6 +220,7 @@ static void enumerate_dimms(const struct dmi_header *dh, void *arg)
 	}
 
 	d = &hw->dimms[hw->num_dimms];
+	memset(d, 0, sizeof(*d));
 	d->idx = hw->num_dimms;
 
 	assign_dmi_dimm_info(d, entry);


-Robert

>  }