Re: svn commit: r218685 - head/sys/dev/acpica
On Monday 14 February 2011 01:33 pm, John Baldwin wrote: > On Monday, February 14, 2011 12:20:20 pm Matthew D Fleming wrote: > > Author: mdf > > Date: Mon Feb 14 17:20:20 2011 > > New Revision: 218685 > > URL: http://svn.freebsd.org/changeset/base/218685 > > > > Log: > > Prevent reading from the ACPI_RESOURCE past its actual end. > > For paranoia limit to the size of the ACPI_RESOURCE as well. > > I think in practice that len would never be > > sizeof(ACPI_RESOURCE). > > You could probably get by with using a KASSERT() instead: > > KASSERT(res->Length <= sizeof(ACPI_RESOURCE), "resource too > large")); bcopy(res, req->acpi_res, res->Length); We should avoid sizeof(ACPI_RESOURCE). If you really have to know size of a specific resource type, there is a convenience macro, i.e., ACPI_RS_SIZE(type). Jung-uk Kim ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r218685 - head/sys/dev/acpica
On Mon, Feb 14, 2011 at 10:33 AM, John Baldwin wrote: > On Monday, February 14, 2011 12:20:20 pm Matthew D Fleming wrote: >> Author: mdf >> Date: Mon Feb 14 17:20:20 2011 >> New Revision: 218685 >> URL: http://svn.freebsd.org/changeset/base/218685 >> >> Log: >> Prevent reading from the ACPI_RESOURCE past its actual end. For >> paranoia limit to the size of the ACPI_RESOURCE as well. > > I think in practice that len would never be > sizeof(ACPI_RESOURCE). > > You could probably get by with using a KASSERT() instead: > > KASSERT(res->Length <= sizeof(ACPI_RESOURCE), "resource too large")); > bcopy(res, req->acpi_res, res->Length); Thanks. I wanted to be paranoid since the problem was sporadic. Anyone who can better test this code should feel free to modify it further. Thanks, matthew ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r218685 - head/sys/dev/acpica
On Monday, February 14, 2011 12:20:20 pm Matthew D Fleming wrote: > Author: mdf > Date: Mon Feb 14 17:20:20 2011 > New Revision: 218685 > URL: http://svn.freebsd.org/changeset/base/218685 > > Log: > Prevent reading from the ACPI_RESOURCE past its actual end. For > paranoia limit to the size of the ACPI_RESOURCE as well. I think in practice that len would never be > sizeof(ACPI_RESOURCE). You could probably get by with using a KASSERT() instead: KASSERT(res->Length <= sizeof(ACPI_RESOURCE), "resource too large")); bcopy(res, req->acpi_res, res->Length); -- John Baldwin ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
Re: svn commit: r218685 - head/sys/dev/acpica
On Monday 14 February 2011 12:20 pm, Matthew D Fleming wrote:
> Author: mdf
> Date: Mon Feb 14 17:20:20 2011
> New Revision: 218685
> URL: http://svn.freebsd.org/changeset/base/218685
>
> Log:
> Prevent reading from the ACPI_RESOURCE past its actual end. For
> paranoia limit to the size of the ACPI_RESOURCE as well.
>
> Reviewd by: jhb (in spirit)
> MFC after: 1 week
>
> Modified:
> head/sys/dev/acpica/acpi_resource.c
>
> Modified: head/sys/dev/acpica/acpi_resource.c
> ===
>=== --- head/sys/dev/acpica/acpi_resource.cMon Feb 14
> 16:54:03 2011 (r218684) +++ head/sys/dev/acpica/acpi_resource.c Mon
> Feb 14 17:20:20 2011 (r218685) @@ -60,6 +60,7 @@ static ACPI_STATUS
> acpi_lookup_irq_handler(ACPI_RESOURCE *res, void *context)
> {
> struct lookup_irq_request *req;
> +size_t len;
> u_int irqnum, irq;
>
> switch (res->Type) {
> @@ -82,7 +83,10 @@ acpi_lookup_irq_handler(ACPI_RESOURCE *r
> req->found = 1;
> KASSERT(irq == rman_get_start(req->res),
> ("IRQ resources do not match"));
> - bcopy(res, req->acpi_res, sizeof(ACPI_RESOURCE));
> + len = res->Length;
> + if (len > sizeof(ACPI_RESOURCE))
> + len = sizeof(ACPI_RESOURCE);
> + bcopy(res, req->acpi_res, len);
> return (AE_CTRL_TERMINATE);
> }
> return (AE_OK);
Hmm... I am not sure this is a correct fix. For most cases, directly
using sizeof(ACPI_RESOURCE) is evil as it does not reflect actual
size of underlying structure. With the same reason,
sizeof(ACPI_RESOURCE_IRQ) and sizeof(ACPI_RESOURCE_EXTENDED_IRQ) is
not recommended, either. A correct fix is to extend
acpi_lookup_irq_resource() to allocate necessary space dynamically.
Jung-uk Kim
___
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
svn commit: r218685 - head/sys/dev/acpica
Author: mdf
Date: Mon Feb 14 17:20:20 2011
New Revision: 218685
URL: http://svn.freebsd.org/changeset/base/218685
Log:
Prevent reading from the ACPI_RESOURCE past its actual end. For
paranoia limit to the size of the ACPI_RESOURCE as well.
Reviewd by: jhb (in spirit)
MFC after:1 week
Modified:
head/sys/dev/acpica/acpi_resource.c
Modified: head/sys/dev/acpica/acpi_resource.c
==
--- head/sys/dev/acpica/acpi_resource.c Mon Feb 14 16:54:03 2011
(r218684)
+++ head/sys/dev/acpica/acpi_resource.c Mon Feb 14 17:20:20 2011
(r218685)
@@ -60,6 +60,7 @@ static ACPI_STATUS
acpi_lookup_irq_handler(ACPI_RESOURCE *res, void *context)
{
struct lookup_irq_request *req;
+size_t len;
u_int irqnum, irq;
switch (res->Type) {
@@ -82,7 +83,10 @@ acpi_lookup_irq_handler(ACPI_RESOURCE *r
req->found = 1;
KASSERT(irq == rman_get_start(req->res),
("IRQ resources do not match"));
- bcopy(res, req->acpi_res, sizeof(ACPI_RESOURCE));
+ len = res->Length;
+ if (len > sizeof(ACPI_RESOURCE))
+ len = sizeof(ACPI_RESOURCE);
+ bcopy(res, req->acpi_res, len);
return (AE_CTRL_TERMINATE);
}
return (AE_OK);
___
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to "svn-src-all-unsubscr...@freebsd.org"
