Re: [PATCH v5 10/27] cxl: harden resource_contains checks to handle zero size resources

[Alejandro Lucero Palau <alucerop@amd.com>]

On 11/21/24 21:00, Alison Schofield wrote:
> On Thu, Nov 21, 2024 at 09:22:33AM +0000, Alejandro Lucero Palau wrote:
>> On 11/21/24 02:46, Alison Schofield wrote:
>>> On Mon, Nov 18, 2024 at 04:44:17PM +0000, alejandro.lucero-palau@amd.com wrote:
>>>> From: Alejandro Lucero <alucerop@amd.com>
>>>>
>>>> For a resource defined with size zero, resource_contains returns
>>>> always true.
>>>>
>>> I'm not following the premise above -
>>>
>>> Looking at resource_contains() and the changes made below,
>>> it seems the concern is with &cxlds->ram_res or &cxlds->pmem_res
>>> being zero - because we already checked that the second param
>>> 'res' is not zero a few lines above.
>>>
>>> Looking at what happens when r1 is of size 0, I don't see how
>>> resource_contains() returns always true.
>>>
>>> In resource_contains(r1, r2), if r1 is of size 0, r1->start == r1->end.
>>> The func can only return true if r2 is also of size 0 and located at
>>> exactly r1->start. But, in this case, we are not going to get there
>>> because we never send an r2 of size 0.
>>>
>>> For any non-zero size r2 the func will always return false because
>>> the size 0 r1 cannot encompass any range.
>>>
>>> I could be misreading it all ;)
>>
>> The key is to know how a resource with size 0 is initialized, what can be
>> understood looking at DEFINE_RES_NAMED macro. The end field is set as  size
>> - 1.
>>
>> With unsigned variables, as it is the case here, it means to have a resource
>> as big as possible ... if you do not check first the size is not 0.
>>
>> The pmem resource is explicitly initialized inside cxl_accel_state_create in
>> the previous patch, so it has:
>>
>> pmem_res->start = 0, pmem_res.end = 0xffffffffffffffff
>>
>> the resource checked against is defined with, for example, a 256MB size:
>>
>> res.start =0, res.end = 0xfffffff
>>
>>
>> if you then use resource_contains(pmem_res, res), that implies always true,
>> whatever the res range defined.
>>
>>
>> All this confused me as well when facing it initially. I hope this
>> explanation makes sense.
>>
> Thanks for the explanation! I'm wondering if we are leaving a trap for the next
> developer.
>
> resource_contains() seems to have intended that a check for IORESOURCE_UNSET
> would take care of the zero size case:
>
> (5edb93b89f6c resource: Add resource_contains)
>
> and it would if folks used _UNSET. Some check r1->start before calling
> resource_contains().
>
> One option would be to use _UNSET in this case, but that only covers us here,
> and doesn't remove the trap ;)
>
> How about hardening resource_contains():
>
> ie: make resource_contains() return false if either res empty
>
>   /* True iff r1 completely contains r2 */
>   static inline bool resource_contains(const struct resource *r1, const struct resource *r2)
>   {
> +       if (!resource_size(r1) || !resource_size(r2))
> +               return false;
>          if (resource_type(r1) != resource_type(r2))
>                  return false;
>          if (r1->flags & IORESOURCE_UNSET || r2->flags & IORESOURCE_UNSET)
> 		return false;
> 	return r1->start <= r2->start && r1->end >= r2->end;
> }


I can try that. If there is a good reason for not hardening it, we will 
know.

Thanks!


> -- Alison
>
>>> --Alison
>>>
>>>
>>>> Add resource size check before using it.
>>>>
>>>> Signed-off-by: Alejandro Lucero <alucerop@amd.com>
>>>> ---
>>>>    drivers/cxl/core/hdm.c | 7 +++++--
>>>>    1 file changed, 5 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/cxl/core/hdm.c b/drivers/cxl/core/hdm.c
>>>> index 223c273c0cd1..c58d6b8f9b58 100644
>>>> --- a/drivers/cxl/core/hdm.c
>>>> +++ b/drivers/cxl/core/hdm.c
>>>> @@ -327,10 +327,13 @@ static int __cxl_dpa_reserve(struct cxl_endpoint_decoder *cxled,
>>>>    	cxled->dpa_res = res;
>>>>    	cxled->skip = skipped;
>>>> -	if (resource_contains(&cxlds->pmem_res, res))
>>>> +	if (resource_size(&cxlds->pmem_res) &&
>>>> +	    resource_contains(&cxlds->pmem_res, res)) {
>>>>    		cxled->mode = CXL_DECODER_PMEM;
>>>> -	else if (resource_contains(&cxlds->ram_res, res))
>>>> +	} else if (resource_size(&cxlds->ram_res) &&
>>>> +		   resource_contains(&cxlds->ram_res, res)) {
>>>>    		cxled->mode = CXL_DECODER_RAM;
>>>> +	}
>>>>    	else {
>>>>    		dev_warn(dev, "decoder%d.%d: %pr mixed mode not supported\n",
>>>>    			 port->id, cxled->cxld.id, cxled->dpa_res);
>>>> -- 
>>>> 2.17.1
>>>>
>>>>