From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vasily Khoruzhick Subject: Re: [PATCH v2] pxa2xx_spi: fix memory corruption Date: Sun, 10 Jul 2011 16:09:30 +0300 Message-ID: <201107101609.31405.anarsoul@gmail.com> References: <201107100957.06377.marek.vasut@gmail.com> <1310299762-20079-1-git-send-email-anarsoul@gmail.com> <201107101443.42307.marek.vasut@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Cc: spi-devel-general@lists.sourceforge.net, Russell King - ARM Linux , Eric Miao , linux-arm-kernel@lists.infradead.org To: Marek Vasut Return-path: In-Reply-To: <201107101443.42307.marek.vasut@gmail.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: linux-arm-kernel-bounces@lists.infradead.org Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=m.gmane.org@lists.infradead.org List-Id: linux-spi.vger.kernel.org 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 > > --- > > 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;