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

[Tzachi Dar]

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) {
		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
>