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

[Al Chu]

Hey Christian,

I've applied the patch, thanks!  Just wondering how bad this is and if
it warrants a FreeIPMI release.  It sounds like on certain systems, a
segfault was probably happening all the time?

Al

On Thu, 2020-09-03 at 02:50 -0700, Al Chu wrote:
> 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++)
> >      {
> 
>