[PATCH] ACPI: fix vendor resource length computation

[PATCH] ACPI: fix vendor resource length computation

[Bjorn Helgaas]

acpi_rs_get_list_length() needs to account for all the vendor-defined
data bytes.  Failing to include these causes buffers to be sized too
small, which causes slab corruption when we later convert AML to
resources and run off the end of the buffer.

I'm no expert on this code, so please scrutinize this carefully.

This causes slab corruption on machines that use ACPI vendor-defined
resources.  All HP ia64 machines do, and I'm told that some NEC
machines may as well.  So if the fix is correct, it would be good
to have it in 2.6.16.

Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>

Index: work-mm4/drivers/acpi/resources/rscalc.c
===================================================================
--- work-mm4.orig/drivers/acpi/resources/rscalc.c	2006-02-14 13:32:50.000000000 -0700
+++ work-mm4/drivers/acpi/resources/rscalc.c	2006-02-14 13:33:25.000000000 -0700
@@ -391,8 +391,7 @@
 			 * Ensure a 32-bit boundary for the structure
 			 */
 			extra_struct_bytes =
-			    ACPI_ROUND_UP_to_32_bITS(resource_length) -
-			    resource_length;
+			    ACPI_ROUND_UP_to_32_bITS(resource_length);
 			break;
 
 		case ACPI_RESOURCE_NAME_END_TAG:
@@ -408,8 +407,7 @@
 			 * Add vendor data and ensure a 32-bit boundary for the structure
 			 */
 			extra_struct_bytes =
-			    ACPI_ROUND_UP_to_32_bITS(resource_length) -
-			    resource_length;
+			    ACPI_ROUND_UP_to_32_bITS(resource_length);
 			break;
 
 		case ACPI_RESOURCE_NAME_ADDRESS32:

Re: [PATCH] ACPI: fix vendor resource length computation

[Bjorn Helgaas]

On Tuesday 14 February 2006 16:13, Bjorn Helgaas wrote:
> acpi_rs_get_list_length() needs to account for all the vendor-defined
> data bytes.  Failing to include these causes buffers to be sized too
> small, which causes slab corruption when we later convert AML to
> resources and run off the end of the buffer.
> 
> I'm no expert on this code, so please scrutinize this carefully.
> 
> This causes slab corruption on machines that use ACPI vendor-defined
> resources.  All HP ia64 machines do, and I'm told that some NEC
> machines may as well.  So if the fix is correct, it would be good
> to have it in 2.6.16.
> 
> Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>

I forgot to mention that this patch may be used under either the GPL
or the BSD license used for the ACPI CA.

> Index: work-mm4/drivers/acpi/resources/rscalc.c
> ===================================================================
> --- work-mm4.orig/drivers/acpi/resources/rscalc.c	2006-02-14 13:32:50.000000000 -0700
> +++ work-mm4/drivers/acpi/resources/rscalc.c	2006-02-14 13:33:25.000000000 -0700
> @@ -391,8 +391,7 @@
>  			 * Ensure a 32-bit boundary for the structure
>  			 */
>  			extra_struct_bytes =
> -			    ACPI_ROUND_UP_to_32_bITS(resource_length) -
> -			    resource_length;
> +			    ACPI_ROUND_UP_to_32_bITS(resource_length);
>  			break;
>  
>  		case ACPI_RESOURCE_NAME_END_TAG:
> @@ -408,8 +407,7 @@
>  			 * Add vendor data and ensure a 32-bit boundary for the structure
>  			 */
>  			extra_struct_bytes =
> -			    ACPI_ROUND_UP_to_32_bITS(resource_length) -
> -			    resource_length;
> +			    ACPI_ROUND_UP_to_32_bITS(resource_length);
>  			break;
>  
>  		case ACPI_RESOURCE_NAME_ADDRESS32:
> -
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

RE: [PATCH] ACPI: fix vendor resource length computation

[Moore, Robert]

I'll look at this. 

Does this fix all of the slab corruptions issues we have been hearing
about?

Bob


> -----Original Message-----
> From: Bjorn Helgaas [mailto:bjorn.helgaas@hp.com]
> Sent: Tuesday, February 14, 2006 3:13 PM
> To: Andreas Schwab
> Cc: Thomas Renninger; Moore, Robert; Luck, Tony; Brown, Len; linux-
> acpi@vger.kernel.org; linux-ia64@vger.kernel.org; Andrew Morton;
> efocht@hpce.nec.com
> Subject: [PATCH] ACPI: fix vendor resource length computation
> 
> acpi_rs_get_list_length() needs to account for all the vendor-defined
> data bytes.  Failing to include these causes buffers to be sized too
> small, which causes slab corruption when we later convert AML to
> resources and run off the end of the buffer.
> 
> I'm no expert on this code, so please scrutinize this carefully.
> 
> This causes slab corruption on machines that use ACPI vendor-defined
> resources.  All HP ia64 machines do, and I'm told that some NEC
> machines may as well.  So if the fix is correct, it would be good
> to have it in 2.6.16.
> 
> Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
> 
> Index: work-mm4/drivers/acpi/resources/rscalc.c
> ===================================================================
> --- work-mm4.orig/drivers/acpi/resources/rscalc.c	2006-02-14
> 13:32:50.000000000 -0700
> +++ work-mm4/drivers/acpi/resources/rscalc.c	2006-02-14
> 13:33:25.000000000 -0700
> @@ -391,8 +391,7 @@
>  			 * Ensure a 32-bit boundary for the structure
>  			 */
>  			extra_struct_bytes =
> -			    ACPI_ROUND_UP_to_32_bITS(resource_length) -
> -			    resource_length;
> +			    ACPI_ROUND_UP_to_32_bITS(resource_length);
>  			break;
> 
>  		case ACPI_RESOURCE_NAME_END_TAG:
> @@ -408,8 +407,7 @@
>  			 * Add vendor data and ensure a 32-bit boundary
for the
> structure
>  			 */
>  			extra_struct_bytes =
> -			    ACPI_ROUND_UP_to_32_bITS(resource_length) -
> -			    resource_length;
> +			    ACPI_ROUND_UP_to_32_bITS(resource_length);
>  			break;
> 
>  		case ACPI_RESOURCE_NAME_ADDRESS32:

Re: [PATCH] ACPI: fix vendor resource length computation

[Bjorn Helgaas]

On Tuesday 14 February 2006 16:25, Moore, Robert wrote:
> Does this fix all of the slab corruptions issues we have been hearing
> about?

It fixes the one I saw with 2.6.16-rc3 on HP rx7620.  I'm pretty sure
that's the same as the one Thomas observed in pci_acpi_scan_root() and
acpi_sba_ioc_add().

But I haven't been paying attention to any other slab corruption issues,
so I can't speculate about them.

> > -----Original Message-----
> > From: Bjorn Helgaas [mailto:bjorn.helgaas@hp.com]
> > Sent: Tuesday, February 14, 2006 3:13 PM
> > To: Andreas Schwab
> > Cc: Thomas Renninger; Moore, Robert; Luck, Tony; Brown, Len; linux-
> > acpi@vger.kernel.org; linux-ia64@vger.kernel.org; Andrew Morton;
> > efocht@hpce.nec.com
> > Subject: [PATCH] ACPI: fix vendor resource length computation
> > 
> > acpi_rs_get_list_length() needs to account for all the vendor-defined
> > data bytes.  Failing to include these causes buffers to be sized too
> > small, which causes slab corruption when we later convert AML to
> > resources and run off the end of the buffer.
> > 
> > I'm no expert on this code, so please scrutinize this carefully.
> > 
> > This causes slab corruption on machines that use ACPI vendor-defined
> > resources.  All HP ia64 machines do, and I'm told that some NEC
> > machines may as well.  So if the fix is correct, it would be good
> > to have it in 2.6.16.
> > 
> > Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
> > 
> > Index: work-mm4/drivers/acpi/resources/rscalc.c
> > ===================================================================
> > --- work-mm4.orig/drivers/acpi/resources/rscalc.c	2006-02-14
> > 13:32:50.000000000 -0700
> > +++ work-mm4/drivers/acpi/resources/rscalc.c	2006-02-14
> > 13:33:25.000000000 -0700
> > @@ -391,8 +391,7 @@
> >  			 * Ensure a 32-bit boundary for the structure
> >  			 */
> >  			extra_struct_bytes =
> > -			    ACPI_ROUND_UP_to_32_bITS(resource_length) -
> > -			    resource_length;
> > +			    ACPI_ROUND_UP_to_32_bITS(resource_length);
> >  			break;
> > 
> >  		case ACPI_RESOURCE_NAME_END_TAG:
> > @@ -408,8 +407,7 @@
> >  			 * Add vendor data and ensure a 32-bit boundary
> for the
> > structure
> >  			 */
> >  			extra_struct_bytes =
> > -			    ACPI_ROUND_UP_to_32_bITS(resource_length) -
> > -			    resource_length;
> > +			    ACPI_ROUND_UP_to_32_bITS(resource_length);
> >  			break;
> > 
> >  		case ACPI_RESOURCE_NAME_ADDRESS32:
> 

RE: [PATCH] ACPI: fix vendor resource length computation

[Moore, Robert]

I think the original code may have been attempting to subtract off the
size of the descriptor header to obtain the length of the vendor data
bytes. However, something like a global replace may have broken it.



> -----Original Message-----
> From: Bjorn Helgaas [mailto:bjorn.helgaas@hp.com]
> Sent: Tuesday, February 14, 2006 3:13 PM
> To: Andreas Schwab
> Cc: Thomas Renninger; Moore, Robert; Luck, Tony; Brown, Len; linux-
> acpi@vger.kernel.org; linux-ia64@vger.kernel.org; Andrew Morton;
> efocht@hpce.nec.com
> Subject: [PATCH] ACPI: fix vendor resource length computation
> 
> acpi_rs_get_list_length() needs to account for all the vendor-defined
> data bytes.  Failing to include these causes buffers to be sized too
> small, which causes slab corruption when we later convert AML to
> resources and run off the end of the buffer.
> 
> I'm no expert on this code, so please scrutinize this carefully.
> 
> This causes slab corruption on machines that use ACPI vendor-defined
> resources.  All HP ia64 machines do, and I'm told that some NEC
> machines may as well.  So if the fix is correct, it would be good
> to have it in 2.6.16.
> 
> Signed-off-by: Bjorn Helgaas <bjorn.helgaas@hp.com>
> 
> Index: work-mm4/drivers/acpi/resources/rscalc.c
> ===================================================================
> --- work-mm4.orig/drivers/acpi/resources/rscalc.c	2006-02-14
> 13:32:50.000000000 -0700
> +++ work-mm4/drivers/acpi/resources/rscalc.c	2006-02-14
> 13:33:25.000000000 -0700
> @@ -391,8 +391,7 @@
>  			 * Ensure a 32-bit boundary for the structure
>  			 */
>  			extra_struct_bytes =
> -			    ACPI_ROUND_UP_to_32_bITS(resource_length) -
> -			    resource_length;
> +			    ACPI_ROUND_UP_to_32_bITS(resource_length);
>  			break;
> 
>  		case ACPI_RESOURCE_NAME_END_TAG:
> @@ -408,8 +407,7 @@
>  			 * Add vendor data and ensure a 32-bit boundary
for the
> structure
>  			 */
>  			extra_struct_bytes =
> -			    ACPI_ROUND_UP_to_32_bITS(resource_length) -
> -			    resource_length;
> +			    ACPI_ROUND_UP_to_32_bITS(resource_length);
>  			break;
> 
>  		case ACPI_RESOURCE_NAME_ADDRESS32:

Re: [PATCH] ACPI: fix vendor resource length computation

[Thomas Renninger]

Moore, Robert wrote:
> I think the original code may have been attempting to subtract off the
> size of the descriptor header to obtain the length of the vendor data
> bytes. However, something like a global replace may have broken it.
> 
Thanks, this one fixes the mem corruption.
Unaligned access messages still remain...


   Thomas

RE: [PATCH] ACPI: fix vendor resource length computation

[Moore, Robert]

I'm working on the alignment issue also, should be a fix this week.
Bob


> -----Original Message-----
> From: Thomas Renninger [mailto:trenn@suse.de]
> Sent: Wednesday, February 15, 2006 9:50 AM
> To: Moore, Robert
> Cc: Bjorn Helgaas; Andreas Schwab; Luck, Tony; Brown, Len; linux-
> acpi@vger.kernel.org; linux-ia64@vger.kernel.org; Andrew Morton;
> efocht@hpce.nec.com
> Subject: Re: [PATCH] ACPI: fix vendor resource length computation
> 
> Moore, Robert wrote:
> > I think the original code may have been attempting to subtract off
the
> > size of the descriptor header to obtain the length of the vendor
data
> > bytes. However, something like a global replace may have broken it.
> >
> Thanks, this one fixes the mem corruption.
> Unaligned access messages still remain...
> 
> 
>    Thomas

RE: [PATCH] ACPI: fix vendor resource length computation

[Luck, Tony]

> I'm working on the alignment issue also, should be a fix this week.

With Bjorn's tip on using console=uart,mmio,0x{address} I can now
get a complete trace from boot using acpi_dbg_level = 0xffffff ...
well in theory I can, I started the boot two days ago and have
2.2 million lines of output so far, and it is still going!  Now
I'm just curious as to whether this will actually complete, but
if it doesn't do so in the next few days I'll have to abort the
boot and get back to using that system for regression testing on
other patches.

Sadly, I think that using the "console=uart,..." flag may have
caused the kernel to skip the bit of acpi probing that caused the
unaligned traps, as I don't see any "unaligned" messages in the
output so far ... so this might be a completely futile exercise.

-Tony

Re: [PATCH] ACPI: fix vendor resource length computation

[Thomas Renninger]

Luck, Tony wrote:
>> I'm working on the alignment issue also, should be a fix this week.
> 
> With Bjorn's tip on using console=uart,mmio,0x{address} I can now
> get a complete trace from boot using acpi_dbg_level = 0xffffff ...
> well in theory I can, I started the boot two days ago and have
> 2.2 million lines of output so far, and it is still going!  Now
> I'm just curious as to whether this will actually complete, but
> if it doesn't do so in the next few days I'll have to abort the
> boot and get back to using that system for regression testing on
> other patches.
> 
> Sadly, I think that using the "console=uart,..." flag may have
> caused the kernel to skip the bit of acpi probing that caused the
> unaligned traps, as I don't see any "unaligned" messages in the
> output so far ... so this might be a completely futile exercise.
> 
Better don't use the boot parameters. Try to identify at which point
the culprit might lie and then embed the code you like to
debug in the sources into:
acpi_dbg_level=0xXY (whatever debug level, avoid mutex and other
                        unnecessary stuff you don't need)
... bad code to debug

acpi_dbg_level=0xF

I expect the output will never end, there will pop up an acpi interrupt
or some polling is done by ACPI and you are trapped in endless output...

    Thomas

RE: [PATCH] ACPI: fix vendor resource length computation

[Moore, Robert]

I recently added a new interface to ACPICA to support this kind of
debugging. It enables/disables debug for a specific control method:

ACPI_STATUS
AcpiDebugTrace (
    char                    *Name,
    UINT32                  DebugLevel,
    UINT32                  DebugLayer,
    UINT32                  Flags);

It would be nice to see this exposed in Linux somehow.

Bob


> -----Original Message-----
> From: Thomas Renninger [mailto:trenn@suse.de]
> Sent: Thursday, February 16, 2006 12:59 AM
> To: Luck, Tony
> Cc: Moore, Robert; Bjorn Helgaas; Andreas Schwab; Brown, Len; linux-
> acpi@vger.kernel.org; linux-ia64@vger.kernel.org; Andrew Morton;
> efocht@hpce.nec.com
> Subject: Re: [PATCH] ACPI: fix vendor resource length computation
> 
> Luck, Tony wrote:
> >> I'm working on the alignment issue also, should be a fix this week.
> >
> > With Bjorn's tip on using console=uart,mmio,0x{address} I can now
> > get a complete trace from boot using acpi_dbg_level = 0xffffff ...
> > well in theory I can, I started the boot two days ago and have
> > 2.2 million lines of output so far, and it is still going!  Now
> > I'm just curious as to whether this will actually complete, but
> > if it doesn't do so in the next few days I'll have to abort the
> > boot and get back to using that system for regression testing on
> > other patches.
> >
> > Sadly, I think that using the "console=uart,..." flag may have
> > caused the kernel to skip the bit of acpi probing that caused the
> > unaligned traps, as I don't see any "unaligned" messages in the
> > output so far ... so this might be a completely futile exercise.
> >
> Better don't use the boot parameters. Try to identify at which point
> the culprit might lie and then embed the code you like to
> debug in the sources into:
> acpi_dbg_level=0xXY (whatever debug level, avoid mutex and other
>                         unnecessary stuff you don't need)
> ... bad code to debug
> 
> acpi_dbg_level=0xF
> 
> I expect the output will never end, there will pop up an acpi
interrupt
> or some polling is done by ACPI and you are trapped in endless
output...
> 
>     Thomas