Messages in this thread |  | | Date | Thu, 5 Apr 2018 17:23:10 -0700 | From | Ram Pai <> | Subject | Re: [PATCH] resource: Fix integer overflow at reallocation |
| |
On Thu, Apr 05, 2018 at 04:40:15PM +0200, Takashi Iwai wrote: > On Mon, 02 Apr 2018 22:35:04 +0200, > Takashi Iwai wrote: > > > > On Mon, 02 Apr 2018 21:09:03 +0200, > > Ram Pai wrote: > > > > > > On Mon, Apr 02, 2018 at 09:16:16AM +0200, Takashi Iwai wrote: > > > > We've got a bug report indicating a kernel panic at booting on an > > > > x86-32 system, and it turned out to be the invalid resource assigned > > > > after reallocation. __find_resource() first aligns the resource start > > > > address and resets the end address with start+size-1 accordingly, then > > > > checks whether it's contained. Here the end address may overflow the > > > > integer, although resource_contains() still returns true because the > > > > function validates only start and end address. So this ends up with > > > > returning an invalid resource (start > end). > > > > > > > > There was already an attempt to cover such a problem in the commit > > > > 47ea91b4052d ("Resource: fix wrong resource window calculation"), but > > > > this case is an overseen one. > > > > > > > > This patch adds the validity check of the newly calculated resource > > > > for avoiding the integer overflow problem. > > > > > > Should we move this check "alloc.start <= alloc.end" into resource_contains()? > > > Doing so will catch all uses of such erroneous (overflowing) resources. > > > > I thought of it, too. OTOH, it's rather a validity check and doesn't > > belong to resource_contains() semantics. If the function returns > > false, you don't know whether it's an invalid resource or it's really > > not contained. > > > > We may return an error code, but I'm not sure whether such an API > > change is needed. Maybe not. > > FWIW, below is the revised one to move the check into > resource_contains(). > > My concern is, however, that the resource validity check isn't a job > of resource_contains(). OTOH, this may avoid other similar cases, so > it might be worth. > > In anyway, if there is no objection, and anyone else doesn't want to > take, I'll forward this to Andrew as a poor orphan kid.
I will stand by you as a poor-orphan-buddy. :-)
Reviewed-by: Ram Pai <linuxram@us.ibm.com>
RP
> > > thanks, > > Takashi > > -- 8< -- > From: Takashi Iwai <tiwai@suse.de> > Subject: [PATCH v2] resource: Fix integer overflow at reallocation > > We've got a bug report indicating a kernel panic at booting on an > x86-32 system, and it turned out to be the invalid resource assigned > after reallocation. __find_resource() first aligns the resource start > address and resets the end address with start+size-1 accordingly, then > checks whether it's contained. Here the end address may overflow the > integer, although resource_contains() still returns true because the > function validates only start and end address. So this ends up with > returning an invalid resource (start > end). > > There was already an attempt to cover such a problem in the commit > 47ea91b4052d ("Resource: fix wrong resource window calculation"), but > this case is an overseen one. > > This patch adds the validity check in resource_contains() to see > whether the given resource has a valid range for avoiding the integer > overflow problem.
> > Bugzilla: https://urldefense.proofpoint.com/v2/url?u=http-3A__bugzilla.opensuse.org_show-5Fbug.cgi-3Fid-3D1086739&d=DwIBAg&c=jf_iaSHvJObTbx-siA1ZOg&r=m-UrKChQVkZtnPpjbF6YY99NbT8FBByQ-E-ygV8luxw&m=NNkNAWFZc1XRu3rkv7Y2sepSCe8re2cvNZuCJt5dWFE&s=7i3g3ZVYsUceGVK_ZKkCJIABp4l0RD59NelK3GoD8mI&e= > Fixes: 23c570a67448 ("resource: ability to resize an allocated resource") > Reported-and-tested-by: Michael Henders <hendersm@shaw.ca> > Cc: <stable@vger.kernel.org> > Signed-off-by: Takashi Iwai <tiwai@suse.de> > --- > include/linux/ioport.h | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/include/linux/ioport.h b/include/linux/ioport.h > index da0ebaec25f0..466d7be046eb 100644 > --- a/include/linux/ioport.h > +++ b/include/linux/ioport.h > @@ -212,6 +212,9 @@ static inline bool resource_contains(struct resource *r1, struct resource *r2) > return false; > if (r1->flags & IORESOURCE_UNSET || r2->flags & IORESOURCE_UNSET) > return false; > + /* sanity check whether it's a valid resource range */ > + if (r2->end < r2->start) > + return false; > return r1->start <= r2->start && r1->end >= r2->end; > } > > -- > 2.16.3
-- Ram Pai
|  |