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

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

On Thu, 25 Nov 2021, Lee Jones wrote:

> On Thu, 25 Nov 2021, Dan Carpenter wrote:
> 
> > On Thu, Nov 25, 2021 at 02:20:04PM +0000, Lee Jones wrote:
> > > Supply additional checks in order to prevent unexpected results.
> > > 
> > > Fixes: b892bf75b2034 ("ion: Switch ion to use dma-buf")
> > > Signed-off-by: Lee Jones <lee.jones@linaro.org>
> > > ---
> > > Should be back-ported from v4.9 and earlier.
> > > 
> > >  drivers/staging/android/ion/ion.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
> > > index 806e9b30b9dc8..402b74f5d7e69 100644
> > > --- a/drivers/staging/android/ion/ion.c
> > > +++ b/drivers/staging/android/ion/ion.c
> > > @@ -29,6 +29,7 @@
> > >  #include <linux/export.h>
> > >  #include <linux/mm.h>
> > >  #include <linux/mm_types.h>
> > > +#include <linux/overflow.h>
> > >  #include <linux/rbtree.h>
> > >  #include <linux/slab.h>
> > >  #include <linux/seq_file.h>
> > > @@ -509,6 +510,10 @@ static void *ion_handle_kmap_get(struct ion_handle *handle)
> > >  	void *vaddr;
> > >  
> > >  	if (handle->kmap_cnt) {
> > > +		if (check_add_overflow(handle->kmap_cnt,
> > > +				       (unsigned int) 1, &handle->kmap_cnt))
> >                                                          ^^^^^^^^^^^^^^^^^
> > 
> > > +			return ERR_PTR(-EOVERFLOW);
> > > +
> > >  		handle->kmap_cnt++;
> >                 ^^^^^^^^^^^^^^^^^^^
> > This will not do what you want at all.  It's a double increment on the
> > success path and it leave handle->kmap_cnt overflowed on failure path.
> 
> I read the helper to take copies of the original variables.
> 
> #define __unsigned_add_overflow(a, b, d) ({     \
>         typeof(a) __a = (a);                    \
>         typeof(b) __b = (b);                    \
>         typeof(d) __d = (d);                    \
>         (void) (&__a == &__b);                  \
>         (void) (&__a == __d);                   \
>         *__d = __a + __b;                       \
>         *__d < __a;                             \
> })
> 
> Maybe I misread it.

I think I see now.

Copies are taken, but because 'd' is a pointer, dereferencing the copy
is just like dereferencing the original, thus the memory address
provided by 'd' is written to, updating the variable.

In this case, you're right, this is not what I was trying to achieve.

> So the original patch [0] was better?
> 
> [0] https://lore.kernel.org/stable/20211125120234.67987-1-lee.jones@linaro.org/

Greg, are you able to take the original patch for v4.4 and v4.9 please?

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