| Message ID | 1310299762-20079-1-git-send-email-anarsoul@gmail.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
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 <anarsoul@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
On Sunday 10 July 2011 15:43:41 Marek Vasut wrote: > 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 <anarsoul@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. Ok, what name do you suggest? > > /* 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 ? Yes, cast is necessary here, otherwise: drivers/spi/pxa2xx_spi.c:1574:10: error: cast specifies array type > #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
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;
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 <anarsoul@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(-)