NULL pointer dereference in search_bus_device()

[Sven Schnelle]

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 
Date:   Mon Jan 10 13:53:18 2011 +0100

Fix a NULL pointer dereference in search_bus_devices()

Signed-off-by: Sven Schnelle

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);

[Myles Watson]

> 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

[Sven Schnelle]

"Myles Watson"  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]

Sven Schnelle  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.

[Myles Watson]

On Mon, Jan 10, 2011 at 6:27 AM, Sven Schnelle  wrote:
> "Myles Watson"  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

[Stefan Reinauer]

* Myles Watson  [110110 14:29]:
> On Mon, Jan 10, 2011 at 6:27 AM, Sven Schnelle  wrote:
> > "Myles Watson"  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?

[Myles Watson]

> > > 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

[Rudolf Marek]

> 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

[Myles Watson]

> > 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

[Stefan Reinauer]

* Myles Watson  [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