linux-kernel - Re: [PATCH v2 -resend#1 1/1] V4L: videobuf, don't use dma addr as physical

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:	Mon, 21 Mar 2011 19:43:07 -0300
From:	Mauro Carvalho Chehab <maurochehab@...il.com>
To:	Jiri Slaby <jslaby@...e.cz>
CC:	Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>,
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
	jirislaby@...il.com
Subject: Re: [PATCH v2 -resend#1 1/1] V4L: videobuf, don't use dma addr as
 physical

Em 28-02-2011 15:20, Mauro Carvalho Chehab escreveu:
> Em 28-02-2011 12:47, Jiri Slaby escreveu:
>> On 02/28/2011 03:53 PM, Konrad Rzeszutek Wilk wrote:
>>> On Mon, Feb 28, 2011 at 10:37:02AM +0100, Jiri Slaby wrote:
>>>> mem->dma_handle is a dma address obtained by dma_alloc_coherent which
>>>> needn't be a physical address in presence of IOMMU. So ensure we are
>>>
>>> Can you add a comment why you are fixing it? Is there a bug report for this?
>>> Under what conditions did you expose this fault?
>>
>> No, by a just peer review when I was looking for something completely
>> different.
>>
>>> You also might want to mention that "needn't be a physical address as
>>> a hardware IOMMU can (and most likely) will return a bus address where
>>> physical != bus address."
>>
>> Mauro, do you want me to resend this with such an udpate in the changelog?
> 
> Having it properly documented is always a good idea, especially since a similar
> fix might be needed on other drivers that also need contiguous memory. While it
> currently is used only on devices embedded on hardware with no iommu, there are
> some x86 hardware that doesn't allow DMA scatter/gather.
> 
> Btw, it may be worth to take a look at vb2 dma contig module, as it might have
> similar issues.
> 
>>> Otherwise you can stick 'Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>'
>>> on it.
> 
> Cheers,
> Mauro
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

As I got no return, and the patch looked sane, I've reviewed the comment myself,
in order to make it more explicit, as suggested by Konrad, and added his
reviewed-by: tag:

Author: Jiri Slaby <jslaby@...e.cz>
Date:   Mon Feb 28 06:37:02 2011 -0300

    [media] V4L: videobuf, don't use dma addr as physical
    
    mem->dma_handle is a dma address obtained by dma_alloc_coherent which
    needn't be a physical address in presence of IOMMU, as
    a hardware IOMMU can (and most likely) will return a bus address where
    physical != bus address.
    
    So ensure we are remapping (remap_pfn_range) the right page in
    __videobuf_mmap_mapper by using virt_to_phys(mem->vaddr) and not
    mem->dma_handle.
    
    While at it, use PFN_DOWN instead of explicit shift.
    
    Signed-off-by: Jiri Slaby <jslaby@...e.cz>
    Reviewed-by: Konrad Rzeszutek Wilk <konrad.wilk@...cle.com>
    Signed-off-by: Mauro Carvalho Chehab <mchehab@...hat.com>

diff --git a/drivers/media/video/videobuf-dma-contig.c b/drivers/media/video/videobuf-dma-contig.c
index c969111..19d3e4a 100644
--- a/drivers/media/video/videobuf-dma-contig.c
+++ b/drivers/media/video/videobuf-dma-contig.c
@@ -300,7 +300,7 @@ static int __videobuf_mmap_mapper(struct videobuf_queue *q,
 
        vma->vm_page_prot = pgprot_noncached(vma->vm_page_prot);
        retval = remap_pfn_range(vma, vma->vm_start,
-                            mem->dma_handle >> PAGE_SHIFT,
+                          PFN_DOWN(virt_to_phys(mem->vaddr))
                                 size, vma->vm_page_prot);
        if (retval) {
                dev_err(q->dev, "mmap: remap failed with error %d. ", retval);
--
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/