Re: [PATCH] sound: memalloc.h - fix wrong truncation of dma_addr_t

[Takashi Iwai <tiwai@suse.de>]

At Tue, 10 Dec 2013 14:20:28 +0000,
Stefano Panella wrote:
> 
> When running a 32bit kernel the hda_intel driver is still reporting
> a 64bit dma_mask if the HW supports it.
> 
> >From sound/pci/hda/hda_intel.c:
> 
>         /* allow 64bit DMA address if supported by H/W */
>         if ((gcap & ICH6_GCAP_64OK) && !pci_set_dma_mask(pci, DMA_BIT_MASK(64)))
>                 pci_set_consistent_dma_mask(pci, DMA_BIT_MASK(64));
>         else {
>                 pci_set_dma_mask(pci, DMA_BIT_MASK(32));
>                 pci_set_consistent_dma_mask(pci, DMA_BIT_MASK(32));
>         }
> 
> which means when there is a call to dma_alloc_coherent from
> snd_malloc_dev_pages a machine address bigger than 32bit can be returned.
> This can be true in particular if running  the 32bit kernel as a pv dom0
> under the Xen Hypervisor or PAE on bare metal.
> 
> The problem is that when calling setup_bdle to program the BLE the
> dma_addr_t returned from the dma_alloc_coherent is wrongly truncated
> from snd_sgbuf_get_addr if running a 32bit kernel:
> 
> static inline dma_addr_t snd_sgbuf_get_addr(struct snd_dma_buffer *dmab,
>                                            size_t offset)
> {
>         struct snd_sg_buf *sgbuf = dmab->private_data;
>         dma_addr_t addr = sgbuf->table[offset >> PAGE_SHIFT].addr;
>         addr &= PAGE_MASK;
>         return addr + offset % PAGE_SIZE;
> }
> 
> where PAGE_MASK in a 32bit kernel is zeroing the upper 32bit af addr.
> 
> Without this patch the HW will fetch the 32bit truncated address,
> which is not the one obtained from dma_alloc_coherent and will result
> to a non working audio but can corrupt host memory at a random location.
> 
> The current patch apply to v3.13-rc3-74-g6c843f5
> 
> Signed-off-by: Stefano Panella <stefano.panella@citrix.com>
> Reviewed-by: Frediano Ziglio <frediano.ziglio@citrix.com>

Thanks for spotting out.  Applied now (I added Cc to stable).

It might be better to have a common PAGE_SIZE definition aligned to
dma_addr_t, but this patch should be enough for now.


Takashi

> ---
>  include/sound/memalloc.h |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/sound/memalloc.h b/include/sound/memalloc.h
> index af99839..5f73785 100644
> --- a/include/sound/memalloc.h
> +++ b/include/sound/memalloc.h
> @@ -108,7 +108,7 @@ static inline dma_addr_t snd_sgbuf_get_addr(struct snd_dma_buffer *dmab,
>  {
>  	struct snd_sg_buf *sgbuf = dmab->private_data;
>  	dma_addr_t addr = sgbuf->table[offset >> PAGE_SHIFT].addr;
> -	addr &= PAGE_MASK;
> +	addr &= ~((dma_addr_t)PAGE_SIZE - 1);
>  	return addr + offset % PAGE_SIZE;
>  }
>  
> -- 
> 1.7.9.5
>