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; }
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
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/
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?
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
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.
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; }
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);@@ -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);
Which is all well and good until somebody changes the type.
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);@@ -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);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
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);@@ -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);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 :)
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);@@ -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);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.
linux-stable-mirror@lists.linaro.org
-
Dan Carpenter -
Greg KH -
Lee Jones