[ofw][PATCH] [IBBUS][WinVerbs][SDP] Validation ofnon-zerosizewhen calling to ExAllocatePoolWithTag

[Fab Tillier]

Hi Tzachi,

Tzachi Dar wrote on Tue, 27 Oct 2009 at 03:44:45:

> Let me explain where the whole issue started:
> 
> There is a very common coding practice (on windows only?) that is used
> when the amount of memory needed is not known: You call the function
> with zero bytes, get an error of low resources but also the amount of
> memory that should be used. Next you allocate the needed memory and call
> the function again.
> 
> An example to that behavior is the function:DWORD GetAdaptersInfo(
> PIP_ADAPTER_INFO pAdapterInfo,  PULONG OutBufLen);
> 
> The second parameter is described as: pOutBufLen [in, out] A pointer to
> a ULONG variable that specifies the size of the buffer pointed to by the
> pAdapterInfo parameter. If this size is insufficient to hold the adapter
> information, GetAdaptersInfo fills in this variable with the required
> size, and returns an error code of ERROR_BUFFER_OVERFLOW.
> 
> An example to this behavior can be found in our code (for example) at
> the function WmQueryCaAttributes:
> 
> static ib_ca_attr_t *WmQueryCaAttributes(WM_IB_DEVICE *pDevice) {
> 	ib_ca_attr_t	*attr; 	UINT32			size; 	ib_api_status_t	ib_status;
> 
> 	size = 0;
> 	ib_status = pDevice->VerbsInterface.Verbs.
> 
> query_ca(pDevice->VerbsInterface.Verbs.p_hca_obj, NULL, &size, NULL);
> 	if (ib_status != IB_INSUFFICIENT_MEMORY) {

Add a check for size == 0 here and your problem is solved.  However, you point out a deficiency in the return codes: there's no way to distinguish between internal memory allocation failure and the user providing too small a buffer.  The Windows APIs tend to use STATUS_BUFFER_OVERFLOW vs. STATUS_NO_MEMORY to distinguish between the two.  Perhaps doing something similar here would help?  In the simplistic case, returning IB_ERROR for internal memory allocation failure would do the trick, so that IB_INSUFFICIENT_MEMORY would only be returned if size was set to a valid value.

Why does query_ca try to allocate memory internally if the specified buffer is NULL?  Seems like it should avoid that if it could.

-Fab

> 		attr = NULL;
> 		goto out;
> 	}
> 
> 	attr = ExAllocatePoolWithTag(PagedPool, size, 'acmw');
> 	if (attr == NULL) {
> 		goto out;
> 	}
> 
> The function query_ca is being called with a NULL pointer and size =0 in
> order to receive the correct amount of memory. Next we check what
> happens if (ib_status == IB_INSUFFICIENT_MEMORY). We treat this as a
> legal return value. But please note that this value can mean two things:
> 	1) The function has returned as expected and all is well. Since the out
> value was NULL it didn't write the result but size points to the amount
> of memory that should be allocated. 	2) (this is the case that the
> verifier has caught) The function was running. When it needed to
> allocate memory it failed. It now returns with status ==
> IB_INSUFFICIENT_MEMORY and size ==0. This is a legal value for the
> query_ca() function.
> 
> Please note that case #2 will cause ExAllocatePoolWithTag to be called
> with size ==0 and from here is the bugcheck.
> 
> So, this is the problem and asserting doesn't help since everything is
> according to the specs, and this can happen.
> 
> Please also note that according to
> http://bytes.com/topic/c/answers/508471-malloc-0-a malloc of 0 bytes is
> legal but is implementation dependent while calling
> ExAllocatePoolWithTag with 0 brings a bugcheck.
> 
> =======================================================================
> = ===========
> 
> That said, we need to think of how we solve the problem ...
> 
> Xalex approach was trying to limit the amount of work that will be used
> to make sure that 0 bytes are never allocated. That is, instead of going
> all over the places where ExAllocatePoolWithTag was used and trying to
> verify that 0 was not used, he was trying to make the function
> ExAllocatePoolWithTag behave like malloc.
> 
> Thoughts ...
> 
> Thanks
> Tzachi
> 
> 
>> -----Original Message-----
>> From: ofw-bounces at lists.openfabrics.org
>> [mailto:ofw-bounces at lists.openfabrics.org] On Behalf Of Sean Hefty
>> Sent: Monday, October 26, 2009 9:05 PM
>> To: Alex Naslednikov; Fab Tillier; ofw at lists.openfabrics.org;
>> Leonid Keller
>> Subject: RE: [ofw][PATCH] [IBBUS][WinVerbs][SDP] Validation
>> ofnon-zerosizewhen calling to ExAllocatePoolWithTag
>> 
>>> I got your point. The problem is that we received zero-size only when
>>> running verifier with a low-mem simulation. It's not a normal
>>> situation, but our code isn't ready for this case. Take into account
>>> that only "problemmatic" calls to ExAllocatePoolWithTag were replaced;
>>> i.e all call were chances to get zero-size > 0. ASSERT may be good for
>>> a check version, but I don't believe we can find and eliminate all the
>>> problemmatic cases by doing so.
>> 
>> IMO, this is too broad of a fix and in the wrong place.
>> Replacing standard Windows calls with a macro that looks like
>> a Windows call just obscures what's happening and increases
>> maintenance costs.  At this point, we don't know if the call
>> to malloc is wrong, or if some other value is being returned
>> to the code that calls malloc incorrectly.
>> 
>> I would expect a fix for a low-memory situation to add a
>> check that malloc worked, versus a check to see if malloc is
>> called with a size of 0.  To me, this is pointing to an issue
>> in some other piece of code that isn't being modified by this patch.
>> 
>> - Sean
>> 
>> _______________________________________________
>> ofw mailing list
>> ofw at lists.openfabrics.org
>> http://lists.openfabrics.org/cgi-bin/mailman/listinfo/ofw
>>