Re: [PATCH] libfreeipmi: fix segfault in SPMI parsing

[Al Chu]

Hi Christian,

LGTM, thanks and thanks for the thorough description.  I'm not a
computer that I can apply right now, but will do so later.

This seems like a critical enough bug to do a new release, i will try
to do so by the end of the week.

Al

On Thu, 2020-09-03 at 09:14 +0200, Christian Ehrhardt wrote:
> Since 68ed819 "acpi-spmi locate driver" the initialization of
> acpi_table
> was done as a direct assignment on a global variable.
> Later on _ipmi_acpi_get_table was restructured and now expects
> malloced
> memory, but the old intialization was left in place now killing the
> malloced
> pointer.
> 
> It turns out that on systems without /sys/firmware/acpi/tables/SPMI
> like
> Dell iDRAC6/9 _ipmi_acpi_get_table then tries to assign
> acpi_table_buf to it.
> This leads to an access of 0x0 and a crash, for a backtrace see:
> https://launchpadlibrarian.net/495892193/apport-retrace-ipmi-locate.txt
> 
> On other hardware (with SPMI file in sysfs) there are some early
> exits before
> this happens. Additionally Before 094cd5ce "Audit libfreeipmi, remove
> unnecessary memsets, fix +1 buffers, etc." which is in >1.6.0 there
> was
> another early exit in _ipmi_acpi_get_table_dev_mem as
> _ipmi_acpi_get_rsdp
> never returned non zero, but since that fix it can reach the bad
> code.
> 
> The old style assignment of acpi_table is wrong (writes the pointer
> instead
> of the pointer target) and superfluous as it is done correct at the
> beginning
> of _ipmi_acpi_get_table_dev_mem and between this init and the usage
> no more
> touched. Therefore it is safe to just remove the bad initialization.
> 
> Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> Reported-by: jeffrey.lane@canonical.com
> ---
>  libfreeipmi/locate/ipmi-locate-acpi-spmi.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/libfreeipmi/locate/ipmi-locate-acpi-spmi.c
> b/libfreeipmi/locate/ipmi-locate-acpi-spmi.c
> index 710674e53..11c688524 100644
> --- a/libfreeipmi/locate/ipmi-locate-acpi-spmi.c
> +++ b/libfreeipmi/locate/ipmi-locate-acpi-spmi.c
> @@ -1384,7 +1384,6 @@ _ipmi_acpi_get_table_dev_mem (ipmi_locate_ctx_t
> ctx,
>    else
>      acpi_table_count = rsdt_xsdt_table_data_length / 8;
>  
> -  acpi_table = NULL;
>    acpi_table_length = 0;
>    for (i = 0, signature_table_count = 0; i < acpi_table_count; i++)
>      {