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

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

[Lee Jones]

Supply additional check in order to prevent unexpected results.

Fixes: b892bf75b2034 ("ion: Switch ion to use dma-buf")
Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Lee Jones <lee.jones@linaro.org>
---
This is a forward-port from linux-4.4.y and linux-4.9.y.

It has never been upstream.

Please apply to v4.14 through v5.10.

 drivers/staging/android/ion/ion.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
index e1fe03ceb7f13..e6d4a3ee6cda5 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -114,6 +114,9 @@ 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;
 	}
-- 
2.36.0.rc2.479.g8af0fa9b8e-goog

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

[Greg KH]

On Mon, Apr 25, 2022 at 04:51:54PM +0100, Lee Jones wrote:
> Supply additional check in order to prevent unexpected results.
> 
> Fixes: b892bf75b2034 ("ion: Switch ion to use dma-buf")
> Suggested-by: Dan Carpenter <dan.carpenter@oracle.com>
> Signed-off-by: Lee Jones <lee.jones@linaro.org>
> ---
> This is a forward-port from linux-4.4.y and linux-4.9.y.
> 
> It has never been upstream.
> 
> Please apply to v4.14 through v5.10.
> 
>  drivers/staging/android/ion/ion.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
> index e1fe03ceb7f13..e6d4a3ee6cda5 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -114,6 +114,9 @@ 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;
>  	}
> -- 
> 2.36.0.rc2.479.g8af0fa9b8e-goog
> 

Now queued up, thanks.

gre gk-h

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

[Lee Jones]

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++;
 		return buffer->vaddr;
 	}
-- 
2.34.0.rc2.393.gf8c9666880-goog

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

[Dan Carpenter]

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.

regards,
dan carpenter

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

[Lee Jones]

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.

So the original patch [0] was better?

[0] https://lore.kernel.org/stable/20211125120234.67987-1-lee.jones@linaro.org/

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

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

[Lee Jones]

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

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

[Dan Carpenter]

On Thu, Nov 25, 2021 at 03:07:37PM +0000, 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;                       \
          ^^^^^^^^^^^^^^^^
This assignment sets handle->kmap_cnt to the overflowed value.

>         *__d < __a;                             \
> })
> 
> Maybe I misread it.
> 
> So the original patch [0] was better?
> 
> [0] https://lore.kernel.org/stable/20211125120234.67987-1-lee.jones@linaro.org/ 

The original at least worked.  :P

You're catching me right as I'm knocking off for the day so I'm not
sure how to write this code.  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'll look at this tomorrow Nairobi time, but I expect by then you'll
already have it all figured out.

regards,
dan carpenter

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

[Lee Jones]

On Thu, 25 Nov 2021, Dan Carpenter wrote:

> On Thu, Nov 25, 2021 at 03:07:37PM +0000, 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;                       \
>           ^^^^^^^^^^^^^^^^
> This assignment sets handle->kmap_cnt to the overflowed value.
> 
> >         *__d < __a;                             \
> > })
> > 
> > Maybe I misread it.
> > 
> > So the original patch [0] was better?
> > 
> > [0] https://lore.kernel.org/stable/20211125120234.67987-1-lee.jones@linaro.org/ 
> 
> The original at least worked.  :P
> 
> You're catching me right as I'm knocking off for the day so I'm not
> sure how to write this code.  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'll look at this tomorrow Nairobi time, but I expect by then you'll
> already have it all figured out.

There's no rush.  This has been broken for a long time.

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

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

[Dan Carpenter]

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;
 	}

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

[Lee Jones]

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.

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

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

[Dan Carpenter]

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.

It would only break if they change it to something smaller than an int.
I have zero sympathy for them if they do that.

regards,
dan carpenter

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

[Greg KH]

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 :)

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

[Lee Jones]

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

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

[Lee Jones]

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>
---
 drivers/staging/android/ion/ion.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
index 806e9b30b9dc8..30f359faba575 100644
--- a/drivers/staging/android/ion/ion.c
+++ b/drivers/staging/android/ion/ion.c
@@ -509,6 +509,9 @@ static void *ion_handle_kmap_get(struct ion_handle *handle)
 	void *vaddr;
 
 	if (handle->kmap_cnt) {
+		if (handle->kmap_cnt + 1 < handle->kmap_cnt)
+			return ERR_PTR(-EOVERFLOW);
+
 		handle->kmap_cnt++;
 		return buffer->vaddr;
 	}
-- 
2.34.0.rc2.393.gf8c9666880-goog

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

[Greg KH]

On Thu, Nov 25, 2021 at 12:02:34PM +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>
> ---
>  drivers/staging/android/ion/ion.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
> index 806e9b30b9dc8..30f359faba575 100644
> --- a/drivers/staging/android/ion/ion.c
> +++ b/drivers/staging/android/ion/ion.c
> @@ -509,6 +509,9 @@ static void *ion_handle_kmap_get(struct ion_handle *handle)
>  	void *vaddr;
>  
>  	if (handle->kmap_cnt) {
> +		if (handle->kmap_cnt + 1 < handle->kmap_cnt)

What about using the nice helpers in overflow.h for this?

> +			return ERR_PTR(-EOVERFLOW);
> +
>  		handle->kmap_cnt++;
>  		return buffer->vaddr;
>  	}
> -- 
> 2.34.0.rc2.393.gf8c9666880-goog

What stable kernel branch(es) is this for?

thanks,

greg k-h

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

[Lee Jones]

On Thu, 25 Nov 2021, Greg KH wrote:

> On Thu, Nov 25, 2021 at 12:02:34PM +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>
> > ---
> >  drivers/staging/android/ion/ion.c | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
> > index 806e9b30b9dc8..30f359faba575 100644
> > --- a/drivers/staging/android/ion/ion.c
> > +++ b/drivers/staging/android/ion/ion.c
> > @@ -509,6 +509,9 @@ static void *ion_handle_kmap_get(struct ion_handle *handle)
> >  	void *vaddr;
> >  
> >  	if (handle->kmap_cnt) {
> > +		if (handle->kmap_cnt + 1 < handle->kmap_cnt)
> 
> What about using the nice helpers in overflow.h for this?

I haven't heard of these before.

Looks like they're not widely used.

I'll try them out and see how they go.

> > +			return ERR_PTR(-EOVERFLOW);
> > +
> >  		handle->kmap_cnt++;
> >  		return buffer->vaddr;
> >  	}
> 
> What stable kernel branch(es) is this for?

I assumed your magic scripts could determine this from the Fixes:
tag.  I'll be more explicit in v2.

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

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

[Greg KH]

On Thu, Nov 25, 2021 at 12:46:23PM +0000, Lee Jones wrote:
> On Thu, 25 Nov 2021, Greg KH wrote:
> 
> > On Thu, Nov 25, 2021 at 12:02:34PM +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>
> > > ---
> > >  drivers/staging/android/ion/ion.c | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
> > > index 806e9b30b9dc8..30f359faba575 100644
> > > --- a/drivers/staging/android/ion/ion.c
> > > +++ b/drivers/staging/android/ion/ion.c
> > > @@ -509,6 +509,9 @@ static void *ion_handle_kmap_get(struct ion_handle *handle)
> > >  	void *vaddr;
> > >  
> > >  	if (handle->kmap_cnt) {
> > > +		if (handle->kmap_cnt + 1 < handle->kmap_cnt)
> > 
> > What about using the nice helpers in overflow.h for this?
> 
> I haven't heard of these before.
> 
> Looks like they're not widely used.
> 
> I'll try them out and see how they go.
> 
> > > +			return ERR_PTR(-EOVERFLOW);
> > > +
> > >  		handle->kmap_cnt++;
> > >  		return buffer->vaddr;
> > >  	}
> > 
> > What stable kernel branch(es) is this for?
> 
> I assumed your magic scripts could determine this from the Fixes:
> tag.  I'll be more explicit in v2.

The fixes tag says how far back for it to go, but not where to start
that process from :)

thanks,

greg k-h

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

[Lee Jones]

On Thu, 25 Nov 2021, Greg KH wrote:

> On Thu, Nov 25, 2021 at 12:46:23PM +0000, Lee Jones wrote:
> > On Thu, 25 Nov 2021, Greg KH wrote:
> > 
> > > On Thu, Nov 25, 2021 at 12:02:34PM +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>
> > > > ---
> > > >  drivers/staging/android/ion/ion.c | 3 +++
> > > >  1 file changed, 3 insertions(+)
> > > > 
> > > > diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
> > > > index 806e9b30b9dc8..30f359faba575 100644
> > > > --- a/drivers/staging/android/ion/ion.c
> > > > +++ b/drivers/staging/android/ion/ion.c
> > > > @@ -509,6 +509,9 @@ static void *ion_handle_kmap_get(struct ion_handle *handle)
> > > >  	void *vaddr;
> > > >  
> > > >  	if (handle->kmap_cnt) {
> > > > +		if (handle->kmap_cnt + 1 < handle->kmap_cnt)
> > > 
> > > What about using the nice helpers in overflow.h for this?
> > 
> > I haven't heard of these before.
> > 
> > Looks like they're not widely used.
> > 
> > I'll try them out and see how they go.
> > 
> > > > +			return ERR_PTR(-EOVERFLOW);
> > > > +
> > > >  		handle->kmap_cnt++;
> > > >  		return buffer->vaddr;
> > > >  	}
> > > 
> > > What stable kernel branch(es) is this for?
> > 
> > I assumed your magic scripts could determine this from the Fixes:
> > tag.  I'll be more explicit in v2.
> 
> The fixes tag says how far back for it to go, but not where to start
> that process from :)

What's your preferred method for identifying a start-point?

In the [PATCH] tag or appended on to Cc: stable ... # <here>?

I know both work, but what makes your life easier?

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

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

[Greg KH]

On Thu, Nov 25, 2021 at 01:03:46PM +0000, Lee Jones wrote:
> On Thu, 25 Nov 2021, Greg KH wrote:
> 
> > On Thu, Nov 25, 2021 at 12:46:23PM +0000, Lee Jones wrote:
> > > On Thu, 25 Nov 2021, Greg KH wrote:
> > > 
> > > > On Thu, Nov 25, 2021 at 12:02:34PM +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>
> > > > > ---
> > > > >  drivers/staging/android/ion/ion.c | 3 +++
> > > > >  1 file changed, 3 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
> > > > > index 806e9b30b9dc8..30f359faba575 100644
> > > > > --- a/drivers/staging/android/ion/ion.c
> > > > > +++ b/drivers/staging/android/ion/ion.c
> > > > > @@ -509,6 +509,9 @@ static void *ion_handle_kmap_get(struct ion_handle *handle)
> > > > >  	void *vaddr;
> > > > >  
> > > > >  	if (handle->kmap_cnt) {
> > > > > +		if (handle->kmap_cnt + 1 < handle->kmap_cnt)
> > > > 
> > > > What about using the nice helpers in overflow.h for this?
> > > 
> > > I haven't heard of these before.
> > > 
> > > Looks like they're not widely used.
> > > 
> > > I'll try them out and see how they go.
> > > 
> > > > > +			return ERR_PTR(-EOVERFLOW);
> > > > > +
> > > > >  		handle->kmap_cnt++;
> > > > >  		return buffer->vaddr;
> > > > >  	}
> > > > 
> > > > What stable kernel branch(es) is this for?
> > > 
> > > I assumed your magic scripts could determine this from the Fixes:
> > > tag.  I'll be more explicit in v2.
> > 
> > The fixes tag says how far back for it to go, but not where to start
> > that process from :)
> 
> What's your preferred method for identifying a start-point?
> 
> In the [PATCH] tag or appended on to Cc: stable ... # <here>?
> 
> I know both work, but what makes your life easier?

Easiest is below the --- line say:
---
 This is for kernel versions X.X and older.

nothing complex.

thanks,

greg k-h

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

[Lee Jones]

On Thu, 25 Nov 2021, Greg KH wrote:

> On Thu, Nov 25, 2021 at 01:03:46PM +0000, Lee Jones wrote:
> > On Thu, 25 Nov 2021, Greg KH wrote:
> > 
> > > On Thu, Nov 25, 2021 at 12:46:23PM +0000, Lee Jones wrote:
> > > > On Thu, 25 Nov 2021, Greg KH wrote:
> > > > 
> > > > > On Thu, Nov 25, 2021 at 12:02:34PM +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>
> > > > > > ---
> > > > > >  drivers/staging/android/ion/ion.c | 3 +++
> > > > > >  1 file changed, 3 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/staging/android/ion/ion.c b/drivers/staging/android/ion/ion.c
> > > > > > index 806e9b30b9dc8..30f359faba575 100644
> > > > > > --- a/drivers/staging/android/ion/ion.c
> > > > > > +++ b/drivers/staging/android/ion/ion.c
> > > > > > @@ -509,6 +509,9 @@ static void *ion_handle_kmap_get(struct ion_handle *handle)
> > > > > >  	void *vaddr;
> > > > > >  
> > > > > >  	if (handle->kmap_cnt) {
> > > > > > +		if (handle->kmap_cnt + 1 < handle->kmap_cnt)
> > > > > 
> > > > > What about using the nice helpers in overflow.h for this?
> > > > 
> > > > I haven't heard of these before.
> > > > 
> > > > Looks like they're not widely used.
> > > > 
> > > > I'll try them out and see how they go.
> > > > 
> > > > > > +			return ERR_PTR(-EOVERFLOW);
> > > > > > +
> > > > > >  		handle->kmap_cnt++;
> > > > > >  		return buffer->vaddr;
> > > > > >  	}
> > > > > 
> > > > > What stable kernel branch(es) is this for?
> > > > 
> > > > I assumed your magic scripts could determine this from the Fixes:
> > > > tag.  I'll be more explicit in v2.
> > > 
> > > The fixes tag says how far back for it to go, but not where to start
> > > that process from :)
> > 
> > What's your preferred method for identifying a start-point?
> > 
> > In the [PATCH] tag or appended on to Cc: stable ... # <here>?
> > 
> > I know both work, but what makes your life easier?
> 
> Easiest is below the --- line say:
> ---
>  This is for kernel versions X.X and older.

Understood, thanks.

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