[PATCH] spi: Fix per-page mapping of unaligned vmalloc-ed buffer

[PATCH] spi: Fix per-page mapping of unaligned vmalloc-ed buffer

[Andrew Gabbasov]

spi_map_buf() processes mapping of vmalloc-ed buffers in a special way,
making mapping of every page separately. However, if the buffer is not
aligned to page boundary (e.g. sub-array in a vmalloc-ed array), it
fills the scatter table with page-size unaligned pieces, that cross
page boundaries. This is incorrect and can, for example, cause memory
corruption and various crashes when working with ubifs on spi-nor chips.

Fix this by using proper scatter table size and intra-page buffer lengths,
so that the whole buffer splits into separate scatter table entries on
page boundaries.

Signed-off-by: Andrew Gabbasov <andrew_gabbasov-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
---
 drivers/spi/spi.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 50910d8..14016dc 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -477,7 +477,9 @@ static int spi_map_buf(struct spi_master *master, struct device *dev,
 {
 	const bool vmalloced_buf = is_vmalloc_addr(buf);
 	const int desc_len = vmalloced_buf ? PAGE_SIZE : master->max_dma_len;
-	const int sgs = DIV_ROUND_UP(len, desc_len);
+	const int sgs = DIV_ROUND_UP(vmalloced_buf ?
+					len + offset_in_page(buf) : len,
+				     desc_len);
 	struct page *vm_page;
 	void *sg_buf;
 	size_t min;
@@ -488,9 +490,10 @@ static int spi_map_buf(struct spi_master *master, struct device *dev,
 		return ret;
 
 	for (i = 0; i < sgs; i++) {
-		min = min_t(size_t, len, desc_len);
 
 		if (vmalloced_buf) {
+			min = min_t(size_t,
+				    len, desc_len - offset_in_page(buf));
 			vm_page = vmalloc_to_page(buf);
 			if (!vm_page) {
 				sg_free_table(sgt);
@@ -499,6 +502,7 @@ static int spi_map_buf(struct spi_master *master, struct device *dev,
 			sg_set_page(&sgt->sgl[i], vm_page,
 				    min, offset_in_page(buf));
 		} else {
+			min = min_t(size_t, len, desc_len);
 			sg_buf = buf;
 			sg_set_buf(&sgt->sgl[i], sg_buf, min);
 		}
-- 
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: Fix per-page mapping of unaligned vmalloc-ed buffer

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 837 bytes --]

On Thu, Jun 25, 2015 at 11:40:15AM -0500, Andrew Gabbasov wrote:
> spi_map_buf() processes mapping of vmalloc-ed buffers in a special way,
> making mapping of every page separately. However, if the buffer is not
> aligned to page boundary (e.g. sub-array in a vmalloc-ed array), it
> fills the scatter table with page-size unaligned pieces, that cross
> page boundaries. This is incorrect and can, for example, cause memory
> corruption and various crashes when working with ubifs on spi-nor chips.

The caller is supposed to be providing us with aligned memory here.
However it could be helpful to do this so...

> -	const int sgs = DIV_ROUND_UP(len, desc_len);
> +	const int sgs = DIV_ROUND_UP(vmalloced_buf ?
> +					len + offset_in_page(buf) : len,
> +				     desc_len);

No, please write this legibly without the ternery operator.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

RE: [PATCH] spi: Fix per-page mapping of unaligned vmalloc-ed buffer

[Andrew Gabbasov]

Hi Mark,

> -----Original Message-----
> From: Mark Brown [mailto:broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org]
> Sent: Friday, June 26, 2015 2:46 PM
> To: Gabbasov, Andrew
> Cc: linux-spi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH] spi: Fix per-page mapping of unaligned vmalloc-ed
buffer
> 
> On Thu, Jun 25, 2015 at 11:40:15AM -0500, Andrew Gabbasov wrote:
> > spi_map_buf() processes mapping of vmalloc-ed buffers in a special
> > way, making mapping of every page separately. However, if the buffer
> > is not aligned to page boundary (e.g. sub-array in a vmalloc-ed
> > array), it fills the scatter table with page-size unaligned pieces,
> > that cross page boundaries. This is incorrect and can, for example,
> > cause memory corruption and various crashes when working with ubifs on
spi-
> nor chips.
> 
> The caller is supposed to be providing us with aligned memory here.
> However it could be helpful to do this so...

Well, actually the rest of spi code does not rely on having a transfer
buffer
page-aligned. And I don't see any reason to make such an assumption here.
Especially that it is not fulfilled, at least by ubifs code.
Anyway, this fix seems to be useful indeed.

> > -	const int sgs = DIV_ROUND_UP(len, desc_len);
> > +	const int sgs = DIV_ROUND_UP(vmalloced_buf ?
> > +					len + offset_in_page(buf) : len,
> > +				     desc_len);
> 
> No, please write this legibly without the ternery operator.

OK, I'll try to make this piece of code more distinct.
I'm submitting v2 of the patch.

Thanks.

Best regards,
Andrew


--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: Fix per-page mapping of unaligned vmalloc-ed buffer

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 559 bytes --]

On Tue, Jun 30, 2015 at 06:37:07PM +0300, Andrew Gabbasov wrote:

> > The caller is supposed to be providing us with aligned memory here.
> > However it could be helpful to do this so...

> Well, actually the rest of spi code does not rely on having a transfer
> buffer
> page-aligned. And I don't see any reason to make such an assumption here.
> Especially that it is not fulfilled, at least by ubifs code.
> Anyway, this fix seems to be useful indeed.

The requirement comes more from the underlying DMA code than from SPI
itself, SPI mostly doesn't care.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

RE: [PATCH] spi: Fix per-page mapping of unaligned vmalloc-ed buffer

[Andrew Gabbasov]

> -----Original Message-----
> From: Mark Brown [mailto:broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org]
> Sent: Tuesday, June 30, 2015 7:03 PM
> To: Gabbasov, Andrew
> Cc: linux-spi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH] spi: Fix per-page mapping of unaligned vmalloc-ed
buffer
> 
> On Tue, Jun 30, 2015 at 06:37:07PM +0300, Andrew Gabbasov wrote:
> 
> > > The caller is supposed to be providing us with aligned memory here.
> > > However it could be helpful to do this so...
> 
> > Well, actually the rest of spi code does not rely on having a transfer
> > buffer page-aligned. And I don't see any reason to make such an
> > assumption here.
> > Especially that it is not fulfilled, at least by ubifs code.
> > Anyway, this fix seems to be useful indeed.
> 
> The requirement comes more from the underlying DMA code than from SPI
> itself, SPI mostly doesn't care.

OK, then it's all the more useful to have SPI code tolerant to arbitrary
input data
(like un-aligned addresses), so that it won't introduce its own unnecessary
restrictions.

Thanks.

Best regards,
Andrew


--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] spi: Fix per-page mapping of unaligned vmalloc-ed buffer

[Andrew Gabbasov]

spi_map_buf() processes mapping of vmalloc-ed buffers in a special way,
making mapping of every page separately. However, if the buffer is not
aligned to page boundary (e.g. sub-array in a vmalloc-ed array), it
fills the scatter table with page-size unaligned pieces, that cross
page boundaries. This is incorrect and can, for example, cause memory
corruption and various crashes when working with ubifs on spi-nor chips.

Fix this by using proper scatter table size and intra-page buffer lengths,
so that the whole buffer splits into separate scatter table entries on
page boundaries.

Signed-off-by: Andrew Gabbasov <andrew_gabbasov-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
---
 drivers/spi/spi.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 50910d8..14016dc 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -477,7 +477,9 @@ static int spi_map_buf(struct spi_master *master, struct device *dev,
 {
 	const bool vmalloced_buf = is_vmalloc_addr(buf);
 	const int desc_len = vmalloced_buf ? PAGE_SIZE : master->max_dma_len;
-	const int sgs = DIV_ROUND_UP(len, desc_len);
+	const int sgs = DIV_ROUND_UP(vmalloced_buf ?
+					len + offset_in_page(buf) : len,
+				     desc_len);
 	struct page *vm_page;
 	void *sg_buf;
 	size_t min;
@@ -488,9 +490,10 @@ static int spi_map_buf(struct spi_master *master, struct device *dev,
 		return ret;
 
 	for (i = 0; i < sgs; i++) {
-		min = min_t(size_t, len, desc_len);
 
 		if (vmalloced_buf) {
+			min = min_t(size_t,
+				    len, desc_len - offset_in_page(buf));
 			vm_page = vmalloc_to_page(buf);
 			if (!vm_page) {
 				sg_free_table(sgt);
@@ -499,6 +502,7 @@ static int spi_map_buf(struct spi_master *master, struct device *dev,
 			sg_set_page(&sgt->sgl[i], vm_page,
 				    min, offset_in_page(buf));
 		} else {
+			min = min_t(size_t, len, desc_len);
 			sg_buf = buf;
 			sg_set_buf(&sgt->sgl[i], sg_buf, min);
 		}
-- 
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] spi: Fix per-page mapping of unaligned vmalloc-ed buffer

[Andrew Gabbasov]

spi_map_buf() processes mapping of vmalloc-ed buffers in a special way,
making mapping of every page separately. However, if the buffer is not
aligned to page boundary (e.g. sub-array in a vmalloc-ed array), it
fills the scatter table with page-size unaligned pieces, that cross
page boundaries. This is incorrect and can, for example, cause memory
corruption and various crashes when working with ubifs on spi-nor chips.

Fix this by using proper scatter table size and intra-page buffer lengths,
so that the whole buffer splits into separate scatter table entries on
page boundaries.

Signed-off-by: Andrew Gabbasov <andrew_gabbasov-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
---
 drivers/spi/spi.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 50910d8..14016dc 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -477,7 +477,9 @@ static int spi_map_buf(struct spi_master *master, struct device *dev,
 {
 	const bool vmalloced_buf = is_vmalloc_addr(buf);
 	const int desc_len = vmalloced_buf ? PAGE_SIZE : master->max_dma_len;
-	const int sgs = DIV_ROUND_UP(len, desc_len);
+	const int sgs = DIV_ROUND_UP(vmalloced_buf ?
+					len + offset_in_page(buf) : len,
+				     desc_len);
 	struct page *vm_page;
 	void *sg_buf;
 	size_t min;
@@ -488,9 +490,10 @@ static int spi_map_buf(struct spi_master *master, struct device *dev,
 		return ret;
 
 	for (i = 0; i < sgs; i++) {
-		min = min_t(size_t, len, desc_len);
 
 		if (vmalloced_buf) {
+			min = min_t(size_t,
+				    len, desc_len - offset_in_page(buf));
 			vm_page = vmalloc_to_page(buf);
 			if (!vm_page) {
 				sg_free_table(sgt);
@@ -499,6 +502,7 @@ static int spi_map_buf(struct spi_master *master, struct device *dev,
 			sg_set_page(&sgt->sgl[i], vm_page,
 				    min, offset_in_page(buf));
 		} else {
+			min = min_t(size_t, len, desc_len);
 			sg_buf = buf;
 			sg_set_buf(&sgt->sgl[i], sg_buf, min);
 		}
-- 
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] spi: Fix per-page mapping of unaligned vmalloc-ed buffer

[Andrew Gabbasov]

> -----Original Message-----
> From: Gabbasov, Andrew
> Sent: Monday, June 15, 2015 9:59 AM
> To: linux-spi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: [PATCH] spi: Fix per-page mapping of unaligned vmalloc-ed buffer
> 
> spi_map_buf() processes mapping of vmalloc-ed buffers in a special way,
making
> mapping of every page separately. However, if the buffer is not aligned to
page
> boundary (e.g. sub-array in a vmalloc-ed array), it fills the scatter
table with
> page-size unaligned pieces, that cross page boundaries. This is incorrect
and
> can, for example, cause memory corruption and various crashes when working
> with ubifs on spi-nor chips.
> 
> Fix this by using proper scatter table size and intra-page buffer lengths,
so that
> the whole buffer splits into separate scatter table entries on page
boundaries.
> 
> Signed-off-by: Andrew Gabbasov <andrew_gabbasov-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/spi/spi.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c index 50910d8..14016dc
100644
> --- a/drivers/spi/spi.c
> +++ b/drivers/spi/spi.c
> @@ -477,7 +477,9 @@ static int spi_map_buf(struct spi_master *master,
struct
> device *dev,  {
>  	const bool vmalloced_buf = is_vmalloc_addr(buf);
>  	const int desc_len = vmalloced_buf ? PAGE_SIZE : master-
> >max_dma_len;
> -	const int sgs = DIV_ROUND_UP(len, desc_len);
> +	const int sgs = DIV_ROUND_UP(vmalloced_buf ?
> +					len + offset_in_page(buf) : len,
> +				     desc_len);
>  	struct page *vm_page;
>  	void *sg_buf;
>  	size_t min;
> @@ -488,9 +490,10 @@ static int spi_map_buf(struct spi_master *master,
> struct device *dev,
>  		return ret;
> 
>  	for (i = 0; i < sgs; i++) {
> -		min = min_t(size_t, len, desc_len);
> 
>  		if (vmalloced_buf) {
> +			min = min_t(size_t,
> +				    len, desc_len - offset_in_page(buf));
>  			vm_page = vmalloc_to_page(buf);
>  			if (!vm_page) {
>  				sg_free_table(sgt);
> @@ -499,6 +502,7 @@ static int spi_map_buf(struct spi_master *master,
struct
> device *dev,
>  			sg_set_page(&sgt->sgl[i], vm_page,
>  				    min, offset_in_page(buf));
>  		} else {
> +			min = min_t(size_t, len, desc_len);
>  			sg_buf = buf;
>  			sg_set_buf(&sgt->sgl[i], sg_buf, min);
>  		}
> --
> 2.1.0

Any opinion on this fix?

Thanks.

Best regards,
Andrew


--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: Fix per-page mapping of unaligned vmalloc-ed buffer

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 381 bytes --]

On Thu, Jun 25, 2015 at 02:27:17PM +0300, Andrew Gabbasov wrote:
> > -----Original Message-----
> > From: Gabbasov, Andrew

> Any opinion on this fix?

Please don't quote entire messages, it makes it hard to find any new
content you have added.  If you want to submit a patch for inclusion in
the kernel please follow the patch submission process in
SubmittingPatches.  

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

RE: [PATCH] spi: Fix per-page mapping of unaligned vmalloc-ed buffer

[Gabbasov, Andrew]

> Please don't quote entire messages, it makes it hard to find any new
> content you have added.  If you want to submit a patch for inclusion in
> the kernel please follow the patch submission process in
> SubmittingPatches.

I did. This patch was sent to linux-spi mailing list twice (June 1 and June 15),
and only after getting no response I asked the maintainer. Sorry if it's too early,
but I believe the existing bug should be fixed. I'll submit the patch once more.

Thanks.

Best regards,
Andrew
--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] spi: Fix per-page mapping of unaligned vmalloc-ed buffer

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 942 bytes --]

On Thu, Jun 25, 2015 at 03:57:50PM +0000, Gabbasov, Andrew wrote:
> > Please don't quote entire messages, it makes it hard to find any new
> > content you have added.  If you want to submit a patch for inclusion in
> > the kernel please follow the patch submission process in
> > SubmittingPatches.

> I did. This patch was sent to linux-spi mailing list twice (June 1 and June 15),
> and only after getting no response I asked the maintainer. Sorry if it's too early,
> but I believe the existing bug should be fixed. I'll submit the patch once more.

SubmittingPatches says:

| 5) Select the recipients for your patch.
| ----------------------------------------
| 
| You should always copy the appropriate subsystem maintainer(s) on any patch
| to code that they maintain; look through the MAINTAINERS file and the

which you haven't done.  If you don't send patches to people they won't
see them and can't review them.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[PATCH] spi: Fix per-page mapping of unaligned vmalloc-ed buffer

[Andrew Gabbasov]

spi_map_buf() processes mapping of vmalloc-ed buffers in a special way,
making mapping of every page separately. However, if the buffer is not
aligned to page boundary (e.g. sub-array in a vmalloc-ed array), it
fills the scatter table with page-size unaligned pieces, that cross
page boundaries. This is incorrect and can, for example, cause memory
corruption and various crashes when working with ubifs on spi-nor chips.

Fix this by using proper scatter table size and intra-page buffer lengths,
so that the whole buffer splits into separate scatter table entries on
page boundaries.

Signed-off-by: Andrew Gabbasov <andrew_gabbasov-nmGgyN9QBj3QT0dZR+AlfA@public.gmane.org>
---
 drivers/spi/spi.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/spi/spi.c b/drivers/spi/spi.c
index 50910d8..14016dc 100644
--- a/drivers/spi/spi.c
+++ b/drivers/spi/spi.c
@@ -477,7 +477,9 @@ static int spi_map_buf(struct spi_master *master, struct device *dev,
 {
 	const bool vmalloced_buf = is_vmalloc_addr(buf);
 	const int desc_len = vmalloced_buf ? PAGE_SIZE : master->max_dma_len;
-	const int sgs = DIV_ROUND_UP(len, desc_len);
+	const int sgs = DIV_ROUND_UP(vmalloced_buf ?
+					len + offset_in_page(buf) : len,
+				     desc_len);
 	struct page *vm_page;
 	void *sg_buf;
 	size_t min;
@@ -488,9 +490,10 @@ static int spi_map_buf(struct spi_master *master, struct device *dev,
 		return ret;
 
 	for (i = 0; i < sgs; i++) {
-		min = min_t(size_t, len, desc_len);
 
 		if (vmalloced_buf) {
+			min = min_t(size_t,
+				    len, desc_len - offset_in_page(buf));
 			vm_page = vmalloc_to_page(buf);
 			if (!vm_page) {
 				sg_free_table(sgt);
@@ -499,6 +502,7 @@ static int spi_map_buf(struct spi_master *master, struct device *dev,
 			sg_set_page(&sgt->sgl[i], vm_page,
 				    min, offset_in_page(buf));
 		} else {
+			min = min_t(size_t, len, desc_len);
 			sg_buf = buf;
 			sg_set_buf(&sgt->sgl[i], sg_buf, min);
 		}
-- 
2.1.0

--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html