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

[Ram Pai]

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