On 8/26/19 3:55 AM, Alexey Skidanov wrote:
On 8/26/19 11:36 AM, Laura Abbott wrote:
On 8/23/19 10:28 PM, Alexey Skidanov wrote:
In ion_cma_heap, the allocated buffer is represented by a single
struct scatterlist instance. The length field of this struct is
32 bit, hence the maximal size of requested buffer should be
less than 4GB.
The len paramer of the allocation function is 64 bit (on 64 bit systems).
Hence the requested size might be greater than 4GB and in this case
the field length of the struct scatterlist is initialized incorrectly.
To fix this, we check that requested size may fit into
the field length of the struct scatterlist
Is this a real issue that's actually possible to hit? Allocating
more than a 4GB region of CMA seems ill advised and likely to
throw off all the accounting.
Yes. Not sure why it seems ill advised - most of the buffers are small or middle size ones, but sometimes really huge one is requested.
Mostly I'm surprised allocating 4GB of CMA actually succeeds. When
I was last doing work on CMA, the reliability just wasn't there
for larger regions. It's been several years since then so maybe
things have changed. My concern about the accounting is that
most of the math is done such that there are a few CMA pages
vs more pages across the rest of the system which seems likely to
hit performance issues. CMA was designed with targets with smaller
memory footprints so anything that has 4GB of memory to go around is
much larger than what CMA was originally designed for.
The check added should only be applicable if we can reliably
allocate more than 4GB of CMA. If you can confirm you have a setup
where you are actually able to allocate 4GB of CMA, I'd rather
just have the check be for 4GB explicitly instead of tying
it to just the scatterlist length. It's a reasonable restriction
to make and it's easier to review.
Signed-off-by: Alexey Skidanov <alexey.skidanov@xxxxxxxxx>
---
drivers/staging/android/ion/ion.h | 5 +++++
drivers/staging/android/ion/ion_cma_heap.c | 3 +++
2 files changed, 8 insertions(+)
diff --git a/drivers/staging/android/ion/ion.h b/drivers/staging/android/ion/ion.h
index e291299..9dd7e20 100644
--- a/drivers/staging/android/ion/ion.h
+++ b/drivers/staging/android/ion/ion.h
@@ -21,6 +21,11 @@
#include "../uapi/ion.h"
+#define MAX_SCATTERLIST_LEN ({\
+ typeof(((struct scatterlist *)0)->length) v;\
+ v = -1;\
+ })
+
/**
* struct ion_buffer - metadata for a particular buffer
* @node: node in the ion_device buffers tree
diff --git a/drivers/staging/android/ion/ion_cma_heap.c b/drivers/staging/android/ion/ion_cma_heap.c
index bf65e67..d069719 100644
--- a/drivers/staging/android/ion/ion_cma_heap.c
+++ b/drivers/staging/android/ion/ion_cma_heap.c
@@ -36,6 +36,9 @@ static int ion_cma_allocate(struct ion_heap *heap, struct ion_buffer *buffer,
unsigned long align = get_order(size);
int ret;
+ if (size > MAX_SCATTERLIST_LEN)
+ return -EINVAL;
+
if (align > CONFIG_CMA_ALIGNMENT)
align = CONFIG_CMA_ALIGNMENT;
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
