Re: [PATCH 1/1] staging: ion: Prevent incorrect reference counting behavour

[Lee Jones <lee.jones@linaro.org>]

On Fri, 26 Nov 2021, Greg KH wrote:

> On Fri, Nov 26, 2021 at 08:56:27AM +0000, Lee Jones wrote:
> > On Fri, 26 Nov 2021, Dan Carpenter wrote:
> > 
> > > On Thu, Nov 25, 2021 at 06:18:22PM +0300, Dan Carpenter wrote:
> > > > I had thought that ->kmap_cnt was a regular int and not an unsigned
> > > > int, but I would have to pull a stable tree to see where I misread the
> > > > code.
> > > 
> > > I was looking at (struct ion_buffer)->kmap_cnt but this is
> > > (struct ion_handle)->kmap_cnt.  I'm not sure how those are related but
> > > it makes me nervous that one can go higher than the other.  Also both
> > > probably need overflow protection.
> > > 
> > > So I guess I would just do something like:
> > > 
> > > diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
> > > index 806e9b30b9dc8..e8846279b33b5 100644
> > > --- a/drivers/staging/android/ion/ion.c
> > > +++ b/drivers/staging/android/ion/ion.c
> > > @@ -489,6 +489,8 @@ static void *ion_buffer_kmap_get(struct ion_buffer *buffer)
> > >  	void *vaddr;
> > >  
> > >  	if (buffer->kmap_cnt) {
> > > +		if (buffer->kmap_cnt == INT_MAX)
> > > +			return ERR_PTR(-EOVERFLOW);
> > >  		buffer->kmap_cnt++;
> > >  		return buffer->vaddr;
> > >  	}
> > > @@ -509,6 +511,8 @@ static void *ion_handle_kmap_get(struct ion_handle *handle)
> > >  	void *vaddr;
> > >  
> > >  	if (handle->kmap_cnt) {
> > > +		if (handle->kmap_cnt == INT_MAX)
> > > +			return ERR_PTR(-EOVERFLOW);
> > >  		handle->kmap_cnt++;
> > >  		return buffer->vaddr;
> > >  	}
> > 
> > Which is all well and good until somebody changes the type.
> 
> That's hard to do on code that is removed from the kernel tree :)

That's a difficult stance to take when reviewing a patch which changes
the very code you base your argument on. :D

I'll do with Dan's PoV though - no sympathy given. :)

v3 to follow.

-- 
Lee Jones [李琼斯]
Senior Technical Lead - Developer Services
Linaro.org │ Open source software for Arm SoCs
Follow Linaro: Facebook | Twitter | Blog