Re: [Openhpi-devel] [PATCH] ipmidirect

[Shuah K. <shu...@hp...>]

Chris,

I was incorrect in saying zero byte data message, insufficient data
packet or short packet is a better characterization. In this case the
insufficient data packet is due to a bug in the carrier mgmt fw that
sends a message with data[13] set to 0. 

(The code in the ReadResponse() is rather unreadable I have to say,
including a FIXME comment in this routine. It is not very clear what it
is looking for in most cases.) :)

In ReadResponse() line # 928, data[13] gets extracted and there is a
check that is done using the len returned by recvfrom() and comparing
(len <  data[13] + 14). If len is less than that, it returns error.
However if data[13] is 0, there will be trouble later on and this logic
doesn't catch it. This is no auth message case and there is similar
problem with the auth message logic that starts at line # 952 which
takes the data[29] value and then does compares with len < data[29] +
30. Again it doesn't really validate if data[29] is > 0.

So far it is ok, but we are setup for trouble later when tmsg is
assigned to data + 30 on line #994 if data[4] != 0 which indicates the
msg contains the auth data and to data+14 on line # 997 in no auth case.
We are still good, however starting on line # 1033 we start accessing
offsets into tmsg and also on line 1051  msg.m_data_len gets set to a
negative value if data_len is 0. data_len is not checked until the else
case for bad IPMB msg if conditional that starts at line # 1071. This
else case checks if data_len < 15.

I haven't isolated the seg fault to specific line, but looking at the
logic after data_len and tmsg are assigned, there are multiple places
this code could segfault, before data_len is validated and/or if it
never gets checked as the data_len < 15 is a conditional check.

I hope this helps answer your question and gives more context. This
patch I proposed ( I think it is incomplete as a similar check is needed
for for the auth case when data_len is extracted from data[29] ) fixes
the seg fault. I still have the bad fw squirreled away if there is need
to isolate the seg fault to specific line. :)

Thanks,
-- Shuah

On Fri, 2008-12-05 at 12:17 -0800, Chris Rinaldo wrote:
> Shuah Khan wrote:
> > Here is a patch for fixing the ipmidirect plug-in segmentation fault
> > when it receives a message with 0 length data bytes. A new check is
> > added to ReadResponse() to check if data[13] is zero. Without this
> > check, later on in this routine, it tries to make an invalid reference
> > and seg faults.
> 
> Shuah,
> 
> Under what circumstances did a message with no data occur?
> 
> Which specific line in the code was causing the seg fault?
> 
> The code you added accepts the message if the data length is greater 
> than 0. But, shouldn't the data contain a valid IPMI Lan response? In 
> which case, I believe the minimum length should be 8, 1 byte for each 
> of the following fields, assuming no response data:
> 	rqAddr,
> 	netFn,
> 	checksum 1,
> 	rsAddr,
> 	rqSeg/rsLUN,
> 	cmd,
> 	completion code,
> 	checksum 2
> 
> Chris
> 
> 
> > Here is the svn diff output:
> > 
> > --- ipmi_con_lan.cpp	(revision 6918)
> > +++ ipmi_con_lan.cpp	(working copy)
> > @@ -931,6 +931,13 @@
> >  	    stdlog << "Dropped message because too small(2)\n";
> >  	    return eResponseTypeError;
> >  	  }
> > +       // no data bytes
> > +       if ( data[13] <= 0 ) 
> > +	  {
> > +	    // Not enough data was supplied, reject the message.
> > +	    stdlog << "Dropped message because data len is <=0 \n";
> > +	    return eResponseTypeError;
> > +          }
> >  
> >         data_len = data[13];
> >       }
> > 
> 

--