linux-kernel - Re: [Bug 487894] New: sky2 0000:06:00.0: DMA-API: device driver frees DMA memory with different size

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 2 Apr 2009 12:54:15 +0200
From:	Joerg Roedel <joerg.roedel@....com>
To:	Stephen Hemminger <shemminger@...tta.com>
CC:	Dave Jones <davej@...hat.com>, <tglx@...uxtronix.de>,
	<mingo@...hat.com>, <hpa@...or.com>,
	<linux-kernel@...r.kernel.org>, <x86@...nel.org>
Subject: Re: [Bug 487894] New: sky2 0000:06:00.0: DMA-API: device driver
	frees DMA memory with different size

On Wed, Apr 01, 2009 at 11:02:45AM -0700, Stephen Hemminger wrote:
> 
> The sky2 driver uses pci_unmap_len and pci_unmap_len_set which on 32 bit
> platforms are meaningless so they are stubbed out. 
> Basically, DMA-API checks are wrong/bogus to enforce on 32bit x86 as is.

As far as I know the VT-d driver is available on 32 bit x86 too. So this should
not always be a nop.
The other question is why pci_unmap_* functions are not defined as a nop too
and call dma_unmap_* instead? This looks inconsisent to me.

> ========================================================
> Unstub pci_unmap macros if doing DMA-API checks
> 
> If doing device driver DMA-API tests then need to keep track of address/length
> even on 32-bit x86 where the information is not normally needed.
> 
> Signed-off-by: Stephen Hemminger <shemminger@...tta.com>
> 
> 
> --- a/arch/x86/include/asm/pci_32.h	2009-04-01 10:52:07.117504355 -0700
> +++ b/arch/x86/include/asm/pci_32.h	2009-04-01 10:56:02.249066174 -0700
> @@ -17,16 +17,25 @@ struct pci_dev;
>   */
>  #define PCI_DMA_BUS_IS_PHYS	(1)
>  
> +#ifdef CONFIG_DMA_API_DEBUG
> +/* keep real values */
> +#define DECLARE_PCI_UNMAP_ADDR(ADDR_NAME)	dma_addr_t ADDR_NAME;
> +#define DECLARE_PCI_UNMAP_LEN(LEN_NAME)		__u32 LEN_NAME;
> +#define pci_unmap_addr(PTR, ADDR_NAME)		((PTR)->ADDR_NAME)
> +#define pci_unmap_addr_set(PTR, ADDR_NAME, VAL)	(((PTR)->ADDR_NAME) = (VAL))
> +#define pci_unmap_len(PTR, LEN_NAME)		((PTR)->LEN_NAME)
> +#define pci_unmap_len_set(PTR, LEN_NAME, VAL)	(((PTR)->LEN_NAME) = (VAL))
> +#else
>  /* pci_unmap_{page,single} is a nop so... */
>  #define DECLARE_PCI_UNMAP_ADDR(ADDR_NAME)	dma_addr_t ADDR_NAME[0];
> -#define DECLARE_PCI_UNMAP_LEN(LEN_NAME)	unsigned LEN_NAME[0];
> -#define pci_unmap_addr(PTR, ADDR_NAME)	sizeof((PTR)->ADDR_NAME)
> +#define DECLARE_PCI_UNMAP_LEN(LEN_NAME)		__u32 LEN_NAME[0];
> +#define pci_unmap_addr(PTR, ADDR_NAME)		sizeof((PTR)->ADDR_NAME)
>  #define pci_unmap_addr_set(PTR, ADDR_NAME, VAL) \
>  	do { break; } while (pci_unmap_addr(PTR, ADDR_NAME))
>  #define pci_unmap_len(PTR, LEN_NAME)		sizeof((PTR)->LEN_NAME)
>  #define pci_unmap_len_set(PTR, LEN_NAME, VAL) \
>  	do { break; } while (pci_unmap_len(PTR, LEN_NAME))
> -
> +#endif

I think we need another solution which takes into account that there might be
VT-d enabled and which defines the pci_unmap_* functions also as a nop when it
is not. I suggest to make a CONFIG option for these functions nit being a nop
and let DMA_API_DEBUG select it on x86. So we get proper checking for drivers
on 32bit x86 too which helps other architectures where these drivers can be
used.

	Joerg

-- 
           | Advanced Micro Devices GmbH
 Operating | Karl-Hammerschmidt-Str. 34, 85609 Dornach bei München
 System    | 
 Research  | Geschäftsführer: Jochen Polster, Thomas M. McCoy, Giuliano Meroni
 Center    | Sitz: Dornach, Gemeinde Aschheim, Landkreis München
           | Registergericht München, HRB Nr. 43632

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/