Jürgen Keil wrote: > In usr/src/uts/i86pc/io/psm/psm_common.c function acpi_set_irq_resource(): > > http://cvs.opensolaris.org/source/xref/usr/src/uts/i86pc/io/psm/psm_common.c#acpi_set_irq_resource > > 490 /* > 491 * Find an IRQ resource descriptor to use as template > 492 */ > 493 srsp = NULL; > 494 for (resp = rsb.Pointer; resp->Length != 0; > 495 resp = ACPI_NEXT_RESOURCE(resp)) { > 496 switch (resp->Id) { > 497 case ACPI_RSTYPE_IRQ: > 498 case ACPI_RSTYPE_EXT_IRQ: > 499 /* > 500 * Mild trickery here; allocate two > 501 * resource structures, and set the Length > 502 * field of the second one to 0 to terminate > 503 * the list. Copy the possible resource into > 504 * the first one as a template. > 505 */ > 506 srsp = kmem_zalloc(2 * sizeof (*srsp), > KM_SLEEP); > 507 srsp[1].Length = 0; > 508 *srsp = *resp; > 509 break; > > > Does the "Mild trickery" work as documented in the comment? Does the code > set the Length field of the second resource structure in all cases? It > seems the resource buffer uses variable sized resource elements, not > fixed size elements!
I knew better when I wrote this ;-) > On as ASUS A7V, a resp->Id ACPI_RSTYPE_IRQ (0) entry is found, it has > a resp->Length of 0x44 bytes. But the size of the array elements > (sizeof ACPI_RESOURCE) is 0x5c. > > Doesn't the second entry start at offset 0x44 from the kmem_zalloc'ed > buffer, after the assignment "*srsp = *resp;" (because the actual resource > data doesn't use the full structure's size) ? Yup. I believe I'm guilty as charged. The reason this doesn't blow up, and probably won't ever blow up, is that this code is only used to set the IRQ for a PCI IRQ link device. The _SRS method for a PCI IRQ link device is only going to use the first item in the resource byte stream (constructed from the ACPI_RESOURCE structures). > The "*srsp = *resp;" assigment might copy part of the next resource buffer > that follows resp, possible including the ->Id and ->Length member. Right. I expect the general manifestation of this failure case is to simply reproduce the entire resource list that was fetched using AcpiGetPossibleResources(), which is benign and one of the reasons I believe this code isn't blowing up. Nonetheless, it's a bug and should be fixed. > Wouldn't it be better to use something like this: > > srsp = kmem_zalloc(2 * sizeof (*srsp), KM_SLEEP); > bcopy(resp, srsp, resp->Length); > ACPI_NEXT_RESOURCE(srsp)->Id = ACPI_RSTYPE_END_TAG; > ACPI_NEXT_RESOURCE(srsp)->Length = 0; > break; Yes, indeed. I'd be delighted to incorporate this contributed fix as a P3 bug. Sound good? Thanks, Dana [EMAIL PROTECTED] _______________________________________________ opensolaris-code mailing list [email protected] https://opensolaris.org:444/mailman/listinfo/opensolaris-code
