[PATCH] libata: fix DMA to stack in reading devslp_timing parameters

[PATCH] libata: fix DMA to stack in reading devslp_timing parameters

[David Woodhouse]

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

Commit 803739d25c2343da6d2f95eebdcbc08bf67097d4 ("[libata] replace
sata_settings with devslp_timing"), which was also Cc: stable, used a
stack buffer to receive data from ata_read_log_page(), which triggers
the following warning:
 ahci 0000:00:1f.2: DMA-API: device driver maps memory fromstack [addr=ffff880140469948]

Fix this by using ap->sector_buf instead of a stack buffer.

Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
Cc: stable@kernel.org

---
I'm somewhat ignorant, but it looks like this is kind of thing is what
ap->sector_buf is *for*, right? And in ata_dev_configure() nobody else
is likely to be using it?

diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
index 497adea..b1160ee 100644
--- a/drivers/ata/libata-core.c
+++ b/drivers/ata/libata-core.c
@@ -2329,7 +2329,7 @@ int ata_dev_configure(struct ata_device *dev)
 		 * from SATA Settings page of Identify Device Data Log.
 		 */
 		if (ata_id_has_devslp(dev->id)) {
-			u8 sata_setting[ATA_SECT_SIZE];
+			u8 *sata_setting = ap->sector_buf;
 			int i, j;
 
 			dev->flags |= ATA_DFLAG_DEVSLP;

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

RE: [PATCH] libata: fix DMA to stack in reading devslp_timing parameters

[Huang, Shane]

Hi David,

> Fix this by using ap->sector_buf instead of a stack buffer.
> 
> ---
> I'm somewhat ignorant, but it looks like this is kind of thing is what
> ap->sector_buf is *for*, right? And in ata_dev_configure() nobody else
> is likely to be using it?

Your fix is fine with me, thanks. I didn't notice this warning since
CONFIG_DMA_API_DEBUG was not set here.

Regards,
Shane

Re: [PATCH] libata: fix DMA to stack in reading devslp_timing parameters

[David Woodhouse]

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

On Fri, 2013-03-29 at 11:54 +0000, David Woodhouse wrote:
> Commit 803739d25c2343da6d2f95eebdcbc08bf67097d4 ("[libata] replace
> sata_settings with devslp_timing"), which was also Cc: stable, used a
> stack buffer to receive data from ata_read_log_page()...

That commit also changes from using ata_id_has_ncq() to
ata_id_has_devslp() as the condition for reading the SATA settings log
page (log 0x30 page 0x08), because of compatibility issues with some
drives which *do* implement NCQ but not DEVSLP.

It's not entirely clear to me that this change was correct. Even if the
drive does support DEVSLP, the timings in the SATA settings log page are
optional, and we're supposed to use default values if they are not
provided. I can imagine some DEVSLP-capable drives not having that page
either.

Shouldn't we be reading page zero of the 0x30 log, which *is* mandatory,
and looking there to see which pages are supported?

-- 
David Woodhouse                            Open Source Technology Centre
David.Woodhouse@intel.com                              Intel Corporation

[-- Attachment #2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 6171 bytes --]

Re: [PATCH] libata: fix DMA to stack in reading devslp_timing parameters

[Jeff Garzik]

On 03/29/2013 07:54 AM, David Woodhouse wrote:
> Commit 803739d25c2343da6d2f95eebdcbc08bf67097d4 ("[libata] replace
> sata_settings with devslp_timing"), which was also Cc: stable, used a
> stack buffer to receive data from ata_read_log_page(), which triggers
> the following warning:
>   ahci 0000:00:1f.2: DMA-API: device driver maps memory fromstack [addr=ffff880140469948]
>
> Fix this by using ap->sector_buf instead of a stack buffer.
>
> Signed-off-by: David Woodhouse <David.Woodhouse@intel.com>
> Cc: stable@kernel.org

applied


> I'm somewhat ignorant, but it looks like this is kind of thing is what
> ap->sector_buf is *for*, right? And in ata_dev_configure() nobody else
> is likely to be using it?

yep