Skip to contentSkip to conversation

ACPICA: Resource Mgr: Prevent infinite loops in resource walks

DISCUSSION Dan Carpenter 2 participants 4 messages Oct 17, 2013 – Oct 23, 2013

Subsystems (2)
ACPI ACPI component architecture (acpica)
lore
Share & tools
lore

Dan Carpenter

author
#0 lore raw

Hello Bob Moore,

The patch c13085e519e8: "ACPICA: Resource Mgr: Prevent infinite loops in resource walks" from Mar 8, 2013 is not beautiful. My static checker complains about the loop because:
"drivers/acpi/acpica/rscalc.c:197 acpi_rs_get_aml_length()
warn: 'resource' can't be NULL."

drivers/acpi/acpica/rscalc.c
195 /* Traverse entire list of internal resource descriptors */
196

   197          while (resource) {
                       ^^^^^^^^

My static checker is wrong because we use the -fno-strict-overflow to prevent GCC from optimizing this check away. But we are looping over a list of pointers until our pointer wraps to NULL. In other words we loop over all the 2**64 - 1 addresses until we wrap to NULL or we find something with an invalid type or something with ->length zero.

I assume the last element in the list always has length zero? If so then we could replace "while (resource)" with "while (resource->length)"

   198  
   199                  /* Validate the descriptor type */
   200  
   201                  if (resource->type > ACPI_RESOURCE_TYPE_MAX) {
   202                          return_ACPI_STATUS(AE_AML_INVALID_RESOURCE_TYPE);
   203                  }
   204  
   205                  /* Sanity check the length. It must not be zero, or we loop forever */
   206  
   207                  if (!resource->length) {
   208                          return_ACPI_STATUS(AE_AML_BAD_RESOURCE_LENGTH);
   209                  }
   210  
   211                  /* Get the base size of the (external stream) resource descriptor */
   212

regards,
dan carpenter

#1 lore raw

We'll take a look.
Thanks,
Bob

Quoted 54 lines

-----Original Message-----
From: Dan Carpenter [mailto:dan.carpenter(a)oracle.com]
Sent: Thursday, October 17, 2013 5:29 AM
To: Moore, Robert
Cc: linux-acpi(a)vger.kernel.org; devel(a)acpica.org
Subject: re: ACPICA: Resource Mgr: Prevent infinite loops in resource walks

Hello Bob Moore,

The patch c13085e519e8: "ACPICA: Resource Mgr: Prevent infinite loops in resource walks" from Mar 8, 2013 is not beautiful. My static checker complains about the loop because:
"drivers/acpi/acpica/rscalc.c:197 acpi_rs_get_aml_length()
warn: 'resource' can't be NULL."

drivers/acpi/acpica/rscalc.c
195 /* Traverse entire list of internal resource descriptors
*/
196

   197          while (resource) {
                       ^^^^^^^^

My static checker is wrong because we use the -fno-strict-overflow to prevent GCC from optimizing this check away. But we are looping over a list of pointers until our pointer wraps to NULL. In other words we loop over all the 2**64 - 1 addresses until we wrap to NULL or we find something with an invalid type or something with ->length zero.

I assume the last element in the list always has length zero? If so then we could replace "while (resource)" with "while (resource->length)"

   198
   199                  /* Validate the descriptor type */
   200
   201                  if (resource->type > ACPI_RESOURCE_TYPE_MAX) {
   202
return_ACPI_STATUS(AE_AML_INVALID_RESOURCE_TYPE);
   203                  }
   204
   205                  /* Sanity check the length. It must not be zero,
or we loop forever */
   206
   207                  if (!resource->length) {
   208
return_ACPI_STATUS(AE_AML_BAD_RESOURCE_LENGTH);
   209                  }
   210
   211                  /* Get the base size of the (external stream)

resource descriptor */
212

regards,
dan carpenter

#2 lore raw

The last resource should always be ACPI_RESOURCE_TYPE_END_TAG and we have nsrepair mechanism to ensure that this entry is always there for resource based properties (there are still 2 kinds of properties haven't been fixed yet). If we do not want to rely on this logic, then we may need to use buffer->length and pass this one to the acpi_rs_get_aml_length to avoid potential infinite loop.
I'll draft an ACPICA patch to achieve the latter.

Thanks and best regards
-Lv

Quoted 71 lines

-----Original Message-----
From: devel-bounces(a)acpica.org [mailto:devel-bounces(a)acpica.org] On Behalf Of Moore, Robert
Sent: Wednesday, October 23, 2013 5:36 AM
To: Dan Carpenter
Cc: linux-acpi(a)vger.kernel.org; devel(a)acpica.org
Subject: Re: [Devel] ACPICA: Resource Mgr: Prevent infinite loops in resource walks

We'll take a look.
Thanks,
Bob

Quoted 49 lines

-----Original Message-----
From: Dan Carpenter [mailto:dan.carpenter(a)oracle.com]
Sent: Thursday, October 17, 2013 5:29 AM
To: Moore, Robert
Cc: linux-acpi(a)vger.kernel.org; devel(a)acpica.org
Subject: re: ACPICA: Resource Mgr: Prevent infinite loops in resource walks

Hello Bob Moore,

The patch c13085e519e8: "ACPICA: Resource Mgr: Prevent infinite loops in resource walks" from Mar 8, 2013 is not beautiful. My static checker complains about the loop because:
"drivers/acpi/acpica/rscalc.c:197 acpi_rs_get_aml_length()
warn: 'resource' can't be NULL."

drivers/acpi/acpica/rscalc.c
195 /* Traverse entire list of internal resource descriptors
*/
196

   197          while (resource) {
                       ^^^^^^^^

My static checker is wrong because we use the -fno-strict-overflow to prevent GCC from optimizing this check away. But we are looping over a list of pointers until our pointer wraps to NULL. In other words we loop over all the 2**64 - 1 addresses until we wrap to NULL or we find something with an invalid type or something with ->length zero.

I assume the last element in the list always has length zero? If so then we could replace "while (resource)" with "while (resource->length)"

   198
   199                  /* Validate the descriptor type */
   200
   201                  if (resource->type > ACPI_RESOURCE_TYPE_MAX) {
   202
return_ACPI_STATUS(AE_AML_INVALID_RESOURCE_TYPE);
   203                  }
   204
   205                  /* Sanity check the length. It must not be zero,
or we loop forever */
   206
   207                  if (!resource->length) {
   208
return_ACPI_STATUS(AE_AML_BAD_RESOURCE_LENGTH);
   209                  }
   210
   211                  /* Get the base size of the (external stream)
Quoted 5 lines

resource descriptor */
212

regards,
dan carpenter

_______________________________________________
Devel mailing list
Devel(a)acpica.org
https://lists.acpica.org/mailman/listinfo/devel

author
#3 lore raw
Quoted 9 lines · Lv

The last resource should always be ACPI_RESOURCE_TYPE_END_TAG and we have nsrepair mechanism to ensure that this entry is always there for resource based properties (there are still 2 kinds of properties haven't been fixed yet).

If we do not want to rely on this logic, then we may need to use buffer->length and pass this one to the acpi_rs_get_aml_length to avoid potential infinite loop.
I'll draft an ACPICA patch to achieve the latter.

Actually couldn't we just change the loop to:

	while (resource->type != ACPI_RESOURCE_TYPE_END_TAG) {

That would be my preference (as an ignorant third party).

regards,
dan carpenter