Re: [PATCH] pxa2xx_spi: fix memory corruption
On Sunday, July 10, 2011 09:14:45 AM Marek Vasut wrote: On Sunday, July 10, 2011 01:11:09 AM Russell King - ARM Linux wrote: On Sun, Jul 10, 2011 at 01:05:48AM +0200, Marek Vasut wrote: On Saturday, July 09, 2011 11:14:58 PM Vasily Khoruzhick wrote: pxa2xx_spi_probe allocates struct driver_data and null_dma_buf at same time via spi_alloc_master(), but then calculates null_dma_buf pointer incorrectly, and it causes memory corruption later if DMA usage is enabled. Signed-off-by: Vasily Khoruzhick anars...@gmail.com --- drivers/spi/pxa2xx_spi.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/drivers/spi/pxa2xx_spi.c b/drivers/spi/pxa2xx_spi.c index dc25bee..ef38fbf 100644 --- a/drivers/spi/pxa2xx_spi.c +++ b/drivers/spi/pxa2xx_spi.c @@ -1569,7 +1569,7 @@ static int __devinit pxa2xx_spi_probe(struct platform_device *pdev) master-transfer = transfer; drv_data-ssp_type = ssp-type; - drv_data-null_dma_buf = (u32 *)ALIGN((u32)(drv_data + + drv_data-null_dma_buf = (u32 *)ALIGN(((u32)drv_data + sizeof(struct driver_data)), 8); This thing looks a bit disturbing in itself. Like, where the heck is that thing pointing in the end ? Since some data are written to address in null_dma_buf ... isn't this just changing the corruption impact ? /* Allocate master with space for drv_data and null dma buffer */ master = spi_alloc_master(dev, sizeof(struct driver_data) + 16); So there's 16 bytes at the end of driver_data. However: (u32)(drv_data + sizeof(struct driver_data)) is pointer arithmetic. drv_data points at an object of sizeof(struct driver_data). Adding one to this increments the pointer by sizeof(struct driver_data) bytes. So the above expression increments the pointer by sizeof(struct driver_data)*sizeof(struct driver_data) bytes, which is obviously complete rubbish. ((u32)drv_data + sizeof(struct driver_data)) casts drv_data to a u32 first, then adds the sizeof(struct driver_data) which moves us into the 16 bytes allocated off the end of the struct. The ptr arritmetic is clear, it was the 16 bytes after the structure I was missing ... if it's allocated there, it's fine. Thanks for clearing this. Actually ... why don't we just add char null_buf[16] at the end of the structure instead of allocating +16 bytes and then doing this strange crap ? Or even better ... kzalloc() the buffer in probe(), then kfree() it in remove()? -- All of the data generated in your IT infrastructure is seriously valuable. Why? It contains a definitive record of application performance, security threats, fraudulent activity, and more. Splunk takes this data and makes sense of it. IT sense. And common sense. http://p.sf.net/sfu/splunk-d2d-c2 ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH 1/1] merging arch/arm/include/asm/mach/flash.h and include/linux/spi/flash.h into single header file include/linux/mtd/flash.h
On Mon, Jan 31, 2011 at 12:29:11PM +0100, Alexis RODET wrote:
Signed-off-by: Alexis RODET alexis.ro...@bvs-tech.com
No commit description? You should always describe what you're doing and why.
I think I'm okay with this. What build testing have you performed?
g.
---
arch/arm/include/asm/mach/flash.h | 35 +
include/linux/mtd/flash.h | 51
+
include/linux/spi/flash.h | 27 +---
3 files changed, 53 insertions(+), 60 deletions(-)
diff --git a/arch/arm/include/asm/mach/flash.h
b/arch/arm/include/asm/mach/flash.h
index 4ca69fe..28bd58b 100644
--- a/arch/arm/include/asm/mach/flash.h
+++ b/arch/arm/include/asm/mach/flash.h
@@ -1,39 +1,6 @@
-/*
- * arch/arm/include/asm/mach/flash.h
- *
- * Copyright (C) 2003 Russell King, All Rights Reserved.
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 as
- * published by the Free Software Foundation.
- */
#ifndef ASMARM_MACH_FLASH_H
#define ASMARM_MACH_FLASH_H
-struct mtd_partition;
-struct mtd_info;
-
-/*
- * map_name: the map probe function name
- * name: flash device name (eg, as used with mtdparts=)
- * width:width of mapped device
- * init: method called at driver/device initialisation
- * exit: method called at driver/device removal
- * set_vpp: method called to enable or disable VPP
- * mmcontrol:method called to enable or disable Sync. Burst Read in
OneNAND
- * parts:optional array of mtd_partitions for static partitioning
- * nr_parts: number of mtd_partitions for static partitoning
- */
-struct flash_platform_data {
- const char *map_name;
- const char *name;
- unsigned intwidth;
- int (*init)(void);
- void(*exit)(void);
- void(*set_vpp)(int on);
- void(*mmcontrol)(struct mtd_info *mtd, int sync_read);
- struct mtd_partition *parts;
- unsigned intnr_parts;
-};
+#include linux/mtd/flash.h
#endif
diff --git a/include/linux/mtd/flash.h b/include/linux/mtd/flash.h
new file mode 100644
index 000..5379eee
--- /dev/null
+++ b/include/linux/mtd/flash.h
@@ -0,0 +1,51 @@
+/*
+ * include/linux/mtd/flash.h
+ * merged from arch/arm/include/asm/mach/flash.h and
include/linux/spi/flash.h
+ *
+ * Copyright (C) 2003 Russell King, All Rights Reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+#ifndef LINUX_MTD_FLASH_H
+#define LINUX_MTD_FLASH_H
+
+struct mtd_partition;
+struct mtd_info;
+
+/**
+ * struct flash_platform_data: board-specific flash data
+ * @map_name:the map probe function name
+ * @name:optional flash device name (eg, as used with mtdparts=)
+ * @width: width of mapped device
+ * @init:method called at driver/device initialisation
+ * @exit:method called at driver/device removal
+ * @set_vpp: method called to enable or disable VPP
+ * @mmcontrol: method called to enable or disable Sync. Burst Read in
OneNAND
+ * @parts: optional array of mtd_partitions for static partitioning
+ * @nr_parts:number of mtd_partitions for static partitoning
+ * @type:optional flash device type (e.g. m25p80 vs m25p64), for use
+ * with chips that can't be queried for JEDEC or other IDs
+ *
+ * Board init code (in arch/.../mach-xxx/board-yyy.c files) can
+ * provide information about SPI flash parts (such as DataFlash) to
+ * help set up the device and its appropriate default partitioning.
+ *
+ * Note that for DataFlash, sizes for pages, blocks, and sectors are
+ * rarely powers of two; and partitions should be sector-aligned.
+ */
+struct flash_platform_data {
+ const char *map_name;
+ const char *name;
+ unsigned intwidth;
+ int (*init)(void);
+ void(*exit)(void);
+ void(*set_vpp)(int on);
+ void(*mmcontrol)(struct mtd_info *mtd, int sync_read);
+ struct mtd_partition *parts;
+ unsigned intnr_parts;
+ const char *type;
+};
+
+#endif
diff --git a/include/linux/spi/flash.h b/include/linux/spi/flash.h
index 3f22932..c7ed65e 100644
--- a/include/linux/spi/flash.h
+++ b/include/linux/spi/flash.h
@@ -1,31 +1,6 @@
#ifndef LINUX_SPI_FLASH_H
#define LINUX_SPI_FLASH_H
-struct mtd_partition;
-
-/**
- * struct flash_platform_data: board-specific flash data
- * @name: optional flash device name (eg, as used with mtdparts=)
- * @parts: optional array of mtd_partitions for static partitioning
- * @nr_parts: number of mtd_partitions for static partitoning
- * @type: optional flash device type (e.g. m25p80 vs
[PATCH v2] pxa2xx_spi: fix memory corruption
pxa2xx_spi_probe allocates struct driver_data and null_dma_buf
at same time via spi_alloc_master(), but then calculates
null_dma_buf pointer incorrectly, and it causes memory corruption
later if DMA usage is enabled.
Signed-off-by: Vasily Khoruzhick anars...@gmail.com
---
v2: - add u8 __null_dma_buf[16] to the end of driver_data structure
and use it as null_dma_buf after alignment.
- use PTR_ALIGN instead of ALIGN
drivers/spi/pxa2xx_spi.c |9 +
1 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/spi/pxa2xx_spi.c b/drivers/spi/pxa2xx_spi.c
index dc25bee..25358cd 100644
--- a/drivers/spi/pxa2xx_spi.c
+++ b/drivers/spi/pxa2xx_spi.c
@@ -106,6 +106,7 @@ struct driver_data {
int rx_channel;
int tx_channel;
u32 *null_dma_buf;
+ u8 __null_dma_buf[16];
/* SSP register addresses */
void __iomem *ioaddr;
@@ -1543,8 +1544,8 @@ static int __devinit pxa2xx_spi_probe(struct
platform_device *pdev)
return -ENODEV;
}
- /* Allocate master with space for drv_data and null dma buffer */
- master = spi_alloc_master(dev, sizeof(struct driver_data) + 16);
+ /* Allocate master with space for drv_data */
+ master = spi_alloc_master(dev, sizeof(struct driver_data));
if (!master) {
dev_err(pdev-dev, cannot alloc spi_master\n);
pxa_ssp_free(ssp);
@@ -1569,8 +1570,8 @@ static int __devinit pxa2xx_spi_probe(struct
platform_device *pdev)
master-transfer = transfer;
drv_data-ssp_type = ssp-type;
- drv_data-null_dma_buf = (u32 *)ALIGN((u32)(drv_data +
- sizeof(struct driver_data)), 8);
+ drv_data-null_dma_buf =
+ (u32 *)PTR_ALIGN((u8 *)drv_data-__null_dma_buf, 8);
drv_data-ioaddr = ssp-mmio_base;
drv_data-ssdr_physical = ssp-phys_base + SSDR;
--
1.7.5.rc3
--
All of the data generated in your IT infrastructure is seriously valuable.
Why? It contains a definitive record of application performance, security
threats, fraudulent activity, and more. Splunk takes this data and makes
sense of it. IT sense. And common sense.
http://p.sf.net/sfu/splunk-d2d-c2
___
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
Re: [PATCH v2] pxa2xx_spi: fix memory corruption
On Sunday, July 10, 2011 02:09:22 PM Vasily Khoruzhick wrote:
pxa2xx_spi_probe allocates struct driver_data and null_dma_buf
at same time via spi_alloc_master(), but then calculates
null_dma_buf pointer incorrectly, and it causes memory corruption
later if DMA usage is enabled.
Signed-off-by: Vasily Khoruzhick anars...@gmail.com
---
v2: - add u8 __null_dma_buf[16] to the end of driver_data structure
and use it as null_dma_buf after alignment.
- use PTR_ALIGN instead of ALIGN
drivers/spi/pxa2xx_spi.c |9 +
1 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/spi/pxa2xx_spi.c b/drivers/spi/pxa2xx_spi.c
index dc25bee..25358cd 100644
--- a/drivers/spi/pxa2xx_spi.c
+++ b/drivers/spi/pxa2xx_spi.c
@@ -106,6 +106,7 @@ struct driver_data {
int rx_channel;
int tx_channel;
u32 *null_dma_buf;
+ u8 __null_dma_buf[16];
Ah, please don't name it starting with two underscores.
/* SSP register addresses */
void __iomem *ioaddr;
@@ -1543,8 +1544,8 @@ static int __devinit pxa2xx_spi_probe(struct
platform_device *pdev) return -ENODEV;
}
- /* Allocate master with space for drv_data and null dma buffer */
- master = spi_alloc_master(dev, sizeof(struct driver_data) + 16);
+ /* Allocate master with space for drv_data */
+ master = spi_alloc_master(dev, sizeof(struct driver_data));
if (!master) {
dev_err(pdev-dev, cannot alloc spi_master\n);
pxa_ssp_free(ssp);
@@ -1569,8 +1570,8 @@ static int __devinit pxa2xx_spi_probe(struct
platform_device *pdev) master-transfer = transfer;
drv_data-ssp_type = ssp-type;
- drv_data-null_dma_buf = (u32 *)ALIGN((u32)(drv_data +
- sizeof(struct driver_data)), 8);
+ drv_data-null_dma_buf =
+ (u32 *)PTR_ALIGN((u8 *)drv_data-__null_dma_buf, 8);
Do you need that (u8 *) cast there ?
#define PTR_ALIGN(p, a) ((typeof(p))ALIGN((unsigned long)(p), (a)))
from linux/kernel.h line 42
drv_data-ioaddr = ssp-mmio_base;
drv_data-ssdr_physical = ssp-phys_base + SSDR;
--
All of the data generated in your IT infrastructure is seriously valuable.
Why? It contains a definitive record of application performance, security
threats, fraudulent activity, and more. Splunk takes this data and makes
sense of it. IT sense. And common sense.
http://p.sf.net/sfu/splunk-d2d-c2
___
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
[PATCH v3] pxa2xx_spi: fix memory corruption
pxa2xx_spi_probe allocates struct driver_data and null_dma_buf
at same time via spi_alloc_master(), but then calculates
null_dma_buf pointer incorrectly, and it causes memory corruption
later if DMA usage is enabled.
Signed-off-by: Vasily Khoruzhick anars...@gmail.com
---
v2: - add u8 __null_dma_buf[16] to the end of driver_data structure
and use it as null_dma_buf after alignment.
- use PTR_ALIGN instead of ALIGN
v3: - drop (u8 *) cast, use operator instead, change array name
drivers/spi/pxa2xx_spi.c |9 +
1 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/spi/pxa2xx_spi.c b/drivers/spi/pxa2xx_spi.c
index dc25bee..b25fe27 100644
--- a/drivers/spi/pxa2xx_spi.c
+++ b/drivers/spi/pxa2xx_spi.c
@@ -106,6 +106,7 @@ struct driver_data {
int rx_channel;
int tx_channel;
u32 *null_dma_buf;
+ u8 null_dma_buf_unaligned[16];
/* SSP register addresses */
void __iomem *ioaddr;
@@ -1543,8 +1544,8 @@ static int __devinit pxa2xx_spi_probe(struct
platform_device *pdev)
return -ENODEV;
}
- /* Allocate master with space for drv_data and null dma buffer */
- master = spi_alloc_master(dev, sizeof(struct driver_data) + 16);
+ /* Allocate master with space for drv_data */
+ master = spi_alloc_master(dev, sizeof(struct driver_data));
if (!master) {
dev_err(pdev-dev, cannot alloc spi_master\n);
pxa_ssp_free(ssp);
@@ -1569,8 +1570,8 @@ static int __devinit pxa2xx_spi_probe(struct
platform_device *pdev)
master-transfer = transfer;
drv_data-ssp_type = ssp-type;
- drv_data-null_dma_buf = (u32 *)ALIGN((u32)(drv_data +
- sizeof(struct driver_data)), 8);
+ drv_data-null_dma_buf =
+ (u32 *)PTR_ALIGN(drv_data-null_dma_buf_unaligned, 8);
drv_data-ioaddr = ssp-mmio_base;
drv_data-ssdr_physical = ssp-phys_base + SSDR;
--
1.7.5.rc3
--
All of the data generated in your IT infrastructure is seriously valuable.
Why? It contains a definitive record of application performance, security
threats, fraudulent activity, and more. Splunk takes this data and makes
sense of it. IT sense. And common sense.
http://p.sf.net/sfu/splunk-d2d-c2
___
spi-devel-general mailing list
spi-devel-general@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/spi-devel-general
[SPAM] 100 Tirages Photos pour 5 euros -65%
Pour voir le message, veuillez utiliser un lecteur de mail compatible HTML Lien miroir : http://m10-fr.com/miroir.php?param=YT0xMyZiPTU1NCZjPTM5MzM3NjEmZD0yMDExLTA3LTEwIDE3OjIwOjAxJmU9MSZmPTU1NCZnPTU1NCZoPTU1NA== Lien de désinscription : http://m10-fr.com/unsubscribe.php?param=YT0xMyZiPTU1NCZjPTM5MzM3NjEmZD0yMDExLTA3LTEwIDE3OjIwOjAxJmU9MSZmPTU1NCZnPTU1NCZoPTU1NA== -- All of the data generated in your IT infrastructure is seriously valuable. Why? It contains a definitive record of application performance, security threats, fraudulent activity, and more. Splunk takes this data and makes sense of it. IT sense. And common sense. http://p.sf.net/sfu/splunk-d2d-c2 ___ spi-devel-general mailing list spi-devel-general@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/spi-devel-general
