Re: [PATCH] resource: Fix integer overflow at reallocation

From: Ram Pai
Date: Thu Apr 05 2018 - 20:23:25 EST


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

RP

>
>
> thanks,
>
> Takashi
>
> -- 8< --
> From: Takashi Iwai <tiwai@xxxxxxx>
> 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@xxxxxxx>
> Cc: <stable@xxxxxxxxxxxxxxx>
> Signed-off-by: Takashi Iwai <tiwai@xxxxxxx>
> ---
> 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