| Submitter | Sven Schnelle |
|---|---|
| Date | 2011-01-10 12:59:02 |
| Message ID | <87d3o4lxt5.fsf@begreifnix.stackframe.org> |
| Download | mbox | patch |
| Permalink | /patch/2504/ |
| State | New |
| Headers | show |
Comments
> diff --git a/src/devices/device_util.c b/src/devices/device_util.c > index 9081a36..d761cba 100644 > --- a/src/devices/device_util.c > +++ b/src/devices/device_util.c > @@ -583,8 +583,9 @@ void search_bus_resources(struct bus *bus, unsigned > long type_mask, > if (subbus->link_num > == IOINDEX_SUBTRACTIVE_LINK(res->index)) > break; > - search_bus_resources(subbus, type_mask, type, > - search, gp); > + if (subbus) > + search_bus_resources(subbus, type_mask, > type, > + search, gp); > continue; > } > search(gp, curdev, res); If subbus is NULL, then accessing subbus->link_num is also a problem. Thanks, Myles
"Myles Watson" <mylesgw@gmail.com> writes: >> diff --git a/src/devices/device_util.c b/src/devices/device_util.c >> index 9081a36..d761cba 100644 >> --- a/src/devices/device_util.c >> +++ b/src/devices/device_util.c >> @@ -583,8 +583,9 @@ void search_bus_resources(struct bus *bus, unsigned >> long type_mask, >> if (subbus->link_num >> == > IOINDEX_SUBTRACTIVE_LINK(res->index)) >> break; >> - search_bus_resources(subbus, type_mask, > type, >> - search, gp); >> + if (subbus) >> + search_bus_resources(subbus, > type_mask, >> type, >> + search, gp); >> continue; >> } >> search(gp, curdev, res); > > If subbus is NULL, then accessing subbus->link_num is also a problem. That doesn't happen, because the if (subbus... is in the for loop, which checks for NULL. the search_bus_resources() is always called outside the for loop. current code: -----------------------8<------------------- if (res->flags & IORESOURCE_SUBTRACTIVE) { struct bus * subbus; for (subbus = curdev->link_list; subbus; subbus = subbus->next) if (subbus->link_num == IOINDEX_SUBTRACTIVE_LINK(res->index)) break; search_bus_resources(subbus, type_mask, type, search, gp); continue; } -----------------------8<------------------- it should be proably something like: -----------------------8<------------------- if (res->flags & IORESOURCE_SUBTRACTIVE) { struct bus * subbus; for (subbus = curdev->link_list; subbus; subbus = subbus->next) { if (subbus->link_num == IOINDEX_SUBTRACTIVE_LINK(res->index)) search_bus_resources(subbus, type_mask, type, search, gp); break; } continue; } -----------------------8<------------------- Regards, Sven.
Sven Schnelle <svens@stackframe.org> writes:
Missed a bracket, sorry:
if (res->flags & IORESOURCE_SUBTRACTIVE) {
struct bus * subbus;
for (subbus = curdev->link_list; subbus; subbus = subbus->next) {
if (subbus->link_num == IOINDEX_SUBTRACTIVE_LINK(res->index)) {
search_bus_resources(subbus, type_mask, type, search, gp);
break;
}
}
continue;
}
> it should be proably something like:
> -----------------------8<-------------------
>
> if (res->flags & IORESOURCE_SUBTRACTIVE) {
> struct bus * subbus;
> for (subbus = curdev->link_list; subbus; subbus = subbus->next) {
> if (subbus->link_num == IOINDEX_SUBTRACTIVE_LINK(res->index))
> search_bus_resources(subbus, type_mask, type, search, gp);
> break;
> }
> continue;
> }
> -----------------------8<-------------------
>
> Regards,
>
> Sven.
On Mon, Jan 10, 2011 at 6:27 AM, Sven Schnelle <svens@stackframe.org> wrote: > "Myles Watson" <mylesgw@gmail.com> writes: > >>> diff --git a/src/devices/device_util.c b/src/devices/device_util.c >>> index 9081a36..d761cba 100644 >>> --- a/src/devices/device_util.c >>> +++ b/src/devices/device_util.c >>> @@ -583,8 +583,9 @@ void search_bus_resources(struct bus *bus, unsigned >>> long type_mask, >>> if (subbus->link_num >>> == >> IOINDEX_SUBTRACTIVE_LINK(res->index)) >>> break; >>> - search_bus_resources(subbus, type_mask, >> type, >>> - search, gp); >>> + if (subbus) >>> + search_bus_resources(subbus, >> type_mask, >>> type, >>> + search, gp); >>> continue; >>> } >>> search(gp, curdev, res); >> >> If subbus is NULL, then accessing subbus->link_num is also a problem. > > That doesn't happen, because the if (subbus... is in the for loop, which > checks for NULL. the search_bus_resources() is always called outside the > for loop. You're right. I should have looked at the code first, instead of just the patch. There wasn't enough context. If there is no bus there, maybe the resource shouldn't be subtractive. Maybe we should print a message when that happens so that we can fix the problem. Thanks, Myles
* Myles Watson <mylesgw@gmail.com> [110110 14:29]: > On Mon, Jan 10, 2011 at 6:27 AM, Sven Schnelle <svens@stackframe.org> wrote: > > "Myles Watson" <mylesgw@gmail.com> writes: > > > >>> diff --git a/src/devices/device_util.c b/src/devices/device_util.c > >>> index 9081a36..d761cba 100644 > >>> --- a/src/devices/device_util.c > >>> +++ b/src/devices/device_util.c > >>> @@ -583,8 +583,9 @@ void search_bus_resources(struct bus *bus, unsigned > >>> long type_mask, > >>> if (subbus->link_num > >>> == > >> IOINDEX_SUBTRACTIVE_LINK(res->index)) > >>> break; > >>> - search_bus_resources(subbus, type_mask, > >> type, > >>> - search, gp); > >>> + if (subbus) > >>> + search_bus_resources(subbus, > >> type_mask, > >>> type, > >>> + search, gp); > >>> continue; > >>> } > >>> search(gp, curdev, res); > >> > >> If subbus is NULL, then accessing subbus->link_num is also a problem. > > > > That doesn't happen, because the if (subbus... is in the for loop, which > > checks for NULL. the search_bus_resources() is always called outside the > > for loop. > You're right. I should have looked at the code first, instead of just > the patch. There wasn't enough context. > > If there is no bus there, maybe the resource shouldn't be subtractive. > Maybe we should print a message when that happens so that we can fix > the problem. What would cause it to be subtractive? We should print an error message if this occurs, but be should also react sanely. How do we proceed?
>> > That doesn't happen, because the if (subbus... is in the for loop, which >> > checks for NULL. the search_bus_resources() is always called outside the >> > for loop. >> You're right. I should have looked at the code first, instead of just >> the patch. There wasn't enough context. >> >> If there is no bus there, maybe the resource shouldn't be subtractive. >> Maybe we should print a message when that happens so that we can fix >> the problem. > > What would cause it to be subtractive? We should print an error message > if this occurs, but be should also react sanely. How do we proceed? This has always been confusing to me. I don't know if we use it "correctly" anywhere in coreboot. My understanding is that subtractive resources should be used when there is nothing explicitly directing an address range to a specific bus or device. So, if an address arrives that doesn't match anything you have and there is a subtractive resource, pass it that way. I'd guess that it's left over from another era, but I'm not sure. Is there another use for subtractive resources? Thanks, Myles
> I'd guess that it's left over from another era, but I'm not sure. Is > there another use for subtractive resources? Well in general I think port 80 is good example of subtractive resource decoding. Thanks Rudolf
> > I'd guess that it's left over from another era, but I'm not sure. Is > > there another use for subtractive resources? > > Well in general I think port 80 is good example of subtractive resource > decoding. I agree. What I meant was... When should we declare subtractive resources in coreboot? Do we ever need them to be allocated, searched for, etc.? Because they're subtractive, they should just work, right? Thanks, Myles
* Myles Watson <mylesgw@gmail.com> [110116 04:42]: > > > I'd guess that it's left over from another era, but I'm not sure. Is > > > there another use for subtractive resources? > > > > Well in general I think port 80 is good example of subtractive resource > > decoding. > I agree. > > What I meant was... When should we declare subtractive resources in > coreboot? Do we ever need them to be allocated, searched for, etc.? I thought a bridge would be subtractive because it spans the window in which its children live. What does the subtractive code do in devices/ ? Eric Biederman would know... > Because they're subtractive, they should just work, right? > > Thanks, > Myles > > > -- > coreboot mailing list: coreboot@coreboot.org > http://www.coreboot.org/mailman/listinfo/coreboot >
Patch
diff --git a/src/devices/device_util.c b/src/devices/device_util.c index 9081a36..d761cba 100644 --- a/src/devices/device_util.c +++ b/src/devices/device_util.c @@ -583,8 +583,9 @@ void search_bus_resources(struct bus *bus, unsigned long type_mask, if (subbus->link_num == IOINDEX_SUBTRACTIVE_LINK(res->index)) break; - search_bus_resources(subbus, type_mask, type, - search, gp); + if (subbus) + search_bus_resources(subbus, type_mask, type, + search, gp); continue; } search(gp, curdev, res);
Hi List, during coreboot development i faced the problem that coreboot sometimes hanged after printing the List of PCI devices. It turned out that this is a NULL pointer dereference in search_bus_devices(). Not sure if the following patch is the correct fix, as i don't know that part of the code well. If it is not, please discard the patch and provide a better one :) commit 11a840272dd5f0d0f1a8414163f5404fb028c209 Author: Sven Schnelle <svens@stackframe.org> Date: Mon Jan 10 13:53:18 2011 +0100 Fix a NULL pointer dereference in search_bus_devices() Signed-off-by: Sven Schnelle <svens@stackframe.org>